[PATCH v1] net: ipv6: fix racey clock check in route cache aging logic

[PATCH v1] net: ipv6: fix racey clock check in route cache aging logic

[Brendan Higgins]

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;
-- 
2.19.1.568.g152ad8e336-goog

Re: [PATCH v1] net: ipv6: fix racey clock check in route cache aging logic

[Eric Dumazet]

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.

Re: [PATCH v1] net: ipv6: fix racey clock check in route cache aging logic

[Brendan Higgins]

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

Re: [PATCH v1] net: ipv6: fix racey clock check in route cache aging logic

[Eric Dumazet]

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)

Re: [PATCH v1] net: ipv6: fix racey clock check in route cache aging logic

[Brendan Higgins]

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!