[PATCH] udhcpc: raw socket cleanup

[Vladislav Grishenko]

Oh, Denis, great it's applied, thank you.

> I think just using it for standard port is ok.
> Realistically, how many users use non-standard port?

I've seen none of them tho,
But sometimes windows dhcp client talks to dhcp server not from 68 port, therefore when udhcpd answers to 68, it gets lost.
Good solution could be respond to the same client port, from where the request was sent, since we trust its unicast source...
Thnx Billy at MS once again.

Best Regards, theMIROn
ICQ: 303357
Skype: the.miron


> -----Original Message-----
> From: Denys Vlasenko [mailto:vda.linux at googlemail.com]
> Sent: Monday, February 14, 2011 8:52 AM
> To: Vladislav Grishenko
> Cc: busybox at busybox.net
> Subject: Re: [PATCH] udhcpc: raw socket cleanup
> 
> On Monday 15 November 2010 09:44, Vladislav Grishenko wrote:
> > > From: Denys Vlasenko
> > > Sent: Monday, November 15, 2010 6:04 AM
> > > To: busybox at busybox.net
> > > Cc: Vladislav Grishenko
> > > Subject: Re: [PATCH] udhcpc: raw socket cleanup
> > >
> > > On Sunday 14 November 2010 22:45, Vladislav Grishenko wrote:
> > > > Hi all, proposed udhcpc patch attached
> > > > Targets:
> > > > 1. Remove dependence from predefined server and client ports
> > >
> > > You mean "use socket filter even if SERVER_PORT != 67 or CLIENT_PORT
> != 68"?
> > > If yes, this does not seem to be a typical use case.
> >
> > Yes, but there'is udhcpc_listen_socket function, which binds to supplied
> client port, even it's not standart.
> > So, why should raw socked binding be an exception?
> >
> > > > 2. Drop ARP packets socket filter pass as redundant feature from
> > > > dhcpclient
> > >
> > > I don't understand this part.
> >
> > Current Berkley packet filter program passes to fd both UDP packets with
> src port == SERVER_PORT, dst port == CLIENT_PORT and ARP packets.
> > I believe it was  used in dhcpclient package, but udhcpc doesn't
> > process ARP responses from that socket, so BPF filter modification now
> passes only UDP packets with corresponding dst port Original code drops
> non-UDP packets at userlevel in udhcpc_recv_raw_packet function.
> >
> > > > 3. Drop fragments 'coz due L4 header absents
> > >
> > > I don't understand this part either.
> > > Looks like you are changing BPF_foo magic. I am not familiar with it.
> > > Can you explain what is changed, and why?
> >
> > Since it's raw socket (actually SOCK_DGRAM to not to include link layer
> headers), fragmented jumbo IP packets aren't reassembled. That means
> only first fragment contains UDP header, and we're unable to do dst port
> check. Moreover, since most DHCP packets are not fragmented due its size,
> reassembling implementation is quite useless here.
> > So, BPF filter modification discards any fragments now at kernel space,
> original code drops them at userlevel in udhcpc_recv_raw_packet function.
> 
> I meant that it should look like this:
> 
>                 /* load 9th byte (protocol) */
>                 BPF_STMT(BPF_LD|BPF_B|BPF_ABS, 9),
>                 /* jump to L1 if it is IPPROTO_UDP, else to L4 */
>                 BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, IPPROTO_UDP, 0, 6),
>                 /* L1: load halfword from offset 6 (flags and frag offset) */
>                 BPF_STMT(BPF_LD|BPF_H|BPF_ABS, 6),
>                 /* jump to L4 if any bits in frag offset are set, else to L2 */
>                 BPF_JUMP(BPF_JMP|BPF_JSET|BPF_K, 0x1fff, 4, 0),
>                 /* L2: skip IP header (load index reg with header len) */
>                 BPF_STMT(BPF_LDX|BPF_B|BPF_MSH, 0),
>                 /* load udp destination port from halfword[header_len + 2] */
>                 BPF_STMT(BPF_LD|BPF_H|BPF_IND, 2),
>                 /* jump to L3 if udp dport is CLIENT_PORT, else to L4 */
>                 BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, 68, 0, 1),
>                 /* L3: accept packet */
>                 BPF_STMT(BPF_RET|BPF_K, 0x0fffffff),
>                 /* L4: discard packet */
>                 BPF_STMT(BPF_RET|BPF_K, 0),
> 
> that is, with comments understandable with mere mortals.
> 
> > > > 4. Attach socket filter after bind to prevent possible
> > > > kernel-dependent leaks
> > >
> > > I don't understand this part too. If it is really important to
> > > attach filter after bond, then it makes sense to add a comment which
> explains why.
> >
> > Every examples I've found attach socket filter after bind, if any, not before.
> > At my side I had non-UDP and wrong ported packets leak over filter on
> heavy load, moving attach filter point is a part of avoiding that issue.
> >
> > > > function                                             old     new
> > > > delta change_listen_mode                                   327     323
> > > > -4 .rodata                                           136378  136306
> > > > -72
> > > > ------------------------------------------------------------------
> > > > ----
> > > > ------
> > > > --
> > > > (add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-76)             Total:
> > > > -76 bytes
> > >
> > > -	static const struct sock_filter filter_instr[] = {
> > > +	static struct sock_filter filter_instr[] = {
> > >
> > > This creates a non-constant object in data section, which exists and
> > > takes space for _every applet_.
> > > I would rather malloc this space if needed.
> >
> > Yep, the only aim was to set dst port in filter from function param.
> > I can rework it with malloc, move from the const struct, set port, set
> > filter, free. Would it be ok?
> 
> I think just using it for standard port is ok.
> 
> Realistically, how many users use non-standard port?
> 
> Here's the commit:
> 
> http://git.busybox.net/busybox/commit/?id=713e6d78e1cb567848805e8dd0
> c9c0cadbfa787a
> 
> Thanks!
> 
> --
> vda