[klibc] Timeout values (-t) not always guaranteed

[Khaled Elmously]

On 2021-12-26 23:09:06 , Ben Hutchings wrote:
> On Thu, 2021-12-02 at 14:49 -0500, Khaled Elmously wrote:
> > Hello,
> > 
> > I have encountered situations where ipconfig takes longer than the
> > user-specified timeout.
> > 
> > The following is an easy way to reproduce the issue:
> > 
> > # ip link add eth3 type dummy
> > # date; /usr/lib/klibc/bin/ipconfig -t 2 eth3; date
> > Thu Dec  2 14:26:15 EST 2021
> > IP-Config: eth3 hardware address aa:8f:6f:bf:35:96 mtu 1500 DHCP RARP
> > IP-Config: no response after 2 secs - giving up
> > Thu Dec  2 14:26:25 EST 2021
> > 
> > 
> > 
> > Notice that 'ipconfig' thinks it waited 2 seconds (which is the
> > timeout value I specified), but in reality it waited 10 seconds as
> > the
> > 'date' values show.
> > 
> > This seems to happen because of the "try again in 10 seconds" that
> > happens when ipconfig encounters an error on send.
> 
> Yes, I can see that.
> 
> > I believe that a fix such as the following is needed to avoid this
> > issue:
> > 
> > diff --git a/usr/kinit/ipconfig/main.c b/usr/kinit/ipconfig/main.c
> > index 5472d0f0..0969e9cb 100644
> > --- a/usr/kinit/ipconfig/main.c
> > +++ b/usr/kinit/ipconfig/main.c
> > @@ -477,6 +477,14 @@ static int loop(void)
> >                 if (pending == 0 || (bringup_first && done))
> >                         break;
> > 
> > +               /* Compensate for already-lost time */
> > +               /* Make sure to never exceed user-specified timeouts */
> > +               gettimeofday(&now, NULL);
> > +               if (now.tv_sec + timeout > start + loop_timeout) {
> > +                       timeout = loop_timeout - (now.tv_sec - start);
> > +                       printf("Lowered timeout to match user request = (%d s) \n", timeout);
> > +               }
> > +
> >                 timeout_ms = timeout * 1000;
> > 
> >                 for (x = 0; x < 2; x++) {
> > 
> > 
> > 
> > 
> > 
> > Does that make sense? Any problems with this?


Thanks a lot for the feedback!

> 
> It does make some sense.  (I think it would make more sense to get rid
> of the inner loop as well; that would simplify this.)

Are you referring to this loop? 

                 for (x = 0; x < 2; x++) {

If so, I think that can be removed only if the device probe loop is moved outside of this while(1) loop as well.
Could you please confirm if that's what you meant?


> 
> However, I worry a bit that users might depend on the current
> behaviour.  In particular, it looks like a DHCP configuration will
> currently work as long as the DHCPOFFER is received within the loop
> timeout (and no packet timeout occurs).  If we apply the loop timeout
> before packet timeouts, the DHCPACK must also be received within the
> loop timeout.

Yes, that is my understanding as well. The user-specified timeout is simply not honoured and the longer loop timeout is in effect so any DHCP offers in the new timeout are accepted.
But breaking that behaviour is kind of the point though, right? If the DHCP offer comes after the user-specified delay has expired, then ipconfig is in fact misbehaving by utilizing it.


> 
> Another issue, if you really want to use short timeouts like 2 seconds,
> is that we can currently time-out nearly 1 second too early due to
> rounding down "start" to the last second.  We would need to start
> including the fractional seconds in timeout calculations.

Understood, thanks.


> 
> Ben.
> 
> -- 
> Ben Hutchings
> It is easier to write an incorrect program
> than to understand a correct one.