| Message ID | 20240209061035.3757-3-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,1/3] net: ipv6/addrconf: ensure that regen_advance is at least 2 seconds | expand |
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
On 2/13/24 3:13 AM, Paolo Abeni wrote: >> 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? > I went through the set this past weekend along with the earlier thread with the problem Dan mentioned. I think the logic is ok. Dan: did you get a chance to test this set? Reviewed-by: David Ahern <dsahern@kernel.org>
> I went through the set this past weekend along with the earlier thread > with the problem Dan mentioned. I think the logic is ok. Dan: did you > get a chance to test this set? I just applied it on top of v6.7.4 and I do not see any issues. It handled the router advertisements and prefix deprecation just fine, AFAICT (it generated just one temporary address as I would have expected, instead of thousands of temporary addresses with very short lifetimes). Cheers, -- Dan
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; 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;
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(-)