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]

Message-ID: <20170307142740.uh2nnaw44albn3t2@tardis>
Date:   Tue, 7 Mar 2017 22:27:40 +0800
From:   Boqun Feng <boqun.feng@...il.com>
To:     Dmitry Vyukov <dvyukov@...gle.com>
Cc:     Paul McKenney <paulmck@...ux.vnet.ibm.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 07, 2017 at 08:05:19AM +0100, Dmitry Vyukov 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?

Regards,
Boqun

> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index d80c2587bed8..aa7ba83f6a56 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -3534,7 +3534,7 @@ static void rcu_seq_start(unsigned long *sp)
>  static void rcu_seq_end(unsigned long *sp)
>  {
>         smp_mb(); /* Ensure update-side operation before counter increment. */
> -       WRITE_ONCE(*sp, *sp + 1);
> +       smp_store_release(sp, *sp + 1);
>         WARN_ON_ONCE(*sp & 0x1);
>  }
> 
> @@ -3554,7 +3554,7 @@ static unsigned long rcu_seq_snap(unsigned long *sp)
>   */
>  static bool rcu_seq_done(unsigned long *sp, unsigned long s)
>  {
> -       return ULONG_CMP_GE(READ_ONCE(*sp), s);
> +       return ULONG_CMP_GE(smp_load_acquire(sp), s);
> 
>  }

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)