[PATCH] udhcpc: raw socket cleanup

[Vladislav Grishenko]

> 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.

> > 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?

Best Regards, theMIROn