[Bug 1001177] New: Redboot DHCP client race condition, XID, and retry problems.

[Bug 1001177] New: Redboot DHCP client race condition, XID, and retry problems.

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001177

           Summary: Redboot DHCP client race condition, XID, and retry
                    problems.
           Product: eCos
           Version: CVS
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: normal
          Priority: low
         Component: RedBoot
        AssignedTo: unassigned@bugs.ecos.sourceware.org
        ReportedBy: grant.b.edwards@gmail.com
                CC: ecos-bugs@ecos.sourceware.org
             Class: Advice Request


Redboot's DHCP implementation has multiple problems:

 1) Received BOOTP packets overwrite the stored configuration
    information regardless of their XID, the destination MAC address,
    the type of the packet, or the state of the DHCP FSM.

    Because of they way the state machine and packet handling is split
    between the receive packet handler and the foreground loop, this
    can result in corruption of the IP configuration and failure to
    obtian an IP address.

 2) The retry mechanism doesn't work.  The retry counter is
    decremented at an inappropriate point in the code -- resulting in
    the counter being decremented multiple times during a single DHCP
    transaction.  At one point in the transaction the retry counter
    also gets reset back to it's max value.  Between these two bugs
    there are failure cases where no retries are performed at all and
    cases where the state machine goes into an infinite loop retrying
    indefinitely.

 3) Although the DHCP spec says the XID should be chosen so as to be
    unique, the XID used is a hard-wired constant that's always the
    same -- this can result in problems if multiple devices are
    started simultaneously.

 4) The XID on received packets is not verified, and the XID sent in
    the REQUEST packet is copied from the OFFER packet regardless of
    what the original XID was in the DISCOVER packet.

I've completely re-written the BOOTP/DHCP support to fix the above
problems and an currently testing it.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001177] Redboot DHCP client race condition, XID, and retry problems.

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001177

Grant Edwards <grant.b.edwards@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1182|                            |assignment+, review?
               Flag|                            |

--- Comment #1 from Grant Edwards <grant.b.edwards@gmail.com> 2011-03-24 14:35:45 GMT ---
Created an attachment (id=1182)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1182)
Patch containing RedBoot BOOTP/DHCP rewrite.

Attached patch fixes race condition and retry mechanism.

It also adds verbose output
CYGSEM_REDBOOT_NETWORKING_VERBOSE_BOOTPDHCP.

It's been tested on a custom ARM board and under Qemu i386 with a
variety of DHCP servers (running on both Unix and Windows).

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001177] Redboot DHCP client race condition, XID, and retry problems.

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001177

Jonathan Larmour <jifl@ecoscentric.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |NEEDINFO
                 CC|                            |jifl@ecoscentric.com

--- Comment #2 from Jonathan Larmour <jifl@ecoscentric.com> 2011-03-24 17:36:49 GMT ---
Some comments: 

- in changelog "fixrace" -> "fix race". Also reference this issue number.

- It would be nice (although admittedly arguably an enhancement) for all of
bootp/dhcp support to be removable by config option. There are many instances
where redboot is only ever given a static IP, so this code is just taking up
space.

If that happens, obviously a few existing CDL config options would then live
under that CDL component.

I notice (in existing code, not your patch) that
CYGSEM_REDBOOT_NETWORKING_USE_GATEWAY depends on
CYGSEM_REDBOOT_NETWORKING_DHCP, which seems spurious to me.

- In various places in the patch, constants like 4 or 6 are used. While
entirely accurate, and they'll never change, from the principle of
self-documenting code it would be nicer if they were things like e.g.
sizeof(b->bp_chaddr) or sizeof(__local_enet_addr) etc. I admit I'm being a bit
idealistic.

- It's not clear to me in bootp_handler that this supports BOOTP even when
CYGSEM_REDBOOT_NETWORKING_DHCP is enabled. Can you confirm it does? Just
because CYGSEM_REDBOOT_NETWORKING_DHCP is enabled, shouldn't mean that we can't
work with BOOTP any more. This may have been a problem in the previous code
too.

- You now depend on the CRC code for xid generation. That's quite a chunky
dependency to introduce, and slightly pointless given there's very little
randomness from every byte of the MAC address. I think it would be good enough
to just take the lower 4 bytes of the MAC (or if you prefer, take the bottom
four, and then XOR in the top 2). You could maybe XOR in MS_TICKS() for a
little more entropy, although not much I agree. Or for a bit more, XOR in &xid
and &__bootp_find_local_ip thus making it more specific to the specific build
of the executable.

I know that CYGPKG_CRC comes from the template, but that doesn't mean the code
is included - only if it's used. Admittedly we should also add on explicit CDL
dependencies where it's already been used - it's rather lax that that hasn't
been done for CYGBLD_BUILD_REDBOOT_WITH_CKSUM and CYGSEM_REDBOOT_FIS_CRC_CHECK.

Formally, I'll mark this patch as not having passed review, even though the
changes are minor. Also please don't set the assignment flag until we hear from
the FSF. I still haven't had a reply from them. I have no doubt you filled in
the assignment, but we've had things before when a company has made changes to
the assignment text, or changes to the copyright disclaimer, or has omitted the
disclaimer entirely in what they sent. So I want to allow for the possibility
that something goes wrong. The copyright is only actually assigned once the FSF
executes the assignment on their side too, so until then, the copyright still
rests with you, even if you've signed.

Jifl

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001177] Redboot DHCP client race condition, XID, and retry problems.

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001177

Jonathan Larmour <jifl@ecoscentric.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned@bugs.ecos.source |jifl@ecoscentric.com
                   |ware.org                    |

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001177] Redboot DHCP client race condition, XID, and retry problems.

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001177

Jonathan Larmour <jifl@ecoscentric.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1182|assignment+, review?        |assignment-, review-
               Flag|                            |

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001177] New: Redboot DHCP client race condition, XID, and retry problems.

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001177

           Summary: Redboot DHCP client race condition, XID, and retry
                    problems.
           Product: eCos
           Version: CVS
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: normal
          Priority: low
         Component: RedBoot
        AssignedTo: unassigned@bugs.ecos.sourceware.org
        ReportedBy: grant.b.edwards@gmail.com
                CC: ecos-bugs@ecos.sourceware.org
             Class: Advice Request


Redboot's DHCP implementation has multiple problems:

 1) Received BOOTP packets overwrite the stored configuration
    information regardless of their XID, the destination MAC address,
    the type of the packet, or the state of the DHCP FSM.

    Because of they way the state machine and packet handling is split
    between the receive packet handler and the foreground loop, this
    can result in corruption of the IP configuration and failure to
    obtian an IP address.

 2) The retry mechanism doesn't work.  The retry counter is
    decremented at an inappropriate point in the code -- resulting in
    the counter being decremented multiple times during a single DHCP
    transaction.  At one point in the transaction the retry counter
    also gets reset back to it's max value.  Between these two bugs
    there are failure cases where no retries are performed at all and
    cases where the state machine goes into an infinite loop retrying
    indefinitely.

 3) Although the DHCP spec says the XID should be chosen so as to be
    unique, the XID used is a hard-wired constant that's always the
    same -- this can result in problems if multiple devices are
    started simultaneously.

 4) The XID on received packets is not verified, and the XID sent in
    the REQUEST packet is copied from the OFFER packet regardless of
    what the original XID was in the DISCOVER packet.

I've completely re-written the BOOTP/DHCP support to fix the above
problems and an currently testing it.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.