[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] 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
----------------------------------------------------------------------------
             Status|MODIFIED                    |RESOLVED
         Resolution|                            |NEXTRELEASE

--- Comment #13 from Grant Edwards <grant.b.edwards@gmail.com> 2012-03-06 16:47:08 GMT ---
Commited changes to CVS:

/cvs/ecos/ecos/packages/redboot/current/ChangeLog,v  <--  ChangeLog
new revision: 1.277; previous revision: 1.276

/cvs/ecos/ecos/packages/redboot/current/cdl/redboot.cdl,v  <--  redboot.cdl
new revision: 1.87; previous revision: 1.86

/cvs/ecos/ecos/packages/redboot/current/include/redboot.h,v  <--  redboot.h
new revision: 1.41; previous revision: 1.40

/cvs/ecos/ecos/packages/redboot/current/src/net/bootp.c,v  <--  bootp.c
new revision: 1.26; previous revision: 1.25

/cvs/ecos/ecos/packages/redboot/current/src/net/net_io.c,v  <--  net_io.c
new revision: 1.51; previous revision: 1.50

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

[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|NEEDINFO                    |MODIFIED
         AssignedTo|jifl@ecoscentric.com        |grant.b.edwards@gmail.com

--- Comment #12 from Jonathan Larmour <jifl@ecoscentric.com> 2012-03-03 03:43:45 GMT ---
(In reply to comment #11)
> I just noticed that the state of this is "needinfo".
> 
> Is somebody waiting on something further from me?

Nope. Just dropped off radar presumably because of the FSF assignment delay.
I've looked through the patch from top to bottom again. It's all fine. You have
commit rights, so I suggest you just go ahead and check it in!

The only little thing I'll just ask you to include, just because it was
mentioned despite no longer being related, is to add "requires CYGPKG_CRC" to
each of
CYGBLD_BUILD_REDBOOT_WITH_CKSUM and CYGSEM_REDBOOT_FIS_CRC_CHECK. Just since
you're in the area anyway.

Thanks for all this work!

Jifl

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

[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 #1283|                            |assignment+, review+
               Flag|                            |

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

[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

--- Comment #11 from Grant Edwards <grant.b.edwards@gmail.com> 2012-03-02 21:10:11 GMT ---
I just noticed that the state of this is "needinfo".

Is somebody waiting on something further from me?

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

[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 #1184|0                           |1
        is obsolete|                            |

--- Comment #10 from Grant Edwards <grant.b.edwards@gmail.com> 2011-06-08 17:13:16 BST ---
Created an attachment (id=1283)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1283)
Patch containing BOOTP/DHCP rewrite

Revised patch which fixes bug in code that checks mac address in
bootp packets.  Was comparing the wrong number of bytes.

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

[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|0                           |1
        is obsolete|                            |

--- Comment #9 from Grant Edwards <grant.b.edwards@gmail.com> 2011-03-26 18:01:42 GMT ---
Created an attachment (id=1184)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1184)
Second pass at BOOTP/DHCP rewrite.

Comments are welcome on the second pass at the BOOTP/DHCP rewrite.

Changes from the first pass are:

 1) Miscellaneous cleanup.

 2) Fixed DHCP support so that it works in "fall-back" mode if the
    server only does plain BOOTP and not DHCP.

 3) Created new option CYGPKG_REDBOOT_NETWORKING_BOOTP (default 1)
    which controls whether or not BOOTP/DHCP support (src/net/bootp.c)
    is built.  This new package contains the sub-options:

      CYGSEM_REDBOOT_NETWORKING_DHCP                   (default 1)
      CYGSEM_REDBOOT_NETWORKING_BOOTP_VERBOSE          (default 0)
      CYGSEM_REDBOOT_DEFAULT_NO_BOOTP                  (default 0)

    That last one used to be contained in CYGDAT_REDBOOT_DEFAULT_IP_ADDR.

    Building with no BOOTP-related options specified should result in
    the same behavior as previously.

I've done test builds for i386 PC platform with all combinations of
"absent/0/1" for the following:

  CYGPKG_REDBOOT_NETWORKING_BOOTP
  CYGSEM_REDBOOT_DEFAULT_NO_BOOTP
  CYGSEM_REDBOOT_NETWORKING_DHCP
  CYGSEM_REDBOOT_NETWORKING_BOOTP_VERBOSE
  CYGDAT_REDBOOT_DEFAULT_IP_ADDR

About a dozen of the combinations have been tested with both DHCP and
BOOTP-only servers.

One thing I don't have the ability to test is flash-config support, so
it wouldn't hurt if somebody took a look at that.  I tried building
for the i386 PC platform with CYGSEM_REDBOOT_FLASH_CONFIG enabled, but
that didn't work.

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

[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

--- Comment #8 from Grant Edwards <grant.b.edwards@gmail.com> 2011-03-26 04:20:52 GMT ---
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #2)
> > 
> > > - 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.
> > 
> > I've create a new CYGPKG_REDBOOT_NETWORKING_BOOTP which now contains
> > CYGSEM_REDBOOT_NETWORKING_DHCP and CYGSEM_REDBOOT_NETWORKING_VERBOSE_BOOTP.
> > 
> > I'm wondering about the latter names.  Are names supposed to
> > reflect package heirachy like more like this?
> > 
> >       CYGPKG_REDBOOT_NETWORKING
> >        CYGPKG_REDBOOT_NETWORKING_BOOTP
> >         CYGPKG_REDBOOT_NETWORKING_BOOTP_DHCP
> >         CYGPKG_REDBOOT_NETWORKING_BOOTP_VERBOSE
> 
> While I think that would be the ideal, changing option names (in
> general invalidates people's existing configurations, so shouldn't
> be done lightly.

Agreed.  It had slipped my mind that two of the names were for options
that already existed -- and enhancing the consistency of the names a
bit doesn't justify breaking somebody's previously working
configuration.

> I'd suggest just leaving it. The more important thing is that the
> actual hierarchy of options is correct. But if you want to change
> it, I'm ok with that too.

The two existing names are going to stay the same.

>[...]
> > I don't have a strong opinion one way or the other, but if I were to
> > pick, I'd move CYGSEM_REDBOOT_DEFAULT_NO_BOOTP into
> > CYGPKG_REDBOOT_NETWORKING_BOOTP and make it independent of whether
> > CYGDAT_REDBOOT_DEFAULT_IP_ADDR is set or not.  Who are we to say that
> > no default IP address and bootp built but disabled by default isn't
> > useful to somebody?
> 
> Yes, there seems no reason to prohibit that, so if you didn't mind,
> I think it would be good if you could do that.

Done.

-- 
Grant

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

[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

--- Comment #7 from Jonathan Larmour <jifl@ecoscentric.com> 2011-03-26 02:16:54 GMT ---
(In reply to comment #6)
> (In reply to comment #2)
> 
> > - 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.
> 
> I've create a new CYGPKG_REDBOOT_NETWORKING_BOOTP which now contains
> CYGSEM_REDBOOT_NETWORKING_DHCP and CYGSEM_REDBOOT_NETWORKING_VERBOSE_BOOTP.
> 
> I'm wondering about the latter names.  Are names supposed to reflect
> package heirachy like more like this?
> 
>       CYGPKG_REDBOOT_NETWORKING
>        CYGPKG_REDBOOT_NETWORKING_BOOTP
>         CYGPKG_REDBOOT_NETWORKING_BOOTP_DHCP
>         CYGPKG_REDBOOT_NETWORKING_BOOTP_VERBOSE

While I think that would be the ideal, changing option names (in general
invalidates people's existing configurations, so shouldn't be done lightly. I'd
suggest just leaving it. The more important thing is that the actual hierarchy
of options is correct. But if you want to change it, I'm ok with that too.

> CYGSEM_REDBOOT_DEFAULT_NO_BOOTP is still contained within
> CYGDAT_REDBOOT_DEFAULT_IP_ADDR but now it's active_if
> CYGPKG_REDBOOT_NETWORKING_BOOTP. I'm not sure I like that.

It makes sense to me.

> Should CYGSEM_REDBOOT_DEFAULT_NO_BOOTP be moved into
> CYGPKG_REDBOOT_NETWORKING_BOOTP?

I think so.

[snip] 
> I don't have a strong opinion one way or the other, but if I were to
> pick, I'd move CYGSEM_REDBOOT_DEFAULT_NO_BOOTP into
> CYGPKG_REDBOOT_NETWORKING_BOOTP and make it independent of whether
> CYGDAT_REDBOOT_DEFAULT_IP_ADDR is set or not.  Who are we to say that
> no default IP address and bootp built but disabled by default isn't
> useful to somebody?

Yes, there seems no reason to prohibit that, so if you didn't mind, I think it
would be good if you could do that.

Thanks!

Jifl

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

[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

--- Comment #6 from Grant Edwards <grant.b.edwards@gmail.com> 2011-03-25 16:20:30 GMT ---
(In reply to comment #2)

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

I've create a new CYGPKG_REDBOOT_NETWORKING_BOOTP which now contains
CYGSEM_REDBOOT_NETWORKING_DHCP and CYGSEM_REDBOOT_NETWORKING_VERBOSE_BOOTP.

I'm wondering about the latter names.  Are names supposed to reflect
package heirachy like more like this?

      CYGPKG_REDBOOT_NETWORKING
       CYGPKG_REDBOOT_NETWORKING_BOOTP
        CYGPKG_REDBOOT_NETWORKING_BOOTP_DHCP
        CYGPKG_REDBOOT_NETWORKING_BOOTP_VERBOSE


CYGSEM_REDBOOT_DEFAULT_NO_BOOTP is still contained within
CYGDAT_REDBOOT_DEFAULT_IP_ADDR but now it's active_if
CYGPKG_REDBOOT_NETWORKING_BOOTP. I'm not sure I like that.

Should CYGSEM_REDBOOT_DEFAULT_NO_BOOTP be moved into
CYGPKG_REDBOOT_NETWORKING_BOOTP?

If so, should it be made active_if CYGDAT_REDBOOT_DEFAULT_IP_ADDR, or
should we allow people to set CYGSEM_REDBOOT_DEFAULT_NO_BOOTP without
providing a default IP address.  That would allow a configuration with
bootp built, no default IP address, but bootp _use_ disabled by
default.  Bootp usage could still be enabled in flash config,
initiated manually via "ip -v", or a default IP could be configured in
flash.

AFAICT, that particular combination wasn't something that could be
done with the previous code.  Without a default IP address bootp was
always enabled by default (but could be overridden via flash config).

I don't have a strong opinion one way or the other, but if I were to
pick, I'd move CYGSEM_REDBOOT_DEFAULT_NO_BOOTP into
CYGPKG_REDBOOT_NETWORKING_BOOTP and make it independent of whether
CYGDAT_REDBOOT_DEFAULT_IP_ADDR is set or not.  Who are we to say that
no default IP address and bootp built but disabled by default isn't
useful to somebody?

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

[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

--- Comment #5 from Jonathan Larmour <jifl@ecoscentric.com> 2011-03-24 19:36:05 GMT ---
(In reply to comment #4)
> 
> I presume that the desired solution would be when waiting for an
> OFFER, if you receive a BOOTP REPLY packet that doesn't have both a
> DHCP cookie and a DHCP type field, you "fall back" to BOOTP mode and
> terminate the transaction with whatever info you can glean from the
> BOOTP REPLY packet?

IMHO, yes.

(And if someone has a BOOTP and DHCP server on the same subnet, they deserve
what they get).

Jifl

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

[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

--- Comment #4 from Grant Edwards <grant.b.edwards@gmail.com> 2011-03-24 19:33:43 GMT ---
(In reply to comment #3)

>> - 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?
> 
> Off the top of my head, I don't think it does.

It does not.

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

I think that because of the promiscuous manner in which reply packets
are copied, the previous code would time-out in DHCP mode and then use
data from whatever BOOTP REPLY packet was last received (regardless of
XID or destination address).

> I don't know.  I've never tried it with a server that doesn't do
> DHCP.  Will a bootp-only server send a BOOTP RESPONSE in reply to a
> DHCP DISCOVER?

Apparently they will.  And the state machine ignores it.

I presume that the desired solution would be when waiting for an
OFFER, if you receive a BOOTP REPLY packet that doesn't have both a
DHCP cookie and a DHCP type field, you "fall back" to BOOTP mode and
terminate the transaction with whatever info you can glean from the
BOOTP REPLY packet?

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

[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

--- Comment #3 from Grant Edwards <grant.b.edwards@gmail.com> 2011-03-24 18:12:39 GMT ---

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

Done.

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

Isn't that what CYGSEM_REDBOOT_DEFAULT_NO_BOOTP does?  I didn't mess
with the logic that controls what gets built and what doesn't, I was
just attempting to fix the DHCP support.  I assumed that setting
CYGSEM_REDBOOT_DEFAULT_NO_BOOTP would build without BOOTP support.

...

But, I see that's not what it does.  It just changes the default value
for a run-time flag that determines whether the call is made to
__bootp_find_local_ip().

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

Done.

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

Off the top of my head, I don't think 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.

I don't know.  I've never tried it with a server that doesn't do DHCP.
Will a bootp-only server send a BOOTP RESPONSE in reply to a DHCP
DISCOVER?

I'll look into it.

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

OK, I'll get rid of the call to the CRC routine.

>   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()

I tried calling HAL_CLOCK_READ(), but the value was always 1.  I'll
try 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.

That sounds like a good idea

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

I'll skip the CRC call.

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

OK.

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

[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 on the CC list 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 on the CC list 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 on the CC list 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 on the CC list for the bug.