udhcpc: limit renew/rebind timeout

[Denys Vlasenko]

On Wednesday 14 September 2011 13:34, Vladislav Grishenko wrote:
> In some cases it's good to have control over manual renew/rebind timeout
> before deconfig state.
> Scenario:
> 1. udhcpc gets lease for 86400 secs and sleeps for 43200 before renew
> attempt
> 2. PC gets physically disconnected and connected to another network
> 3. some phy control software sends SIGUSR1 to renew the lease, SIGUSR2 isn't
> used because newly connected network could be the same as before
> 4. udhcpc sends unicast renew requests up to 60 timeout, which are ignored
> by new network dhcp servers

I guess this is a wrong behavior. SIGUSR was most likely meant to
force udhcpc to renew to completely renegotiate the lease *quickly*,
that it, to broadcast (at once, or after a few inucast tries),
instead of unicasting until current lease expirea.

> 5. udhcpc sends broadcast rebind requests, which could be naked or ignored
> too, if there's no authority dhcp servers configured
> 6. udhcpc deconfigs and starting from discover state, gets new lease for the
> new network
> So, pt.4+5 it could take up to 86400 secs without correct lease, which is
> too long and not acceptable in my case.
> Second SIGUSR1 will immediately run into deconfig/discover state, which is
> not preferable in case of the same subnet replugged.
> 
> With the patch, it's possible to limit pt.4 and pt.5 stage timeouts, for
> example:
> By specifying -N61 option udhcpc will make one  unicast renew attempt
> (timeout greater than 60) and will get 61/2 timeout for broadcast rebind, if
> original timeout before signal is greater than 61 sec.
> By specifying -N60 and lower, no unicast renew attempt will be made, only
> broadcast rebind.
> Default behavior - is to continue use of unchanged timeouts, like before
> patch.

When is "before patch" behavior is useful? I don't see it.

I propose to use your fix unconditionally.

This way, we don't need a new option. We can use hardcoded value (~30 sec?)
or abuse one of existing tweakable values.
I think -A N can be used, it's 20 by default. The fact that we don't try
unicast even once is likely not a problem in practice.

How about this patch?

(I applied it to git)

-- 
vda

diff -ad -urpN busybox.0/networking/udhcp/dhcpc.c busybox.1/networking/udhcp/dhcpc.c
--- busybox.0/networking/udhcp/dhcpc.c	2011-10-16 05:16:40.000000000 +0200
+++ busybox.1/networking/udhcp/dhcpc.c	2011-10-18 01:29:01.000000000 +0200
@@ -1362,8 +1362,22 @@ int udhcpc_main(int argc UNUSED_PARAM, c
 		case SIGUSR1:
 			client_config.first_secs = 0; /* make secs field count from 0 */
 			perform_renew();
-			if (state == RENEW_REQUESTED)
+			if (state == RENEW_REQUESTED) {
+				already_waited_sec = 0;
+				/* We might be either on the same network
+				 * (in which case renew might work),
+				 * or maybe we are on a completely different one
+				 * (in which case renew won't ever succeed!).
+				 * For the second case, must make sure timeout
+				 * is not too big, or else we can send renew requests
+				 * for hours.
+				 * (Ab)use -A TIMEOUT value (usually 20 sec)
+				 * as a cap on the timeout.
+				 */
+				if (timeout > tryagain_timeout)
+					timeout = tryagain_timeout;
 				goto case_RENEW_REQUESTED;
+			}
 			/* Start things over */
 			packet_num = 0;
 			/* Kill any timeouts, user wants this to hurry along */