Is there a race between __mod_timer() and del_timer()?

Is there a race between the optimisation for networking code in __mod_timer()
and del_timer() - or, at least, a race that matters?

Consider:

	CPU A				CPU B
	===============================	===============================
	[timer X is active]
	==>__mod_timer(X)
	if (timer_pending(timer))
		[Take the true path]
	-- IRQ --			==>del_timer(X)
					<==
	if (timer->expires == expires)
		[Take the true path]
	<==return 1
	[timer X is not active]

There's no locking to prevent this, but __mod_timer() returns without
restarting the timer.  I'm not sure this is a problem exactly, however, since
del_timer() *was* issued, and could've deleted the timer after __mod_timer()
returned.

A couple of possible alleviations:

 (1) Recheck timer_pending() before returning from __mod_timer().

 (2) Set timer->expires to jiffies in del_timer() - but since there's nothing
     preventing the optimisation in __mod_timer() from occurring concurrently
     with del_timer(), this probably won't help.

I think it might just be best to put a note in the comments in __mod_timer().

Thoughts?

David

David Howells [off-list ref] wrote:

I think it might just be best to put a note in the comments in __mod_timer().

How about the attached?

David
---
commit d538c734f9bf885292b88a81a06c5efee528d70d
Author: David Howells [off-list ref]
Date:   Wed Nov 8 10:20:27 2017 +0000

    Add a comment into __mod_timer() noting a possible race with del_timer()
    
    Add a comment into __mod_timer() noting a possible race with del_timer() in
    which the 'common optimization' case could leave the timer unstarted if
    del_timer() happens between the timer_pending() check and the timer
    expiration check.
    
    Signed-off-by: David Howells [off-list ref]

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index f2674a056c26..e0ac4486529c 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c

@@ -949,6 +949,9 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only)
 		 * The downside of this optimization is that it can result in
 		 * larger granularity than you would get from adding a new
 		 * timer with this expiry.
+		 *
+		 * Note that if del_timer() happens whilst we're just here, we
+		 * will return with the timer unstarted.
 		 */
 		if (timer->expires == expires)
 			return 1;

On Wed, 8 Nov 2017, David Howells wrote:

Is there a race between the optimisation for networking code in __mod_timer()
and del_timer() - or, at least, a race that matters?

Consider:

	CPU A				CPU B
	===============================	===============================
	[timer X is active]
	==>__mod_timer(X)
	if (timer_pending(timer))
		[Take the true path]
	-- IRQ --			==>del_timer(X)
					<==
	if (timer->expires == expires)
		[Take the true path]
	<==return 1
	[timer X is not active]

There's no locking to prevent this, but __mod_timer() returns without
restarting the timer.  I'm not sure this is a problem exactly, however, since
del_timer() *was* issued, and could've deleted the timer after __mod_timer()
returned.

Correct, if two CPUs fiddle with the same timer concurrently then there is
no guaranteed outcome.

A couple of possible alleviations:

 (1) Recheck timer_pending() before returning from __mod_timer().

That's just adding more instructions into that code path for a dubious
value.

 (2) Set timer->expires to jiffies in del_timer() - but since there's nothing
     preventing the optimisation in __mod_timer() from occurring concurrently
     with del_timer(), this probably won't help.

Right.

I think it might just be best to put a note in the comments in __mod_timer().

Agreed.

Thanks,

	tglx

On Wed, Nov 8, 2017 at 2:15 AM, David Howells [off-list ref] wrote:

 (2) Set timer->expires to jiffies in del_timer() - but since there's nothing
     preventing the optimisation in __mod_timer() from occurring concurrently
     with del_timer(), this probably won't help.

Right. The "race" is fundamental, and not in the timer code, but in the user.

If somebody does "del_timer()" at the same time somebody else modifies
the timer, it's not clear which one will win. The timer going away is
basically just "somebody modified it, but then immediately afterwards
another user deleted it".

So the modification was successful, but the end result is that the
timer is deleted, so it obviously isn't started.

I'm not even sure it merits a comment in the timer code, because the
timer code seems to do the right thing. The problem is in whoever
modifies and deletes a timer at the same time. It's fundamentally not
well-defined: either operation might happen "last", so you may end up
with a timer active or not, based purely on timing and luck.

              Linus