[ECOS] RedBoot DHCP failure due to race condition.

[Grant Edwards]

We've been having intermittent problems with RedBoot's DHCP client
failing to acquire an address.  I've tracked it down to what looks
like a race condition in RedBoot's DHCP code.

The failure mostly happens when the DHCP state machine has reached the
"ACK" state and the "ACK" BOOTP packet has been stored at *bp_info.

If another BOOTP packet arrives before the foreground loop has had a
chance to process the stuff stored at bp_info, that packet will
overwrite what's stored at *bp_info even though the rx state machine
will later decide to "ignore" the packet -- it's already blown away
the packet we care about.  Since we're in the ACK state, the
foreground loop decides we're done, and it doesn't retry.

The basic problem is that _all_ received BOOTP packets are stored at
*bp_info by the code starting at bootp.c line 92:

    82	static void
    83	bootp_handler(udp_socket_t *skt, char *buf, int len,
    84	              ip_route_t *src_route, word src_port)
    85	{
    86	    bootp_header_t *b;
    87	#ifdef CYGSEM_REDBOOT_NETWORKING_DHCP
    88	    unsigned char *p, expected = 0;
    89	#endif
    90	
    91	    b = (bootp_header_t *)buf;
    92	    if (bp_info) {
    93	        memset(bp_info,0,sizeof *bp_info);
    94	        if (len > sizeof *bp_info)
    95	            len = sizeof *bp_info;
    96	        memcpy(bp_info, b, len);
    97	    }
    98	
    99	    // Only accept pure REPLY responses
   100	    if (b->bp_op != BOOTREPLY)
   101	      return;
   102	    
   103	    // Must be sent to me, as well!
   104	    if (memcmp(b->bp_chaddr, __local_enet_addr, 6))
   105	      return;
   106	        
   [...]    [ ... DHCP state machine stuff ... ]

  
AFAICT, received BOOTP packets overwrite *bp_info regardless of what
state we're in, whether or not they're BOOTP "REPLY" packets, and
*bp_info even gets overwritten by packets that aren't addressed to us.

Am I misunderstanding the code?

I've gotten RedBoot DHCP to work more reliably by changing line 92 to:

    92	    if (bp_info && (dhcpState != DHCP_ACK)) {


While that's an improvement, I don't think that fix is sufficient
since bogus BOOTP packets can still incorrectly overwrite *bp_info in
other states.

Shouldn't we store the received packet in *bp_info only after we've
checked to make sure it's a BOOTP REPLY packet and it's addressed to
us?

Better yet, only store it if the DHCP type is what we're expecting for
the current state?

-- 
Grant


-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss