Message-ID: <CAMMLpeT_19VjoUZ=zAogPcXU0-auUOn43YMnN=XCfvHQL60QsA@mail.gmail.com>
Date: Tue, 30 Jan 2024 22:50:00 -0700
From: Alex Henrie <alexhenrie24@...il.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, dan@...m.net, bagasdotme@...il.com, 
	davem@...emloft.net, dsahern@...nel.org, edumazet@...gle.com, kuba@...nel.org, 
	jikos@...nel.org
Subject: Re: [PATCH net-next] net: ipv6/addrconf: make regen_advance
 independent of retrans time

On Tue, Jan 30, 2024 at 3:46 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On Wed, 2024-01-24 at 20:57 -0700, Alex Henrie wrote:
> > In RFC 4941, REGEN_ADVANCE is a constant value of 5 seconds, and the RFC
> > does not permit the creation of temporary addresses with lifetimes
> > shorter than that:
> >
> > > When processing a Router Advertisement with a Prefix
> > > Information option carrying a global scope prefix for the purposes of
> > > address autoconfiguration (i.e., the A bit is set), the node MUST
> > > perform the following steps:
> >
> > > 5.  A temporary address is created only if this calculated Preferred
> > >     Lifetime is greater than REGEN_ADVANCE time units.
> >
> > Moreover, using a non-constant regen_advance has undesirable side
> > effects. If regen_advance swelled above temp_prefered_lft,
> > ipv6_create_tempaddr would error out without creating any new address.
>
> RFC 4941 has been obsoleted by RFC 8981, which in turns makes
> REGEN_ADVANCE non constant:
>
> 3.8. Defined Protocol Parameters and Configuration Variables
>
> REGEN_ADVANCE
>    2 + (TEMP_IDGEN_RETRIES * DupAddrDetectTransmits * RetransTimer /
>    1000)

Ah, so that's where Linux's regen_advance formula came from! Thank you
very much for pointing me to the updated RFC.

However, according to the formula defined in RFC 8981, even though
REGEN_ADVANCE is not a constant, it still must not be less than 2
seconds. So unfortunately, it still seems that Linux's current
implementation is technically in violation of the spec because it
doesn't add the 2.

> > On my machine and network, 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.
> >
> > Some users want to change their IPv6 address as frequently as possible
> > regardless of the RFC's arbitrary minimum lifetime. For the benefit of
> > those users, add a regen_advance sysctl parameter that can be set to
> > below or above 5 seconds.
>
> I guess we can't accommodate every user desire while speaking the same
> protocol.

Linux already allows the user to reduce regen_advance to 0 by
disabling duplicate address detection (setting
/proc/sys/net/ipv6/conf/*/dad_transmits to 0). Disabling DAD might be
a protocol violation, and setting regen_advance to 0 probably is too,
but I didn't want to make it impossible because I can see people
having good reasons to do both of those things.

The bug I'm trying to fix is a different scenario: It happens when the
user wants to rotate their IPv6 address as frequently as the protocol
allows, but not so frequently as to break things like duplicate
address detection.

> Perhaps emitting a kernel message when user settings do not allow the
> address regeneration could be a better option?

The fundamental problem with emitting a warning is that when the
network parameters are set, the kernel might not know that there's
going to be a problem. The network could work fine for hours or days
and then have a period of merely a few seconds when regen_advance
swells above prefered_lft, at which point the kernel just gives up on
temporary addresses. The kernel could print a warning then, but even
if the user is knowledgeable enough to look at dmesg and understand
the problem, unless they disable DAD to make regen_advance zero or set
prefered_lft to an excessively large value, there's no way for them to
pick a value for prefered_lft that is guaranteed to always be greater
than regen_advance.

The fact that regen_advance does not have to be a constant and there
is no hard minimum means that my original patch that was in 6.7-rc1
[1] and reverted in 6.7-rc8 [2] was essentially correct, it just needs
to be fixed to respect the maximums that are checked earlier in the
function.[3] But if we want to add a 2-second minimum as well, I think
I need to send three new patches:

1. Move the calculation of regen_advance to a helper function and add
2 to the calculated value

2. Add a regen_advance sysctl that corresponds to the number 2 in the
formula, to allow changing it back to 0 or to any other value

3. Clamp preferred_lft to the minimum required as originally intended

Thanks very much for the feedback and please let me know if you have
any more thoughts before I get cracking.

-Alex

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=629df6701c8a9172f4274af6de9dfa99e2c7ac56
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8cdafdd94654ba418648d039c48e7a90508c1982
[3] https://lore.kernel.org/netdev/20231222234237.44823-2-alexhenrie24@gmail.com/

Powered by blists - more mailing lists