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

Hi Denis,

On 1/16/24 8:13 AM, Denis Kenzior wrote:
> Hi James,
>
>>
>> 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.
>
> I disagree here.  For the dhcp_client, max retries is a much clearer 
> 'lever' to adjust than the overall timeout.  As you said earlier, with 
> fuzzing the number of retries in a given period can vary.  If the 
> upper layers want to enforce a given timeout, they can do so anyway.
>
>>
>> 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 
>
> You'll have to explain why you make this assertion?  If DBus bindings 
> being used don't allow setting an infinite timeout, get better 
> bindings.  I don't see any need to force iwd into fitting into this 
> arbitrary 25 second limit.  And anyway, Connect() already can invoke 
> Agent methods, which can take arbitrarily long anyway.  So netconfig 
> is the least of your problems.
Just based on the bindings we use for test-runner and the bindings I've 
been using recently (dbus_next), both have a 25 second method timeout. 
It appears that "dbus" (what we use in test-runner) might let you 
specify a timeout, not sure about dbus_next. I just wanted to 
communicate that a timeout is common across these bindings, for better 
or worse. If we don't want to conform to that, that's fine.
>
>> 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).
>
> Do you have any data / examples to back up this assertion?
We have seen DHCP take 10-15 seconds due to dropped frames. All I'm 
saying is that just a few dropped frames can drastically increase the 
time. If we don't care about the arbitrary 25 second timeout, then this 
is fine, I just was pointing that out.
>
>>
>> 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 
>
> Not if they're written properly.
>
>> 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.
>
> That is why I said that timeout handling has to be handled by the 
> application. Most I've seen in real life do this poorly since there is 
> usually a fundamental misunderstanding of how D-Bus works under the hood.
Ok, I'll definitely add handling for this, hopefully everything else 
using Connect() already handles it.
>
> In practice, locally generated timeouts like the one used by default 
> in libdbus-1 are just a bad idea.  I have no clue why they were added 
> originally. If you have a misbehaving service on your bus which leaks 
> messages, you will end up clogging D-Bus at some point and only a 
> service restart will fix that.  Your service always has to reply to 
> the message.  If you trust your running-as-root services to do the 
> right thing, then adding arbitrary timeouts just gets in the way.
>
> The fact that the timeout happened is unknown to the dbus peer. It 
> will continue as if nothing happened, and if well-behaved will 
> generate a reply message eventually.  This has to happen for proper 
> functioning of D-Bus.  The dbus-daemon will track that reply and 
> forward it to the caller.  The caller's dbus implementation will just 
> drop it on the floor since the reply-to serial number is no longer on 
> the pending list.
>
> So your iwd client has only two choices:
> 1. Call Connect() with no timeout and trust iwd to do the right thing.
> 2. Handle timeouts by trying to abort the connection somehow. Perhaps 
> via Station.Disconnect().  But that has its own side-effects.
> 3. If a timeout happens, assume iwd service is dead and restart it.
>
> Hint: Option 1 or 3 is preferred
>
> Regards,
> -Denis