[PATCH] ipv6: Fix preferred_lft not updating in some cases

[PATCH] ipv6: Fix preferred_lft not updating in some cases

[Paul Marks]

Consider the scenario where an IPv6 router is advertising a fixed
preferred_lft of 1800 seconds, while the valid_lft begins at 3600
seconds and counts down in realtime.

A client should reset its preferred_lft to 1800 every time the RA is
received, but a bug is causing Linux to ignore the update.

The core problem is here:
  if (prefered_lft != ifp->prefered_lft) {

Note that ifp->prefered_lft is an offset, so it doesn't decrease over
time.  Thus, the comparison is always (1800 != 1800), which fails to
trigger an update.

The most direct solution would be to compute a "stored_prefered_lft",
and use that value in the comparison.  But I think that trying to filter
out unnecessary updates here is a premature optimization.  In order for
the filter to apply, both of these would need to hold:

  - The advertised valid_lft and preferred_lft are both declining in
    real time.
  - No clock skew exists between the router & client.

So in this patch, I've set "update_lft = 1" unconditionally, which
allows the surrounding code to be greatly simplified.

Signed-off-by: Paul Marks <pmarks@google.com>
---
 net/ipv6/addrconf.c | 52 +++++++++++++++-------------------------------------
 1 file changed, 15 insertions(+), 37 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index d6ff126..9a5052c 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2193,43 +2193,21 @@ ok:
 			else
 				stored_lft = 0;
 			if (!update_lft && !create && stored_lft) {
-				if (valid_lft > MIN_VALID_LIFETIME ||
-				    valid_lft > stored_lft)
-					update_lft = 1;
-				else if (stored_lft <= MIN_VALID_LIFETIME) {
-					/* valid_lft <= stored_lft is always true */
-					/*
-					 * RFC 4862 Section 5.5.3e:
-					 * "Note that the preferred lifetime of
-					 *  the corresponding address is always
-					 *  reset to the Preferred Lifetime in
-					 *  the received Prefix Information
-					 *  option, regardless of whether the
-					 *  valid lifetime is also reset or
-					 *  ignored."
-					 *
-					 *  So if the preferred lifetime in
-					 *  this advertisement is different
-					 *  than what we have stored, but the
-					 *  valid lifetime is invalid, just
-					 *  reset prefered_lft.
-					 *
-					 *  We must set the valid lifetime
-					 *  to the stored lifetime since we'll
-					 *  be updating the timestamp below,
-					 *  else we'll set it back to the
-					 *  minimum.
-					 */
-					if (prefered_lft != ifp->prefered_lft) {
-						valid_lft = stored_lft;
-						update_lft = 1;
-					}
-				} else {
-					valid_lft = MIN_VALID_LIFETIME;
-					if (valid_lft < prefered_lft)
-						prefered_lft = valid_lft;
-					update_lft = 1;
-				}
+				const u32 minimum_lft = min(
+					stored_lft, (u32)MIN_VALID_LIFETIME);
+				valid_lft = max(valid_lft, minimum_lft);
+
+				/* RFC4862 Section 5.5.3e:
+				 * "Note that the preferred lifetime of the
+				 *  corresponding address is always reset to
+				 *  the Preferred Lifetime in the received
+				 *  Prefix Information option, regardless of
+				 *  whether the valid lifetime is also reset or
+				 *  ignored."
+				 *
+				 * So we should always update prefered_lft here.
+				 */
+				update_lft = 1;
 			}
 
 			if (update_lft) {
-- 
1.8.4

Re: [PATCH] ipv6: Fix preferred_lft not updating in some cases

[Hannes Frederic Sowa]

On Wed, Sep 25, 2013 at 03:12:55PM -0700, Paul Marks wrote:
> Consider the scenario where an IPv6 router is advertising a fixed
> preferred_lft of 1800 seconds, while the valid_lft begins at 3600
> seconds and counts down in realtime.
> 
> A client should reset its preferred_lft to 1800 every time the RA is
> received, but a bug is causing Linux to ignore the update.
> 
> The core problem is here:
>   if (prefered_lft != ifp->prefered_lft) {
> 
> Note that ifp->prefered_lft is an offset, so it doesn't decrease over
> time.  Thus, the comparison is always (1800 != 1800), which fails to
> trigger an update.
> 
> The most direct solution would be to compute a "stored_prefered_lft",
> and use that value in the comparison.  But I think that trying to filter
> out unnecessary updates here is a premature optimization.  In order for
> the filter to apply, both of these would need to hold:
> 
>   - The advertised valid_lft and preferred_lft are both declining in
>     real time.
>   - No clock skew exists between the router & client.
> 
> So in this patch, I've set "update_lft = 1" unconditionally, which
> allows the surrounding code to be greatly simplified.

I actually find this much harder to follow when verifying against the RFC.

> Signed-off-by: Paul Marks <pmarks@google.com>
> ---
>  net/ipv6/addrconf.c | 52 +++++++++++++++-------------------------------------
>  1 file changed, 15 insertions(+), 37 deletions(-)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index d6ff126..9a5052c 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -2193,43 +2193,21 @@ ok:
>  			else
>  				stored_lft = 0;
>  			if (!update_lft && !create && stored_lft) {
> -				if (valid_lft > MIN_VALID_LIFETIME ||
> -				    valid_lft > stored_lft)
> -					update_lft = 1;
> -				else if (stored_lft <= MIN_VALID_LIFETIME) {
> -					/* valid_lft <= stored_lft is always true */
> -					/*
> -					 * RFC 4862 Section 5.5.3e:
> -					 * "Note that the preferred lifetime of
> -					 *  the corresponding address is always
> -					 *  reset to the Preferred Lifetime in
> -					 *  the received Prefix Information
> -					 *  option, regardless of whether the
> -					 *  valid lifetime is also reset or
> -					 *  ignored."
> -					 *
> -					 *  So if the preferred lifetime in
> -					 *  this advertisement is different
> -					 *  than what we have stored, but the
> -					 *  valid lifetime is invalid, just
> -					 *  reset prefered_lft.
> -					 *
> -					 *  We must set the valid lifetime
> -					 *  to the stored lifetime since we'll
> -					 *  be updating the timestamp below,
> -					 *  else we'll set it back to the
> -					 *  minimum.
> -					 */
> -					if (prefered_lft != ifp->prefered_lft) {

Wouldn't the easiest solution be to just drop this if and execute the two
lines below unconditionally?

> -						valid_lft = stored_lft;
> -						update_lft = 1;
> -					}
> -				} else {
> -					valid_lft = MIN_VALID_LIFETIME;
> -					if (valid_lft < prefered_lft)
> -						prefered_lft = valid_lft;
> -					update_lft = 1;
> -				}
> +				const u32 minimum_lft = min(
> +					stored_lft, (u32)MIN_VALID_LIFETIME);
> +				valid_lft = max(valid_lft, minimum_lft);

Quick question: Don't we need a prefered_lft = min(preferred_lft, valid_lft)
here?

> +
> +				/* RFC4862 Section 5.5.3e:
> +				 * "Note that the preferred lifetime of the
> +				 *  corresponding address is always reset to
> +				 *  the Preferred Lifetime in the received
> +				 *  Prefix Information option, regardless of
> +				 *  whether the valid lifetime is also reset or
> +				 *  ignored."
> +				 *
> +				 * So we should always update prefered_lft here.
> +				 */
> +				update_lft = 1;
>  			}
>  
>  			if (update_lft) {

Thanks,

  Hannes

Re: [PATCH] ipv6: Fix preferred_lft not updating in some cases

[Paul Marks]

On Fri, Sep 27, 2013 at 1:16 AM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> On Wed, Sep 25, 2013 at 03:12:55PM -0700, Paul Marks wrote:
>> -                                     if (prefered_lft != ifp->prefered_lft) {
>
> Wouldn't the easiest solution be to just drop this if and execute the two
> lines below unconditionally?

Yes, that's also correct.  But is it not better to have simpler code
than shorter diffs?  Should we transliterate English to C, or think
about what the algorithm is actually doing?  The fact that this bug
has gone unnoticed provides some evidence that the code may have been
too complicated.

>> +                             const u32 minimum_lft = min(
>> +                                     stored_lft, (u32)MIN_VALID_LIFETIME);
>> +                             valid_lft = max(valid_lft, minimum_lft);
>
> Quick question: Don't we need a prefered_lft = min(preferred_lft, valid_lft)
> here?

The invariant is (preferred_lft <= valid_lft), and valid_lft can only
get bigger, so I don't think there's a problem.

Re: [PATCH] ipv6: Fix preferred_lft not updating in some cases

[Hannes Frederic Sowa]

On Fri, Sep 27, 2013 at 01:28:06PM -0700, Paul Marks wrote:
> On Fri, Sep 27, 2013 at 1:16 AM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
> > On Wed, Sep 25, 2013 at 03:12:55PM -0700, Paul Marks wrote:
> >> -                                     if (prefered_lft != ifp->prefered_lft) {
> >
> > Wouldn't the easiest solution be to just drop this if and execute the two
> > lines below unconditionally?
> 
> Yes, that's also correct.  But is it not better to have simpler code
> than shorter diffs?  Should we transliterate English to C, or think
> about what the algorithm is actually doing?  The fact that this bug
> has gone unnoticed provides some evidence that the code may have been
> too complicated.

I don't care about the length of diffs or shorter code. I would favour
a transliteration here because it makes verification easier (at least
for me). The algorithm is not that complex and I guess the bug has been
unnoticed because nobody ran into problems and cared til now.

So, why not get rid of update_lft then?

> >> +                             const u32 minimum_lft = min(
> >> +                                     stored_lft, (u32)MIN_VALID_LIFETIME);
> >> +                             valid_lft = max(valid_lft, minimum_lft);
> >
> > Quick question: Don't we need a prefered_lft = min(preferred_lft, valid_lft)
> > here?
> 
> The invariant is (preferred_lft <= valid_lft), and valid_lft can only
> get bigger, so I don't think there's a problem.

Ah, I got confused. Missed in the last case that it got tested earlier in the
function. Your code looks correct regarding every rule.

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Thanks,

  Hannes

Re: [PATCH] ipv6: Fix preferred_lft not updating in some cases

[David Miller]

From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Sat, 28 Sep 2013 22:28:28 +0200

> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Applied, thanks everyone.