linux-kernel - Re: rcu: WARNING in rcu_seq

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:   Tue, 7 Mar 2017 19:37:57 +0100
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Paul McKenney <paulmck@...ux.vnet.ibm.com>
Cc:     Boqun Feng <boqun.feng@...il.com>, josh@...htriplett.org,
        Steven Rostedt <rostedt@...dmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        jiangshanlai@...il.com, LKML <linux-kernel@...r.kernel.org>,
        syzkaller <syzkaller@...glegroups.com>
Subject: Re: rcu: WARNING in rcu_seq_end

On Tue, Mar 7, 2017 at 4:27 PM, Paul E. McKenney
<paulmck@...ux.vnet.ibm.com> wrote:
>> > [...]
>> >> >>
>> >> >> What is that mutex? And what locks/unlocks provide synchronization? I
>> >> >> see that one uses exp_mutex and another -- exp_wake_mutex.
>> >> >
>> >> > Both of them.
>> >> >
>> >> > ->exp_mutex is acquired by the task requesting the grace period, and
>> >> > the counter's first increment is done by that task under that mutex.
>> >> > This task then schedules a workqueue, which drives forward the grace
>> >> > period.  Upon grace-period completion, the workqueue handler does the
>> >> > second increment (the one that your patch addressed).  The workqueue
>> >> > handler then acquires ->exp_wake_mutex and wakes the task that holds
>> >> > ->exp_mutex (along with all other tasks waiting for this grace period),
>> >> > and that task releases ->exp_mutex, which allows the next grace period to
>> >> > start (and the first increment for that next grace period to be carried
>> >> > out under that lock).  The workqueue handler releases ->exp_wake_mutex
>> >> > after finishing its wakeups.
>> >>
>> >> Then we need the following for the case when task requesting the grace
>> >> period does not block, right?
>> >
>> > Won't be necessary I think, as the smp_mb() in rcu_seq_end() and the
>> > smp_mb__before_atomic() in sync_exp_work_done() already provide the
>> > required ordering, no?
>>
>> smp_mb() is probably fine, but smp_mb__before_atomic() is release not
>> acquire. If we want to play that game, then I guess we also need
>> smp_mb__after_atomic() there. But it would be way easier to understand
>> what's happens there and prove that it's correct, if we use
>> store_release/load_acquire.
>
> Fair point, how about the following?

I am not qualified enough to reason about these smp_mb__after_atomic.
>From practical point of view there may be enough barriers in the
resulting machine code already, but re formal semantics of adding
smp_mb__after_atomic after an unrelated subsequent atomic RMW op I
gave up. You must be the best candidate for this now :)



>                                                         Thanx, Paul
>
> ------------------------------------------------------------------------
>
> commit 6fd8074f1976596898e39f5b7ea1755652533906
> Author: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
> Date:   Tue Mar 7 07:21:23 2017 -0800
>
>     rcu: Add smp_mb__after_atomic() to sync_exp_work_done()
>
>     The sync_exp_work_done() function needs to fully order the counter-check
>     operation against anything happening after the corresponding grace period.
>     This is a theoretical bug, as all current architectures either provide
>     full ordering for atomic operation on the one hand or implement,
>     however, a little future-proofing is a good thing.  This commit
>     therefore adds smp_mb__after_atomic() after the atomic_long_inc()
>     in sync_exp_work_done().
>
>     Reported-by: Dmitry Vyukov <dvyukov@...gle.com>
>     Signed-off-by: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
>
> diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> index 027e123d93c7..652071abd9b4 100644
> --- a/kernel/rcu/tree_exp.h
> +++ b/kernel/rcu/tree_exp.h
> @@ -247,6 +247,7 @@ static bool sync_exp_work_done(struct rcu_state *rsp, atomic_long_t *stat,
>                 /* Ensure test happens before caller kfree(). */
>                 smp_mb__before_atomic(); /* ^^^ */
>                 atomic_long_inc(stat);
> +               smp_mb__after_atomic(); /* ^^^ */
>                 return true;
>         }
>         return false;