netdev - Re: [PATCH] ipv6/addrconf: clamp preferred_lft to the minimum instead of erroring

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e0e8e74a65ae24580d3ab742a8e76ca82bf26ff8.camel@redhat.com>
Date: Tue, 22 Aug 2023 11:54:03 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Alex Henrie <alexhenrie24@...il.com>, netdev@...r.kernel.org, 
	jbohac@...e.cz, benoit.boissinot@...-lyon.org, davem@...emloft.net, 
	hideaki.yoshifuji@...aclelinux.com, dsahern@...nel.org
Subject: Re: [PATCH] ipv6/addrconf: clamp preferred_lft to the minimum
 instead of erroring

Hi,

On Sun, 2023-08-20 at 19:11 -0600, Alex Henrie wrote:
> I tried setting /proc/sys/net/ipv6/conf/*/temp_prefered_lft to 1 so that
> the address would roll over as frequently as possible, then spent hours
> trying to understand why the preferred lifetime jumped to 4 billion
> seconds. On my machine and network the shortest lifetime that avoids
> underflow is 3 seconds.
> 
> After fixing the underflow, I ran into a second problem: The preferred
> lifetime was less than the minimum required lifetime, so
> ipv6_create_tempaddr would error out without creating any new address.
> This error happened immediately with the preferred lifetime set to
> 1 second, after a few minutes with the preferred lifetime set to
> 4 seconds, and not at all with the preferred lifetime set to 5 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 3 or 4 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 also disabling duplicate address detection (setting
> /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@...il.com>

It looks like you are fixing 2 separate bugs, so 2 separate patches
would be better.

You should explicitly state the target tree (in this case 'net') into
the patch subj.

You should add a suitable fixes tag to each patch.

> ---
>  net/ipv6/addrconf.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 94cec2075eee..4008d4a5e58d 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -1368,7 +1368,7 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, bool block)
>  	 * idev->desync_factor if it's larger
>  	 */
>  	cnf_temp_preferred_lft = READ_ONCE(idev->cnf.temp_prefered_lft);
> -	max_desync_factor = min_t(__u32,
> +	max_desync_factor = min_t(__s64,
>  				  idev->cnf.max_desync_factor,
>  				  cnf_temp_preferred_lft - regen_advance);

It would be better if you describe in the commit message your above
fix.

Also possibly using 'long' as the target type (same as
'max_desync_factor') would be more clear.
>  
> @@ -1402,12 +1402,8 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, bool block)
>  	 * 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;
> -	}
> +	if (cfg.preferred_lft <= regen_advance + age)
> +		cfg.preferred_lft = regen_advance + age + 1;

This change obsoletes the comment pairing the code. At very least you
should update that and the sysctl knob description in
Documentation/networking/ip-sysctl.rst.

But I'm unsure we can raise the preferred lifetime so easily. e.g. what
if preferred_lft becomes greater then valid_lft?

I think a fairly safer alternative option would be documenting the
current behavior in ip-sysctl.rst

Cheers,

Paolo