| Message ID | 1380147175-14874-1-git-send-email-pmarks@google.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) {
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(-)