| Message ID | 20230821011116.21931-1-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ipv6/addrconf: clamp preferred_lft to the minimum instead of erroring | expand |
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@gmail.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
Hi Paolo, thanks for the review. On Tue, Aug 22, 2023 at 3:54 AM Paolo Abeni <pabeni@redhat.com> wrote: > It looks like you are fixing 2 separate bugs, so 2 separate patches > would be better. The two problems are closely related, and in the same function. But I will split the patch into two patches to your preference. > You should explicitly state the target tree (in this case 'net') into > the patch subj. Will fix in v2, thanks. > You should add a suitable fixes tag to each patch. That would be "Fixes: 76506a986dc31394fd1f2741db037d29c7e57843" and "Fixes: eac55bf97094f6b64116426864cf4666ef7587bc", correct? > On Sun, 2023-08-20 at 19:11 -0600, Alex Henrie wrote: > > @@ -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. I did mention the underflow problem in the commit message. When I split the patch into two patches, it will be even more prominent. What more would you like the commit message to say? > Also possibly using 'long' as the target type (same as > 'max_desync_factor') would be more clear. OK, will change in v2. > > @@ -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. The general idea is still valid: The preferred lifetime must be greater than regen_advance. I will rephrase the comment to be more clear in v2. > But I'm unsure we can raise the preferred lifetime so easily. e.g. what > if preferred_lft becomes greater then valid_lft? Excellent point. We really should clamp preferred_lft to valid_lft as well. I can make that change in v2. By the way, if valid_lft is less than regen_advance, temporary addresses still won't work. However, that is much more understandable because valid_lft has to be at least the length of the longest needed connection, so in practice it's always going to be much longer than 5 seconds. > I think a fairly safer alternative option would be documenting the > current behavior in ip-sysctl.rst I feel strongly that the current behavior, which can appear to be working fine for a few minutes before breaking, is very undesirable. I could, nonetheless, add some explanation to ip-sysctl.rst about what happens if preferred_lft or valid_lft is too small. Thanks for caring about doing IPv6 right, -Alex
On 8/22/23 9:41 PM, Alex Henrie wrote: > Hi Paolo, thanks for the review. > > On Tue, Aug 22, 2023 at 3:54 AM Paolo Abeni <pabeni@redhat.com> wrote: > >> It looks like you are fixing 2 separate bugs, so 2 separate patches >> would be better. > > The two problems are closely related, and in the same function. But I > will split the patch into two patches to your preference. > >> You should explicitly state the target tree (in this case 'net') into >> the patch subj. > > Will fix in v2, thanks. > >> You should add a suitable fixes tag to each patch. > > That would be "Fixes: 76506a986dc31394fd1f2741db037d29c7e57843" and > "Fixes: eac55bf97094f6b64116426864cf4666ef7587bc", correct? See `git log` and search for Fixes to see examples. e.g., Fixes: eac55bf97094 ("IPv6: do not create temporary adresses with too short preferred lifetime")
On Tue, Aug 22, 2023 at 09:41:37PM -0600, Alex Henrie wrote: > "Fixes: eac55bf97094f6b64116426864cf4666ef7587bc", correct? > > > On Sun, 2023-08-20 at 19:11 -0600, Alex Henrie wrote: > > > > @@ -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. > > I did mention the underflow problem in the commit message. When I > split the patch into two patches, it will be even more prominent. What > more would you like the commit message to say? > > > Also possibly using 'long' as the target type (same as > > 'max_desync_factor') would be more clear. > > OK, will change in v2. This part looks good to me. Sorry for introducing the bug and thanks for finding it!
On Tue, 2023-08-22 at 21:41 -0600, Alex Henrie wrote: > On Tue, Aug 22, 2023 at 3:54 AM Paolo Abeni <pabeni@redhat.com> wrote: > > On Sun, 2023-08-20 at 19:11 -0600, Alex Henrie wrote: > > > > @@ -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. > > I did mention the underflow problem in the commit message. When I > split the patch into two patches, it will be even more prominent. What > more would you like the commit message to say? I think explicitly mentioning that the existing code incorrectly casted a negative value to an unsigned one should suffice. > > > Also possibly using 'long' as the target type (same as > > 'max_desync_factor') would be more clear. > > OK, will change in v2. > > > > @@ -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. > > The general idea is still valid: The preferred lifetime must be > greater than regen_advance. I will rephrase the comment to be more > clear in v2. > > > But I'm unsure we can raise the preferred lifetime so easily. e.g. what > > if preferred_lft becomes greater then valid_lft? > > Excellent point. We really should clamp preferred_lft to valid_lft as > well. I can make that change in v2. > > By the way, if valid_lft is less than regen_advance, temporary > addresses still won't work. However, that is much more understandable > because valid_lft has to be at least the length of the longest needed > connection, so in practice it's always going to be much longer than 5 > seconds. > > > I think a fairly safer alternative option would be documenting the > > current behavior in ip-sysctl.rst > > I feel strongly that the current behavior, which can appear to be > working fine for a few minutes before breaking, is very undesirable. > I > could, nonetheless, add some explanation to ip-sysctl.rst about what > happens if preferred_lft or valid_lft is too small. I think that we could accept the general idea that setting some "extreme"/edge values on system settings will lead to unexpected results/limited functionality. IDK how much relevant is the 'preferred_lft < 5' use-case. I fear that changing "under-the-hood" the preferred lifetime value in use could have unexpected side effects for other scenarios. e.g. we can hit the 'increase preferred lifetime' condition even when: cfg.preferred_lft == <some largish, more common, value> age == ~cfg.preferred_lft @David A.: I would love to hear your opinion here. Thank, Paolo
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); @@ -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; cfg.ifa_flags = IFA_F_TEMPORARY; /* set in addrconf_prefix_rcv() */
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@gmail.com> --- net/ipv6/addrconf.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)