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

Hi James,

On 1/9/24 06:18, James Prestwood wrote:
> Hi Denis,
> 
> On 1/8/24 8:27 PM, Denis Kenzior wrote:
>> Hi James,
>>
>> On 1/8/24 22:06, 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.
>>>
>>> Since 5 attempts results in a value of 64 we can instead just limit
>>> the attempts to 5, and set next_timeout to 64 after that as the spec
>>> requires.
>>
>> Overflowing next_timeout implies that discovery is taking ~25 minutes?  I 
>> think I'd rather we gave up and sent a NO_LEASE event after retries hits a 
>> certain max number?
> 
> Yep, this is whats happening, the DHCP server is down. Obviously we could change 
> the logic, but NO_LEASE appears to just restart the DHCP client, i.e. no real 

I'm not sure why netconfig does that actually.  From the comment it seems like 
it expects NO_LEASE event when the old address could not be obtained.  But, if I 
recall correctly, ell's dhcp client implementation only includes the old address 
when using RENEW or REBIND, and never on a fresh dhcp_client_start()-up due to 
privacy policy.

In the end, it is up to netconfig to implement a policy it wants.  Re-starting 
discovery is a valid policy, but I think in iwd's case, we should have netconfig 
treat 'NO_LEASE' as a DHCP server going down / server not present.  Having a max 
number of retries can facilitate that.  For example, if we're connecting and 
reach max_attempts of 6 or so, then give up and have iwd fail the connection.

> difference between trying discovery forever. Should we add another event, 
> L_DHCP_CLIENT_EVENT_TIMEOUT?

l_dhcp_client sends NO_LEASE when it gets a NAK and there's no point in 
continuing without further intervention (or configuration change).  Server not 
responding seems to fall into the same category, but we can also add an event 
explicitly for this.  Maybe MAX_DISCOVER_ATTEMPTS_REACHED?

Regards,
-Denis