Received-SPF: pass (google.com: domain of linux-kernel+bounces-91619-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <tencent_2E076343DA2D43D9B05F9DE3A01E0ACF2407@qq.com>
Content-Type: text/plain;
	charset=utf-8
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3774.100.2.1.4\))
Subject: Re: [PATCH] rcutorture: Fix
 rcu_torture_pipe_update_one()/rcu_torture_writer() data race and concurrency
 bug
From: linke <lilinke99@qq.com>
In-Reply-To: <0640c3b9-fa19-4c2d-9158-a6aa5b398da3@paulmck-laptop>
Date: Tue, 5 Mar 2024 11:30:18 +0800
Cc: Joel Fernandes <joel@joelfernandes.org>,
 Davidlohr Bueso <dave@stgolabs.net>,
 Josh Triplett <josh@joshtriplett.org>,
 Frederic Weisbecker <frederic@kernel.org>,
 Neeraj Upadhyay <quic_neeraju@quicinc.com>,
 Boqun Feng <boqun.feng@gmail.com>,
 Steven Rostedt <rostedt@goodmis.org>,
 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
 Lai Jiangshan <jiangshanlai@gmail.com>,
 Zqiang <qiang.zhang1211@gmail.com>,
 linux-kernel@vger.kernel.org,
 rcu@vger.kernel.org
Content-Transfer-Encoding: quoted-printable
References: <tencent_B51A9DA220288A95A435E3435A0443BEB007@qq.com>
 <f3624f39-bbb1-451d-8161-8518e4108d8e@joelfernandes.org>
 <4857c5ef-bd8f-4670-87ac-0600a1699d05@paulmck-laptop>
 <a8e9c84f-c354-4536-b676-d38043913d09@joelfernandes.org>
 <a9ff4787-1b07-4d47-b2a0-5eb1336d3710@paulmck-laptop>
 <51c4b25f-1cdc-4bcc-8b40-c98096bebacf@joelfernandes.org>
 <0640c3b9-fa19-4c2d-9158-a6aa5b398da3@paulmck-laptop>
To: paulmck@kernel.org

Thank you both. This looks good to me. I will send a new patch.

> 2024=E5=B9=B43=E6=9C=885=E6=97=A5 04:47=EF=BC=8CPaul E. McKenney =
<paulmck@kernel.org> =E5=86=99=E9=81=93=EF=BC=9A
>=20
> On Mon, Mar 04, 2024 at 03:13:10PM -0500, Joel Fernandes wrote:
>>=20
>>=20
>> On 3/4/2024 2:44 PM, Paul E. McKenney wrote:
>>> On Mon, Mar 04, 2024 at 02:10:09PM -0500, Joel Fernandes wrote:
>>>>=20
>>>>=20
>>>> On 3/4/2024 12:14 PM, Paul E. McKenney wrote:
>>>>> On Mon, Mar 04, 2024 at 11:19:21AM -0500, Joel Fernandes wrote:
>>>>>>=20
>>>>>>=20
>>>>>> On 3/4/2024 5:54 AM, linke li wrote:
>>>>>>> Some changes are done to fix a data race in commit 202489101f2e =
("rcutorture: Fix rcu_torture_one_read()/rcu_torture_writer() data =
race")
>>>>>>>=20
>>>>>>> {
>>>>>>> 	int i;
>>>>>>>=20
>>>>>>> -	i =3D rp->rtort_pipe_count;
>>>>>>> +	i =3D READ_ONCE(rp->rtort_pipe_count);
>>>>>>> 	if (i > RCU_TORTURE_PIPE_LEN)
>>>>>>> 		i =3D RCU_TORTURE_PIPE_LEN;
>>>>>>> 	atomic_inc(&rcu_torture_wcount[i]);
>>>>>>> -	if (++rp->rtort_pipe_count >=3D RCU_TORTURE_PIPE_LEN) {
>>>>>>> +	WRITE_ONCE(rp->rtort_pipe_count, i + 1);
>>>>>>> +	if (rp->rtort_pipe_count >=3D RCU_TORTURE_PIPE_LEN) {
>>>>>>> 		rp->rtort_mbtest =3D 0;
>>>>>>> 		return true;
>>>>>>> 	}
>>>>>>>=20
>>>>>>> But ++rp->rtort_pipe_count is meant to add itself by 1, not give =
i+1 to
>>>>>>> rp->rtort_pipe_count, because rp->rtort_pipe_count may write by
>>>>>>> rcu_torture_writer() concurrently.
>>>>>>>=20
>>>>>>> Also, rp->rtort_pipe_count in the next line should be read using
>>>>>>> READ_ONCE() because of data race.
>>>>>>>=20
>>>>>>> Signed-off-by: linke li <lilinke99@qq.com>
>>>>>>> ---
>>>>>>> kernel/rcu/rcutorture.c | 4 ++--
>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>=20
>>>>>>> diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
>>>>>>> index 7567ca8e743c..00059ace4fd5 100644
>>>>>>> --- a/kernel/rcu/rcutorture.c
>>>>>>> +++ b/kernel/rcu/rcutorture.c
>>>>>>> @@ -465,8 +465,8 @@ rcu_torture_pipe_update_one(struct =
rcu_torture *rp)
>>>>>>> 	if (i > RCU_TORTURE_PIPE_LEN)
>>>>>>> 		i =3D RCU_TORTURE_PIPE_LEN;
>>>>>>> 	atomic_inc(&rcu_torture_wcount[i]);
>>>>>>> -	WRITE_ONCE(rp->rtort_pipe_count, i + 1);
>>>>>>> -	if (rp->rtort_pipe_count >=3D RCU_TORTURE_PIPE_LEN) {
>>>>>>> +	WRITE_ONCE(rp->rtort_pipe_count, rp->rtort_pipe_count + =
1);
>>>>>>> +	if (READ_ONCE(rp->rtort_pipe_count) >=3D =
RCU_TORTURE_PIPE_LEN) {
>>>>>>=20
>>>>>> I want to say, I am not convinced with the patch because what's =
wrong with
>>>>>> writing to an old index?
>>>>>>=20
>>>>>> You win/lose the race anyway, say the CPU executed the =
WRITE_ONCE() a bit too
>>>>>> early/late and another WRITE_ONCE() lost/won, regardless of =
whether you wrote
>>>>>> the "incremented i" or "the increment from the latest value of =
pipe_count".
>>>>>>=20
>>>>>> Anyway, a slightly related/different question:
>>>>>>=20
>>>>>> Should that:
>>>>>> WRITE_ONCE(rp->rtort_pipe_count, rp->rtort_pipe_count + 1);
>>>>>>=20
>>>>>> Be:
>>>>>> WRITE_ONCE(rp->rtort_pipe_count, READ_ONCE(rp->rtort_pipe_count) =
+ 1);
>>>>>>=20
>>>>>> ?
>>>>>=20
>>>>> Thank you both!
>>>>>=20
>>>>> At first glance, I would argue for something like this:
>>>>>=20
>>>>> =
------------------------------------------------------------------------
>>>>>=20
>>>>> static bool
>>>>> rcu_torture_pipe_update_one(struct rcu_torture *rp)
>>>>> {
>>>>> 	int i;
>>>>> 	struct rcu_torture_reader_check *rtrcp =3D =
READ_ONCE(rp->rtort_chkp);
>>>>>=20
>>>>> 	if (rtrcp) {
>>>>> 		WRITE_ONCE(rp->rtort_chkp, NULL);
>>>>> 		smp_store_release(&rtrcp->rtc_ready, 1); // Pair with =
smp_load_acquire().
>>>>> 	}
>>>>> 	i =3D READ_ONCE(rp->rtort_pipe_count) + 1;
>>>>> 	if (i > RCU_TORTURE_PIPE_LEN)
>>>>> 		i =3D RCU_TORTURE_PIPE_LEN;
>>>>> 	atomic_inc(&rcu_torture_wcount[i]);
>>>>> 	WRITE_ONCE(rp->rtort_pipe_count, i);
>>>>> 	if (i >=3D RCU_TORTURE_PIPE_LEN) {
>>>>> 		rp->rtort_mbtest =3D 0;
>>>>> 		return true;
>>>>> 	}
>>>>> 	return false;
>>>>> }
>>>>>=20
>>>>> =
------------------------------------------------------------------------
>>>>>=20
>>>>> That is, move the increment to the read and replace the re-read =
with
>>>>> the value "i" that was just written.
>>>>=20
>>>> But that changes the original logic as well? It looks like with the =
above
>>>> change, you're now writing to =
rcu_torture_wcount[READ_ONCE(rp->rtort_pipe_count)
>>>> + 1] instead of =
rcu_torture_wcount[READ_ONCE(rp->rtort_pipe_count)].
>>>>=20
>>>> I think that might break rcutorture, because there is an increment =
outside of
>>>> the first 2 entries in rcu_torture_wcount but not sure (need to =
look more).
>>>=20
>>> Good point on never incrementing the zeroth entry!  Clearly I should
>>> have waited before replying.
>>>=20
>>> How about the following?
>>>=20
>>> =
------------------------------------------------------------------------
>>>=20
>>> static bool
>>> rcu_torture_pipe_update_one(struct rcu_torture *rp)
>>> {
>>> 	int i;
>>> 	struct rcu_torture_reader_check *rtrcp =3D =
READ_ONCE(rp->rtort_chkp);
>>>=20
>>> 	if (rtrcp) {
>>> 		WRITE_ONCE(rp->rtort_chkp, NULL);
>>> 		smp_store_release(&rtrcp->rtc_ready, 1); // Pair with =
smp_load_acquire().
>>> 	}
>>> 	i =3D READ_ONCE(rp->rtort_pipe_count);
>>> 	if (i > RCU_TORTURE_PIPE_LEN)
>>> 		i =3D RCU_TORTURE_PIPE_LEN;
>>> 	atomic_inc(&rcu_torture_wcount[i]);
>>> 	WRITE_ONCE(rp->rtort_pipe_count, i + 1);
>>> 	if (i + 1 >=3D RCU_TORTURE_PIPE_LEN) {
>>> 		rp->rtort_mbtest =3D 0;
>>> 		return true;
>>> 	}
>>> 	return false;
>>> }
>>=20
>> Yes, this looks good to me. Thanks,
>> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>=20
> Again, thank you.
>=20
> linke li, does this approach work for you?  If so, would you be =
willing to
> send a new patch along these lines?  If it does not work, what =
additional
> problems do you see?
>=20
> 							Thanx, Paul