Received-SPF: pass (google.com: domain of linux-kernel+bounces-96396-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 7 Mar 2024 16:55:08 -0800
From: "Paul E. McKenney" <paulmck@kernel.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Julia Lawall <julia.lawall@inria.fr>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	Steven Rostedt <rostedt@goodmis.org>, linke li <lilinke99@qq.com>,
	joel@joelfernandes.org, boqun.feng@gmail.com, dave@stgolabs.net,
	frederic@kernel.org, jiangshanlai@gmail.com, josh@joshtriplett.org,
	linux-kernel@vger.kernel.org, qiang.zhang1211@gmail.com,
	quic_neeraju@quicinc.com, rcu@vger.kernel.org
Subject: Re: [PATCH] rcutorture: Fix
 rcu_torture_pipe_update_one()/rcu_torture_writer() data race and concurrency
 bug
Message-ID: <7b11260c-ebed-4440-9d35-d6a788151888@paulmck-laptop>
Reply-To: paulmck@kernel.org
References: <83b47424-e5e0-46de-aa63-d413a5aa6cec@paulmck-laptop>
 <CAHk-=wiX_zF5Mpt8kUm_LFQpYY-mshrXJPOe+wKNwiVhEUcU9g@mail.gmail.com>
 <851dc594-d2ea-4050-b7c6-e33a1cddf3a1@efficios.com>
 <72b14322-78c1-4479-9c4e-b0e11c1f0d53@paulmck-laptop>
 <bebbed4a-ced1-42c5-865c-dc9dc7857b6c@efficios.com>
 <c1bb35c4-29af-4a84-8ba7-81ba30639a69@paulmck-laptop>
 <CAHk-=wia769uoyVz3P7yZURhO8NNB7xeOLX07ZM2vWf1nTLYkQ@mail.gmail.com>
 <65a9665e-22d4-4f21-a1cb-7ef1c82ed078@paulmck-laptop>
 <alpine.DEB.2.22.394.2403072231130.3161@hadrien>
 <CAHk-=wjq1g4jOhDvGNyvTiBxwhu97+Ymszf3W4i6MS1jqw5=kg@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHk-=wjq1g4jOhDvGNyvTiBxwhu97+Ymszf3W4i6MS1jqw5=kg@mail.gmail.com>

On Thu, Mar 07, 2024 at 02:09:00PM -0800, Linus Torvalds wrote:
> On Thu, 7 Mar 2024 at 13:40, Julia Lawall <julia.lawall@inria.fr> wrote:
> >
> > I tried the following:
> >
> > @@
> > expression x;
> > @@
> >
> > *WRITE_ONCE(x,<+...READ_ONCE(x)...+>)
> >
> > This gave a number of results, shown below.  Let me know if some of them
> > are undesirable.
> 
> Well, all the ones you list do look like garbage.
> 
> That said, quite often the garbage does seem to be "we don't actually
> care about the result". Several of them look like statistics.

[ . . . ]

> > diff -u -p /home/julia/linux/kernel/rcu/tree.c /tmp/nothing/kernel/rcu/tree.c
> > --- /home/julia/linux/kernel/rcu/tree.c
> > +++ /tmp/nothing/kernel/rcu/tree.c
> > @@ -1620,8 +1620,6 @@ static void rcu_gp_fqs(bool first_time)
> >         /* Clear flag to prevent immediate re-entry. */
> >         if (READ_ONCE(rcu_state.gp_flags) & RCU_GP_FLAG_FQS) {
> >                 raw_spin_lock_irq_rcu_node(rnp);
> > -               WRITE_ONCE(rcu_state.gp_flags,
> > -                          READ_ONCE(rcu_state.gp_flags) & ~RCU_GP_FLAG_FQS);
> >                 raw_spin_unlock_irq_rcu_node(rnp);
> 
> This smells bad to me. The code is holding a lock, but apparently not
> one that protects gp_flags.
> 
> And that READ_ONCE->WRITE_ONCE sequence can corrupt all the other flags.
> 
> Maybe it's fine for some reason (that reason being either that the
> ONCE operations aren't actually needed at all, or because nobody
> *really* cares about the flags), but it smells.
> 
> > @@ -1882,8 +1880,6 @@ static void rcu_report_qs_rsp(unsigned l
> >  {
> >         raw_lockdep_assert_held_rcu_node(rcu_get_root());
> >         WARN_ON_ONCE(!rcu_gp_in_progress());
> > -       WRITE_ONCE(rcu_state.gp_flags,
> > -                  READ_ONCE(rcu_state.gp_flags) | RCU_GP_FLAG_FQS);
> >         raw_spin_unlock_irqrestore_rcu_node(rcu_get_root(), flags);
> 
> Same field, same lock held, same odd smelly pattern.
> 
> > -       WRITE_ONCE(rcu_state.gp_flags,
> > -                  READ_ONCE(rcu_state.gp_flags) | RCU_GP_FLAG_FQS);
> >         raw_spin_unlock_irqrestore_rcu_node(rnp_old, flags);
> 
> .. and again.

In all three cases, the updates are protected by the lock, so the
READ_ONCE() is unnecessary.  I have queued a commit to remove the
READ_ONCE()s.

Thanks to both of you (Julia and Linus) for spotting these!

							Thanx, Paul