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

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 <redacted>
---
 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

On 10/25/2018 02:13 PM, Brendan Higgins wrote:

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 [off-list ref] wrote:

On 10/25/2018 02:13 PM, Brendan Higgins wrote:

<snip>

quoted

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 [off-list ref] wrote:

quoted

On 10/25/2018 02:13 PM, Brendan Higgins wrote:

<snip>

quoted

quoted

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 [off-list ref] wrote:

On 10/25/2018 02:46 PM, Brendan Higgins wrote:

quoted

On Thu, Oct 25, 2018 at 2:40 PM Eric Dumazet [off-list ref] wrote:

quoted

On 10/25/2018 02:13 PM, Brendan Higgins wrote:

<snip>

quoted

quoted

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!