linux-kernel - Re: [PATCH v7 6/6] rcu/segcblist: Add additional comments to explain smp

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:   Wed, 21 Oct 2020 23:16:43 +0200
From:   Frederic Weisbecker <frederic@...nel.org>
To:     Joel Fernandes <joel@...lfernandes.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...hat.com>,
        Josh Triplett <josh@...htriplett.org>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Marco Elver <elver@...gle.com>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        "Paul E. McKenney" <paulmck@...nel.org>, rcu <rcu@...r.kernel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        "Uladzislau Rezki (Sony)" <urezki@...il.com>,
        Frederic Weisbecker <fweisbec@...il.com>,
        Neeraj upadhyay <neeraj.iitr10@...il.com>,
        Alan Stern <stern@...land.harvard.edu>
Subject: Re: [PATCH v7 6/6] rcu/segcblist: Add additional comments to explain
 smp_mb()

On Wed, Oct 21, 2020 at 11:57:04AM -0700, Joel Fernandes wrote:
> On Mon, Oct 19, 2020 at 5:37 AM Frederic Weisbecker <frederic@...nel.org> wrote:
> > Now, reading the documentation of rcu_barrier() (thanks to you!):
> >
> >     Pseudo-code using rcu_barrier() is as follows:
> >
> >    1. Prevent any new RCU callbacks from being posted.
> >    2. Execute rcu_barrier().
> >    3. Allow the module to be unloaded.
> >
> 
> Basically, you are saying that if all CPUs agree that len == 0
> henceforth (through other memory barriers), then callback enqueuing
> does not need a memory barrier before setting length to 0.

I think setting length to 0 isn't much an issue. At worst we send a useless
IPI and queue a needless callback. But incrementing from 0 to 1 is precisely
what we don't want to miss.

> I think that makes sense but is it worth removing the memory barrier
> before WRITE(len, 1) and hoping after #1, the caller would have
> ensured things are fine? Also I am not sure if the above is the only
> usecase for rcu_barrier().

I'm not sure either. Also I need to check your scenario again.

> > cancel_work_sync() also seem to really sync as well. I'm less sure about del_timer_sync().
> >
> > Say we have:
> >
> > expire_timers (CPU 0)                               CPU 1
> > -------------                                       -----------
> > detach_timer(timer)
> > raw_spin_unlock(&base->lock);
> > call_timer_fn(timer, fn, baseclk);
> >    -> enqueue callback
> > //would need at least smp_wmb() here

Aah, my bad, the smp_mb() after inc_len does that.

> > base->running_timer = NULL;
> >
> >                                                     del_timer_sync() {
> >                                                         raw_spin_lock(&base->lock);
> >                                                         if (base->running_timer != timer)
> >                                                             ret = detach_if_pending(timer, base, true);
> >                                                                 if (!timer_pending())
> >                                                                     return 0;
> >                                                         raw_spin_unlock(&base->lock);
> >                                                     }
> >                                                     //would need at least smp_rmb() here

And rcu_seq_start() implies that, although I'm not sure that's what was intended.
So we are good.

> 
> Regarding "would need at least smp_rmb.." :
> But the rcu_barrier() has the control dependency we discussed in last
> emails, between READ(len) and whatever follows the rcu_barrier().
> That itself will provide the ordering right?

I'm not sure that was enough. The len itself has to be synchronized against
whatever callback enqueuer that got stopped.

> I could be missing something too :-/. But I'll include this patch in
> my next posting anyway and let us also maybe see if Paul disagrees.

Ok.

Thanks!