| Message ID | 20181025211352.112096-1-brendanhiggins@google.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v1] net: ipv6: fix racey clock check in route cache aging logic | expand |
On 10/25/2018 02:13 PM, Brendan Higgins wrote: > Fix a bug where, with certain settings, the aging logic does not use the > time passed in as the current time, but instead directly checks jiffies. > > This bug can be reproduced with (and this fix verified with) the test > at: https://kunit-review.googlesource.com/c/linux/+/1156 > > Fixes: 31afeb425f7f ("ipv6: change route cache aging logic") > Discovered-by-KUnit: https://kunit-review.googlesource.com/c/linux/+/1156 > Signed-off-by: Brendan Higgins <brendanhiggins@google.com> > --- > net/ipv6/route.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/ipv6/route.c b/net/ipv6/route.c > index 2a7423c394560..54d28b91fd840 100644 > --- a/net/ipv6/route.c > +++ b/net/ipv6/route.c > @@ -1734,7 +1734,7 @@ static void rt6_age_examine_exception(struct rt6_exception_bucket *bucket, > rt6_remove_exception(bucket, rt6_ex); > return; > } > - } else if (time_after(jiffies, rt->dst.expires)) { > + } else if (time_after(now, rt->dst.expires)) { > RT6_TRACE("purging expired route %p\n", rt); > rt6_remove_exception(bucket, rt6_ex); > return; > I do not think there is a bug here ? As a matter of fact, using the latest value of jiffies is probably better, since in some cases the @now variable could be quite in the past.
On Thu, Oct 25, 2018 at 2:40 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > On 10/25/2018 02:13 PM, Brendan Higgins wrote: <snip> > > > > diff --git a/net/ipv6/route.c b/net/ipv6/route.c > > index 2a7423c394560..54d28b91fd840 100644 > > --- a/net/ipv6/route.c > > +++ b/net/ipv6/route.c > > @@ -1734,7 +1734,7 @@ static void rt6_age_examine_exception(struct rt6_exception_bucket *bucket, > > rt6_remove_exception(bucket, rt6_ex); > > return; > > } > > - } else if (time_after(jiffies, rt->dst.expires)) { > > + } else if (time_after(now, rt->dst.expires)) { > > RT6_TRACE("purging expired route %p\n", rt); > > rt6_remove_exception(bucket, rt6_ex); > > return; > > > > > I do not think there is a bug here ? > > As a matter of fact, using the latest value of jiffies is probably better, > since in some cases the @now variable could be quite in the past. Then why do we pass the `now` parameter in at all and use it at all, like here: https://elixir.bootlin.com/linux/latest/source/net/ipv6/route.c#L1764 ? I am still skeptical that we should check jiffies in each check, but we should at least be consistent. Cheers
On 10/25/2018 02:46 PM, Brendan Higgins wrote: > On Thu, Oct 25, 2018 at 2:40 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: >> On 10/25/2018 02:13 PM, Brendan Higgins wrote: > <snip> >>> >>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c >>> index 2a7423c394560..54d28b91fd840 100644 >>> --- a/net/ipv6/route.c >>> +++ b/net/ipv6/route.c >>> @@ -1734,7 +1734,7 @@ static void rt6_age_examine_exception(struct rt6_exception_bucket *bucket, >>> rt6_remove_exception(bucket, rt6_ex); >>> return; >>> } >>> - } else if (time_after(jiffies, rt->dst.expires)) { >>> + } else if (time_after(now, rt->dst.expires)) { >>> RT6_TRACE("purging expired route %p\n", rt); >>> rt6_remove_exception(bucket, rt6_ex); >>> return; >>> >> >> >> I do not think there is a bug here ? >> >> As a matter of fact, using the latest value of jiffies is probably better, >> since in some cases the @now variable could be quite in the past. > > Then why do we pass the `now` parameter in at all and use it at all, > like here: https://elixir.bootlin.com/linux/latest/source/net/ipv6/route.c#L1764 > ? > > I am still skeptical that we should check jiffies in each check, but > we should at least be consistent. Well, this is a case where we do not really care. When a bug is fixed (you added a Fixes: tag which is good), we want to understand the real problem that needs to be fixed on stable kernels. Since this does not seem to be a real issue, I would suggest you send a cleanup patch when net-next is open (few days after linux-4.20-rc1 is release)
On Thu, Oct 25, 2018 at 3:15 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > On 10/25/2018 02:46 PM, Brendan Higgins wrote: > > On Thu, Oct 25, 2018 at 2:40 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > >> On 10/25/2018 02:13 PM, Brendan Higgins wrote: > > <snip> > >>> > >>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c > >>> index 2a7423c394560..54d28b91fd840 100644 > >>> --- a/net/ipv6/route.c > >>> +++ b/net/ipv6/route.c > >>> @@ -1734,7 +1734,7 @@ static void rt6_age_examine_exception(struct rt6_exception_bucket *bucket, > >>> rt6_remove_exception(bucket, rt6_ex); > >>> return; > >>> } > >>> - } else if (time_after(jiffies, rt->dst.expires)) { > >>> + } else if (time_after(now, rt->dst.expires)) { > >>> RT6_TRACE("purging expired route %p\n", rt); > >>> rt6_remove_exception(bucket, rt6_ex); > >>> return; > >>> > >> > >> > >> I do not think there is a bug here ? > >> > >> As a matter of fact, using the latest value of jiffies is probably better, > >> since in some cases the @now variable could be quite in the past. > > > > Then why do we pass the `now` parameter in at all and use it at all, > > like here: https://elixir.bootlin.com/linux/latest/source/net/ipv6/route.c#L1764 > > ? > > > > I am still skeptical that we should check jiffies in each check, but > > we should at least be consistent. > > Well, this is a case where we do not really care. > > When a bug is fixed (you added a Fixes: tag which is good), we want > to understand the real problem that needs to be fixed on stable kernels. So by bug you mean user visible bug? > > Since this does not seem to be a real issue, I would suggest you send a cleanup > patch when net-next is open (few days after linux-4.20-rc1 is release) Sounds good, I will resend shortly. Thanks!
diff --git a/net/ipv6/route.c b/net/ipv6/route.c index 2a7423c394560..54d28b91fd840 100644 --- a/net/ipv6/route.c +++ b/net/ipv6/route.c @@ -1734,7 +1734,7 @@ static void rt6_age_examine_exception(struct rt6_exception_bucket *bucket, rt6_remove_exception(bucket, rt6_ex); return; } - } else if (time_after(jiffies, rt->dst.expires)) { + } else if (time_after(now, rt->dst.expires)) { RT6_TRACE("purging expired route %p\n", rt); rt6_remove_exception(bucket, rt6_ex); return;
Fix a bug where, with certain settings, the aging logic does not use the time passed in as the current time, but instead directly checks jiffies. This bug can be reproduced with (and this fix verified with) the test at: https://kunit-review.googlesource.com/c/linux/+/1156 Fixes: 31afeb425f7f ("ipv6: change route cache aging logic") Discovered-by-KUnit: https://kunit-review.googlesource.com/c/linux/+/1156 Signed-off-by: Brendan Higgins <brendanhiggins@google.com> --- net/ipv6/route.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)