dhcpcd-discuss

> On October 15, 2019 at 4:21 PM Roy Marples <roy@xxxxxxxxxxxx> wrote:
> 
> 
> Hi Giorgio
> On 15/10/2019 09:02, Giorgio Dal Molin wrote:
> > after updating dhcpcd from v8.0.6 to v8.1.0 I'm getting the following udp
> > checksum errors:
> > 
> > ~ # dhcpcd -d eth1
> > dhcpcd-8.1.0 starting
> > eth1: executing `//lib/dhcpcd/dhcpcd-run-hooks' PREINIT
> > eth1: executing `//lib/dhcpcd/dhcpcd-run-hooks' CARRIER
> > DUID 00:04:03:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx
> > eth1: IAID d6:14:c2:b9
> > eth1: adding address fe80::e30:7021:c623:ad0f
> > eth1: pltime infinity, vltime infinity
> > eth1: delaying IPv6 router solicitation for 0.1 seconds
> > eth1: delaying IPv4 for 0.2 seconds
> > eth1: soliciting an IPv6 router
> > eth1: delaying Router Solicitation for LL address
> > eth1: soliciting a DHCP lease
> > eth1: sending DISCOVER (xid 0x65e41544), next in 4.7 seconds
> > eth1: checksum failure from 172.18.4.226
> > eth1: sending Router Solicitation
> > eth1: sending DISCOVER (xid 0x65e41544), next in 8.1 seconds
> > eth1: checksum failure from 172.18.4.226
> > eth1: checksum failure from 172.18.4.226
> > eth1: probing for an IPv4LL address
> > eth1: probing for 169.254.78.153
> > eth1: ARP probing 169.254.78.153 (1 of 3), next in 1.4 seconds
> > eth1: sending Router Solicitation
> > eth1: checksum failure from 172.18.4.225
> > eth1: ARP probing 169.254.78.153 (2 of 3), next in 1.6 seconds
> > eth1: ARP probing 169.254.78.153 (3 of 3), next in 2.0 seconds
> > eth1: sending Router Solicitation
> > eth1: using IPv4LL address 169.254.78.153
> > eth1: adding IP address 169.254.78.153/16 broadcast 169.254.255.255
> > eth1: adding route to 169.254.0.0/16
> > eth1: adding default route
> > eth1: ARP announcing 169.254.78.153 (1 of 2), next in 2.0 seconds
> > eth1: executing `//lib/dhcpcd/dhcpcd-run-hooks' IPV4LL
> > forking to background
> > forked to background, child pid 22198
> > 
> > I've bisected this on the git repo to the following commit:
> > 
> > f198ce2b2b1df1febe0cb64ecd023a5ecb1c0bbf is the first bad commit
> > commit f198ce2b2b1df1febe0cb64ecd023a5ecb1c0bbf
> > Author: Roy Marples <roy@xxxxxxxxxxxx>
> > Date: Fri Oct 11 11:24:38 2019 +0100
> > 
> >  BPF: Move validation logic from BPF to consumers
> >  
> >  Even though we program the BPF filter should we trust it?
> >  On Linux at least there is a window between opening the socket,
> >  binding the interface and setting the filter where we receive data.
> >  This data is NOT checked OR flushed and IS returned when reading.
> >  We have no way of flushing it other than reading these packets!
> >  But we don't know if they passed the filter or not ..... so we need
> >  to validate each and every packet that comes through ourselves as well.
> >  Even if Linux does fix this sorry state, who is to say other kernels
> >  don't have bugs causing a similar effect?
> >  
> >  As such, let's strive to keep the filters just for pattern matching
> >  to avoid waking dhcpcd up.
> > 
> >  src/arp.c | 53 ++++++++++++++++++----------
> >  src/bpf.c | 81 ++++---------------------------------------
> >  src/bpf.h | 19 ++++++++++
> >  src/dhcp.c | 108 ++++++++++++++++++++++++++++-----------------------------
> >  src/if-linux.c | 49 +++++++++++++++-----------
> >  5 files changed, 142 insertions(+), 168 deletions(-)
> > 
> > Inspecting the changes to the function 'checksums_valid()' in 'src/dhcp.c' they
> > look a bit messed up:
> > 
> > /* Lengths have already been checked. */
> > static bool
> > checksums_valid(void *packet,
> >  struct in_addr *from, unsigned int flags)
> > {
> >  struct ip *ip = packet;
> >  struct ip pseudo_ip = {
> >  .ip_p = IPPROTO_UDP,
> >  .ip_src = ip->ip_src,
> >  .ip_dst = ip->ip_dst
> >  };
> >  size_t ip_hlen;
> >  uint16_t udp_len, uh_sum;
> >  struct udphdr *udp;
> >  uint32_t csum;
> > 
> >  if (from != NULL)
> >  from->s_addr = ip->ip_src.s_addr;
> > 
> >  ip_hlen = (size_t)ip->ip_hl * 4;
> >  if (in_cksum(ip, ip_hlen, NULL) != 0)
> >  return false;
> > 
> >  if (flags & BPF_PARTIALCSUM)
> >  return 0;
> > 
> >  udp = (struct udphdr *)(void *)((char *)ip + ip_hlen);
> >  if (udp->uh_sum == 0)
> >  return 0;
> > 
> >  /* UDP checksum is based on a pseudo IP header alongside
> >  * the UDP header and payload. */
> >  udp_len = ntohs(udp->uh_ulen);
> >  uh_sum = udp->uh_sum;
> >  udp->uh_sum = 0;
> >  pseudo_ip.ip_len = udp->uh_ulen;
> >  csum = 0;
> >  in_cksum(&pseudo_ip, sizeof(pseudo_ip), &csum);
> >  csum = in_cksum(udp, udp_len, &csum);
> >  return csum == uh_sum;
> > }
> > 
> > you changed it to return a bool but a couple of return 0; were forgotten,
> > a very 'unfortunate circumstance'.
> 
> Yes, I've already pushed some commits, but this patch is the 
> combination. Does it fix it for you?
> 
> Roy

Hi,

a quick test says no! the current master is still broken, v8.0.6 works.

I'll put a couple of debug printf's in the code and find exactly where it
fails.

giorgio