Re: [PATCH v2] dhcp: fix overflow causing retries to stop

Hi Denis,

On 1/15/24 6:30 AM, Denis Kenzior wrote:
> Hi James,
>
> On 1/14/24 10:12, James Prestwood wrote:
>> Hi Denis,
>>
>> On 1/12/24 9:11 AM, Denis Kenzior wrote:
>>> Hi James,
>>>
>>> On 1/11/24 07:33, James Prestwood wrote:
>>>> If DHCP is in a SELECTING/REQUESTING state and the number of attempts
>>>> reached a value where 2 << attempts overflowed an unsigned int the
>>>> next timeout would become zero, causing DHCP to never retry without
>>>> any event notification.
>>>>
>>>> Roughly 2 minutes has passed when we reach >5 attempts so it makes
>>>> little sense to retry indefinitely, at least without notifying the
>>>> upper layers (which could decided to retry themselves).
>>>
>>> Okay, but '5' is just a magic number in the code with no context. 
>>> Lets avoid that.
>>>
>>> Can we make this limit configurable?  Say between 3..30?  For iwd, 
>>> even 2 minutes might be too long.
>>
>> I'm thinking it may be better to leave the default unlimited retries 
>> but add a configurable time limit versus retry limit. Setting based 
>> on retries isn't intuitive a) because 99% of people have no idea what 
>> the backoff algorithm is, and b) time limits are fuzzed so its not an 
>> exact amount of time.
>
> Hopefully we know enough to tune the retries :)
I was more saying in the context of someone using the ELL API, not 
specifically for IWD.
>
>>
>> I say this because if we want to include netconfig into IWD's 
>> Connect() DBus method return we kinda need a way to define a time 
>> limit as opposed to guessing how long 2-3 retries will actually take. 
>> We, potentially, could iterate through several BSS's and fail 
>> extending the time to connect, then start DHCP. We would need a way 
>> to limit netconfig to within the DBus method timeout (25 seconds 
>
> DBus method timeouts don't exist on the protocol level.  ell doesn't 
> even let you specify a method timeout for that reason. Some libraries 
> use a locally generated timeout by default (such as libdbus-1), but 
> such libraries usually provide a way to specify an arbitrary timeout, 
> or even disable it.
>
> The problem with such timeouts is that when a library-local timeout is 
> hit, creating a fake dbus method reply, there's no way to tell the 
> callee.  In the case of iwd, it will just continue its Connect() 
> operation since nothing happened as far as it is concerned.  Any dbus 
> method timeouts are up to the application to deal with correctly...
>
> Also, having dhcp use its own timeout incurs a cost of an extra 
> file-descriptor.  Not worth it.
>
>> IIRC), including how long all the connect attempts took prior. Maybe 
>> I'm overthinking it but theoretically we could hit the method timeout 
>> limit so we may want to add handling for it, which would be easier if 
>> we did the above with a timeout versus retries.
>
> All we can do is document that Connect might be a long-running 
> operation (should be obvious) for the reasons you outline.

So from an ELL perspective I'm fine adding a retries setter (though 
timeout makes more intuitive sense IMO) but I do want to explain some 
issues I see with utilizing this in IWD, and doing netconfig prior to 
the DBus method return.

Adding netconfig into the mix would greatly increase the likelihood of a 
DBus method timeout in "semi-normal" cases depending on the retry limit 
chosen. I understand the dilemma you explained above but I do think IWD 
should try its best to stay within the default DBus method timeout of 25 
seconds. Trying to do this while also doing netconfig seems difficult 
unless we really cut down the allowed retries, which has is drawbacks as 
poor RF could easily drop a few frames and we could quickly get up to 25 
seconds with the nature of the DHCP timeouts and if the wifi association 
took a while (multiple BSS retries).

I'm also not sure if a DBus method timeout is going to break or cause 
weird behavior with Connman/NetworkManager etc. And even if its handled, 
what exactly should they do in this case? Will DHCP complete? should 
they re-issue connect which could return -EBUSY? What happens when IWD 
does reply and a timeout has already occurred? It really opens up a lot 
of possibilities here.

This would have to be an IWD 3.0 change, but adding netconfig as a DBus 
state may be the better decision here. Then netconfig could take much 
longer (maybe user configurable), and we avoid/lessen the risk of method 
timeouts. To me this is similar to how Scan() works where the method 
returns quickly, but the caller then needs to wait for the scanning 
property before getting results.

Thanks,

James

> Regards,
> -Denis