linux-kernel - Re: [Question]: try to fix contention between expire_timers and try_to_del_timer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 27 Jul 2017 16:14:03 +0100
From:   Will Deacon <will.deacon@....com>
To:     qiaozhou <qiaozhou@...micro.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        John Stultz <john.stultz@...aro.org>, sboyd@...eaurora.org,
        LKML <linux-kernel@...r.kernel.org>,
        Wang Wilbur <wilburwang@...micro.com>,
        Marc Zyngier <marc.zyngier@....com>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [Question]: try to fix contention between expire_timers and
 try_to_del_timer_sync

On Thu, Jul 27, 2017 at 09:29:20AM +0800, qiaozhou wrote:
> On 2017年07月26日 22:16, Thomas Gleixner wrote:
> >--- a/kernel/time/timer.c
> >+++ b/kernel/time/timer.c
> >@@ -1301,10 +1301,12 @@ static void expire_timers(struct timer_b
> >  		if (timer->flags & TIMER_IRQSAFE) {
> >  			raw_spin_unlock(&base->lock);
> >  			call_timer_fn(timer, fn, data);
> >+			base->running_timer = NULL;
> >  			raw_spin_lock(&base->lock);
> >  		} else {
> >  			raw_spin_unlock_irq(&base->lock);
> >  			call_timer_fn(timer, fn, data);
> >+			base->running_timer = NULL;
> >  			raw_spin_lock_irq(&base->lock);
> >  		}
> >  	}
> It should work for this particular issue and I'll test it. Previously I
> thought it was unsafe to touch base->running_timer without holding lock.

I think it works out in practice because base->lock and base->running_timer
share a cacheline, so end up being ordered correctly. We should probably be
using READ_ONCE/WRITE_ONCE for accessing the running_time field though.

One thing I don't get though, is why try_to_del_timer_sync needs to check
base->running_timer at all. Given that it holds the base->lock, can't it
be the person that sets it to NULL?

Will