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

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

[Alex Henrie]

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(-)

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() */
-- 
2.41.0

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

[Paolo Abeni]

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

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

[Alex Henrie]

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

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

[David Ahern]

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")

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

[Jiri Bohac]

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!

-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, Prague, Czechia

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

[Paolo Abeni]

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

[PATCH v2 0/5] net: ipv6/addrconf: ensure that temporary addresses' preferred lifetimes are in the valid range

[Alex Henrie]

Changes from v1:
- Split into multiple patches
- Add "Fixes" lines to the commit messages
- Use long instead of __s64
- Ensure that the valid lifetime is not shorter than the required lifetime
- Ensure that the preferred lifetime is not longer than the valid lifetime
- Update the documentation

Thanks to David, Jiri, and Paolo for your feedback.

Alex Henrie (5):
  net: ipv6/addrconf: avoid integer underflow in ipv6_create_tempaddr
  net: ipv6/addrconf: clamp preferred_lft to the maximum allowed
  net: ipv6/addrconf: clamp preferred_lft to the minimum required
  Documentation: networking: explain what happens if temp_valid_lft is
    too small
  Documentation: networking: explain what happens if temp_prefered_lft
    is too small or too large

 Documentation/networking/ip-sysctl.rst | 10 ++++++++--
 net/ipv6/addrconf.c                    | 21 +++++++++++++++------
 2 files changed, 23 insertions(+), 8 deletions(-)

-- 
2.42.0

[PATCH v2 1/5] net: ipv6/addrconf: avoid integer underflow in ipv6_create_tempaddr

[Alex Henrie]

The existing code incorrectly casted a negative value (the result of a
subtraction) to an unsigned value without checking. For example, if
/proc/sys/net/ipv6/conf/*/temp_prefered_lft was set to 1, the preferred
lifetime would jump to 4 billion seconds. On my machine and network the
shortest lifetime that avoided underflow was 3 seconds.

Fixes: 76506a986dc3 (IPv6: fix DESYNC_FACTOR, 2016-10-13)
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 net/ipv6/addrconf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 94cec2075eee..c93a2b9a9172 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(long,
 				  idev->cnf.max_desync_factor,
 				  cnf_temp_preferred_lft - regen_advance);
 
-- 
2.42.0

[PATCH v2 2/5] net: ipv6/addrconf: clamp preferred_lft to the maximum allowed

[Alex Henrie]

Without this patch, there is nothing to stop the preferred lifetime of a
temporary address from being greater than its valid lifetime. If that
was the case, the valid lifetime was effectively ignored.

Fixes: 76506a986dc3 (IPv6: fix DESYNC_FACTOR, 2016-10-13)
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 net/ipv6/addrconf.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index c93a2b9a9172..561c6266040a 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1387,6 +1387,7 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, bool block)
 			      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, cfg.valid_lft, cfg.preferred_lft);
 
 	cfg.plen = ifp->prefix_len;
 	tmp_tstamp = ifp->tstamp;
-- 
2.42.0

[PATCH v2 3/5] net: ipv6/addrconf: clamp preferred_lft to the minimum required

[Alex Henrie]

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 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
Fixes: eac55bf97094 (IPv6: do not create temporary adresses with too short preferred lifetime, 2008-04-02)
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 net/ipv6/addrconf.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 561c6266040a..05c22dac32e6 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1395,15 +1395,23 @@ 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.
+	 *
+	 * Clamp the preferred lifetime to a minimum of regen_advance, unless
+	 * that would exceed valid_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) {
+	if (cfg.preferred_lft <= regen_advance + age)
+		cfg.preferred_lft = regen_advance + age + 1;
+	if (cfg.preferred_lft > cfg.valid_lft) {
 		in6_ifa_put(ifp);
 		in6_dev_put(idev);
 		ret = -1;
-- 
2.42.0

[PATCH v2 4/5] Documentation: networking: explain what happens if temp_valid_lft is too small

[Alex Henrie]

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 Documentation/networking/ip-sysctl.rst | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index 4a010a7cde7f..ae196e78df88 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst
@@ -2462,7 +2462,9 @@ use_tempaddr - INTEGER
 		* -1 (for point-to-point devices and loopback devices)
 
 temp_valid_lft - INTEGER
-	valid lifetime (in seconds) for temporary addresses.
+	valid lifetime (in seconds) for temporary addresses. If less than the
+	minimum required lifetime (typically 5 seconds), temporary addresses
+	will not be created.
 
 	Default: 172800 (2 days)
 
-- 
2.42.0

[PATCH v2 5/5] Documentation: networking: explain what happens if temp_prefered_lft is too small or too large

[Alex Henrie]

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 Documentation/networking/ip-sysctl.rst | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index ae196e78df88..65daececd9bd 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst
@@ -2469,7 +2469,11 @@ temp_valid_lft - INTEGER
 	Default: 172800 (2 days)
 
 temp_prefered_lft - INTEGER
-	Preferred lifetime (in seconds) for temporary addresses.
+	Preferred lifetime (in seconds) for temporary addresses. If
+	temp_prefered_lft is less than the minimum required lifetime (typically
+	5 seconds), the preferred lifetime is the minimum required. If
+	temp_prefered_lft is greater than temp_valid_lft, the preferred lifetime
+	is temp_valid_lft.
 
 	Default: 86400 (1 day)
 
-- 
2.42.0

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

[Jakub Kicinski]

On Mon, 28 Aug 2023 23:44:45 -0600 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 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.

Not entirely sure what the best way to handle this is.
And whether the patch should be treated as a Fix or "general
improvement" - meaning - whether we should try to backport this :(

> Link: https://serverfault.com/a/1031168/310447
> Fixes: eac55bf97094 (IPv6: do not create temporary adresses with too short preferred lifetime, 2008-04-02)

Thanks for adding the Fixes tag - you're missing the quotes inside
the parenthesis:

Fixes: eac55bf97094 ("IPv6: do not create temporary adresses with too short preferred lifetime, 2008-04-02")

The exact format is important since people may script around it.
Since we haven't heard back from Paolo or David on v2 could you repost
with that fixed?
-- 
pw-bot: cr

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

[Alex Henrie]

On Wed, Aug 30, 2023 at 7:28 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 28 Aug 2023 23:44:45 -0600 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 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.
>
> Not entirely sure what the best way to handle this is.
> And whether the patch should be treated as a Fix or "general
> improvement" - meaning - whether we should try to backport this :(

I'm not exactly a subject matter expert here, but for what it's worth,
I think it's important but not important enough to backport. (I would
definitely like to backport the integer underflow fix though.) I'd
love to get more people to test these patches and to hear more from
the original authors.

> > Link: https://serverfault.com/a/1031168/310447
> > Fixes: eac55bf97094 (IPv6: do not create temporary adresses with too short preferred lifetime, 2008-04-02)
>
> Thanks for adding the Fixes tag - you're missing the quotes inside
> the parenthesis:
>
> Fixes: eac55bf97094 ("IPv6: do not create temporary adresses with too short preferred lifetime, 2008-04-02")
>
> The exact format is important since people may script around it.
> Since we haven't heard back from Paolo or David on v2 could you repost
> with that fixed?

Sorry, I should have looked at the examples more closely instead of
assuming that they were the same as `git log --format=ref`. I will
send a v3 with the Fixes tags in the conventional Linux kernel format.

Thanks for the feedback,

-Alex

Re: [PATCH v2 2/5] net: ipv6/addrconf: clamp preferred_lft to the maximum allowed

[Paolo Abeni]

On Mon, 2023-08-28 at 23:44 -0600, Alex Henrie wrote:
> Without this patch, there is nothing to stop the preferred lifetime of a
> temporary address from being greater than its valid lifetime. If that
> was the case, the valid lifetime was effectively ignored.

AFAICS this change makes the ipv6 implementation more in compliance
with the RFC, but on the flip side it will also break existing users
(if any) which set preferred > valid as a way to get an unlimited
validity period.

I'm quite unsure if the above is really the best option, but I think it
should not threaded as a fix.

My suggestion would be to re-send the uncontroversial patch 1/5 as a
stand-alone fix, and the following patches as a series targeting net-
next (no fixes tag there).

Cheers,

Paolo

Re: [PATCH v2 5/5] Documentation: networking: explain what happens if temp_prefered_lft is too small or too large

[Paolo Abeni]

On Mon, 2023-08-28 at 23:44 -0600, Alex Henrie wrote:
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>  Documentation/networking/ip-sysctl.rst | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
> index ae196e78df88..65daececd9bd 100644
> --- a/Documentation/networking/ip-sysctl.rst
> +++ b/Documentation/networking/ip-sysctl.rst
> @@ -2469,7 +2469,11 @@ temp_valid_lft - INTEGER
>  	Default: 172800 (2 days)
>  
>  temp_prefered_lft - INTEGER
> -	Preferred lifetime (in seconds) for temporary addresses.
> +	Preferred lifetime (in seconds) for temporary addresses. If
> +	temp_prefered_lft is less than the minimum required lifetime (typically
> +	5 seconds), the preferred lifetime is the minimum required. If
> +	temp_prefered_lft is greater than temp_valid_lft, the preferred lifetime
> +	is temp_valid_lft.

I think the above could be more clear as:

"""
	If temp_prefered_lft is less than the minimum required lifetime (typically
	5 seconds), the preferred lifetime is extended to the minimum required.
	If temp_prefered_lft is greater than temp_valid_lft, the preferred
	lifetime is limited to temp_valid_lft.
"""

cheers,

Paolo

[PATCH v3] net: ipv6/addrconf: avoid integer underflow in ipv6_create_tempaddr

[Alex Henrie]

The existing code incorrectly casted a negative value (the result of a
subtraction) to an unsigned value without checking. For example, if
/proc/sys/net/ipv6/conf/*/temp_prefered_lft was set to 1, the preferred
lifetime would jump to 4 billion seconds. On my machine and network the
shortest lifetime that avoided underflow was 3 seconds.

Fixes: 76506a986dc3 ("IPv6: fix DESYNC_FACTOR")
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
Changes from v2:
- Use conventional format for "Fixes" line
- Send separately and leave the other four patches for later
---
 net/ipv6/addrconf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 967913ad65e5..0b6ee962c84e 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1378,7 +1378,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(long,
 				  idev->cnf.max_desync_factor,
 				  cnf_temp_preferred_lft - regen_advance);
 
-- 
2.42.0

Re: [PATCH v3] net: ipv6/addrconf: avoid integer underflow in ipv6_create_tempaddr

[David Ahern]

On 8/31/23 10:41 PM, Alex Henrie wrote:
> The existing code incorrectly casted a negative value (the result of a
> subtraction) to an unsigned value without checking. For example, if
> /proc/sys/net/ipv6/conf/*/temp_prefered_lft was set to 1, the preferred
> lifetime would jump to 4 billion seconds. On my machine and network the
> shortest lifetime that avoided underflow was 3 seconds.
> 
> Fixes: 76506a986dc3 ("IPv6: fix DESYNC_FACTOR")
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
> Changes from v2:
> - Use conventional format for "Fixes" line
> - Send separately and leave the other four patches for later
> ---
>  net/ipv6/addrconf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH v3] net: ipv6/addrconf: avoid integer underflow in ipv6_create_tempaddr

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Thu, 31 Aug 2023 22:41:27 -0600 you wrote:
> The existing code incorrectly casted a negative value (the result of a
> subtraction) to an unsigned value without checking. For example, if
> /proc/sys/net/ipv6/conf/*/temp_prefered_lft was set to 1, the preferred
> lifetime would jump to 4 billion seconds. On my machine and network the
> shortest lifetime that avoided underflow was 3 seconds.
> 
> Fixes: 76506a986dc3 ("IPv6: fix DESYNC_FACTOR")
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> 
> [...]

Here is the summary with links:
  - [v3] net: ipv6/addrconf: avoid integer underflow in ipv6_create_tempaddr
    https://git.kernel.org/netdev/net/c/f31867d0d9d8

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

[PATCH resend 1/4] net: ipv6/addrconf: clamp preferred_lft to the maximum allowed

[Alex Henrie]

Without this patch, there is nothing to stop the preferred lifetime of a
temporary address from being greater than its valid lifetime. If that
was the case, the valid lifetime was effectively ignored.

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 net/ipv6/addrconf.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 0b6ee962c84e..e51c30d1daff 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1397,6 +1397,7 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, bool block)
 			      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, cfg.valid_lft, cfg.preferred_lft);
 
 	cfg.plen = ifp->prefix_len;
 	tmp_tstamp = ifp->tstamp;
-- 
2.42.0

[PATCH resend 2/4] net: ipv6/addrconf: clamp preferred_lft to the minimum required

[Alex Henrie]

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 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 | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index e51c30d1daff..a1eec8f09594 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1405,15 +1405,23 @@ 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.
+	 *
+	 * Clamp the preferred lifetime to a minimum of regen_advance, unless
+	 * that would exceed valid_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) {
+	if (cfg.preferred_lft <= regen_advance + age)
+		cfg.preferred_lft = regen_advance + age + 1;
+	if (cfg.preferred_lft > cfg.valid_lft) {
 		in6_ifa_put(ifp);
 		in6_dev_put(idev);
 		ret = -1;
-- 
2.42.0

[PATCH resend 3/4] Documentation: networking: explain what happens if temp_valid_lft is too small

[Alex Henrie]

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 Documentation/networking/ip-sysctl.rst | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index a66054d0763a..f200382858da 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst
@@ -2471,7 +2471,9 @@ use_tempaddr - INTEGER
 		* -1 (for point-to-point devices and loopback devices)
 
 temp_valid_lft - INTEGER
-	valid lifetime (in seconds) for temporary addresses.
+	valid lifetime (in seconds) for temporary addresses. If less than the
+	minimum required lifetime (typically 5 seconds), temporary addresses
+	will not be created.
 
 	Default: 172800 (2 days)
 
-- 
2.42.0

[PATCH resend 4/4] Documentation: networking: explain what happens if temp_prefered_lft is too small or too large

[Alex Henrie]

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 Documentation/networking/ip-sysctl.rst | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index f200382858da..d919380b1729 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst
@@ -2478,7 +2478,11 @@ temp_valid_lft - INTEGER
 	Default: 172800 (2 days)
 
 temp_prefered_lft - INTEGER
-	Preferred lifetime (in seconds) for temporary addresses.
+	Preferred lifetime (in seconds) for temporary addresses. If
+	temp_prefered_lft is less than the minimum required lifetime (typically
+	5 seconds), the preferred lifetime is the minimum required. If
+	temp_prefered_lft is greater than temp_valid_lft, the preferred lifetime
+	is temp_valid_lft.
 
 	Default: 86400 (1 day)
 
-- 
2.42.0

Re: [PATCH resend 1/4] net: ipv6/addrconf: clamp preferred_lft to the maximum allowed

[Jakub Kicinski]

On Tue, 24 Oct 2023 13:40:01 -0600 Alex Henrie wrote:
> Without this patch, there is nothing to stop the preferred lifetime of a
> temporary address from being greater than its valid lifetime. If that
> was the case, the valid lifetime was effectively ignored.

Please:
 - add cover letter, with the changes from v2
 - PATCH net-next v2 in the subject prefix
 - do not post in-reply-to, instead put a lore link:
   https://lore.kernel.org/all/20230829054623.104293-1-alexhenrie24@gmail.com/
   to the v2 in the cover letter.