Re: [PATCH net-next 3/3] net: ipv6/addrconf: clamp preferred_lft to the minimum required

[Paolo Abeni <pabeni@redhat.com>]

On Thu, 2024-02-08 at 23:10 -0700, Alex Henrie wrote:
> If the preferred lifetime was less than the minimum required lifetime,
> ipv6_create_tempaddr would error out without creating any new address.
> On my machine and network, this error happened immediately with the
> preferred lifetime set to 5 seconds or less, after a few minutes with
> the preferred lifetime set to 6 seconds, and not at all with the
> preferred lifetime set to 7 seconds. During my investigation, I found a
> Stack Exchange post from another person who seems to have had the same
> problem: They stopped getting new addresses if they lowered the
> preferred lifetime below 3 seconds, and they didn't really know why.
> 
> The preferred lifetime is a preference, not a hard requirement. The
> kernel does not strictly forbid new connections on a deprecated address,
> nor does it guarantee that the address will be disposed of the instant
> its total valid lifetime expires. So rather than disable IPv6 privacy
> extensions altogether if the minimum required lifetime swells above the
> preferred lifetime, it is more in keeping with the user's intent to
> increase the temporary address's lifetime to the minimum necessary for
> the current network conditions.
> 
> With these fixes, setting the preferred lifetime to 5 or 6 seconds "just
> works" because the extra fraction of a second is practically
> unnoticeable. It's even possible to reduce the time before deprecation
> to 1 or 2 seconds by setting /proc/sys/net/ipv6/conf/*/regen_min_advance
> and /proc/sys/net/ipv6/conf/*/dad_transmits to 0. I realize that that is
> a pretty niche use case, but I know at least one person who would gladly
> sacrifice performance and convenience to be sure that they are getting
> the maximum possible level of privacy.
> 
> Link: https://serverfault.com/a/1031168/310447
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>  net/ipv6/addrconf.c | 43 ++++++++++++++++++++++++++++++++++---------
>  1 file changed, 34 insertions(+), 9 deletions(-)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 0b78ffc101ef..8d3023e54822 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -1347,6 +1347,7 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, bool block)
>  	unsigned long regen_advance;
>  	unsigned long now = jiffies;
>  	s32 cnf_temp_preferred_lft;
> +	u32 if_public_preferred_lft;

[only if a repost is needed for some other reason] please respect the
reverse x-mas tree above.

>  	struct inet6_ifaddr *ift;
>  	struct ifa6_config cfg;
>  	long max_desync_factor;
> @@ -1401,11 +1402,13 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, bool block)
>  		}
>  	}
>  
> +	if_public_preferred_lft = ifp->prefered_lft;
> +
>  	memset(&cfg, 0, sizeof(cfg));
>  	cfg.valid_lft = min_t(__u32, ifp->valid_lft,
>  			      idev->cnf.temp_valid_lft + age);
>  	cfg.preferred_lft = cnf_temp_preferred_lft + age - idev->desync_factor;
> -	cfg.preferred_lft = min_t(__u32, ifp->prefered_lft, cfg.preferred_lft);
> +	cfg.preferred_lft = min_t(__u32, if_public_preferred_lft, cfg.preferred_lft);
>  	cfg.preferred_lft = min_t(__u32, cfg.valid_lft, cfg.preferred_lft);
>  
>  	cfg.plen = ifp->prefix_len;
> @@ -1414,19 +1417,41 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, bool block)
>  
>  	write_unlock_bh(&idev->lock);
>  
> -	/* A temporary address is created only if this calculated Preferred
> -	 * Lifetime is greater than REGEN_ADVANCE time units.  In particular,
> -	 * an implementation must not create a temporary address with a zero
> -	 * Preferred Lifetime.
> +	/* From RFC 4941:
> +	 *
> +	 *     A temporary address is created only if this calculated Preferred
> +	 *     Lifetime is greater than REGEN_ADVANCE time units.  In
> +	 *     particular, an implementation must not create a temporary address
> +	 *     with a zero Preferred Lifetime.
> +	 *
> +	 *     ...
> +	 *
> +	 *     When creating a temporary address, the lifetime values MUST be
> +	 *     derived from the corresponding prefix as follows:
> +	 *
> +	 *     ...
> +	 *
> +	 *     *  Its Preferred Lifetime is the lower of the Preferred Lifetime
> +	 *        of the public address or TEMP_PREFERRED_LIFETIME -
> +	 *        DESYNC_FACTOR.
> +	 *
> +	 * To comply with the RFC's requirements, clamp the preferred lifetime
> +	 * to a minimum of regen_advance, unless that would exceed valid_lft or
> +	 * ifp->prefered_lft.
> +	 *
>  	 * Use age calculation as in addrconf_verify to avoid unnecessary
>  	 * temporary addresses being generated.
>  	 */
>  	age = (now - tmp_tstamp + ADDRCONF_TIMER_FUZZ_MINUS) / HZ;
>  	if (cfg.preferred_lft <= regen_advance + age) {
> -		in6_ifa_put(ifp);
> -		in6_dev_put(idev);
> -		ret = -1;
> -		goto out;
> +		cfg.preferred_lft = regen_advance + age + 1;
> +		if (cfg.preferred_lft > cfg.valid_lft ||
> +		    cfg.preferred_lft > if_public_preferred_lft) {
> +			in6_ifa_put(ifp);
> +			in6_dev_put(idev);
> +			ret = -1;
> +			goto out;
> +		}
>  	}
>  
>  	cfg.ifa_flags = IFA_F_TEMPORARY;

The above sounds reasonable to me, but I would appreciate a couple of
additional eyeballs on it. @David, could you please have a look?

Thanks,

Paolo