Received-SPF: pass (google.com: domain of linux-kernel+bounces-90926-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <f3624f39-bbb1-451d-8161-8518e4108d8e@joelfernandes.org>
Date: Mon, 4 Mar 2024 11:19:21 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] rcutorture: Fix
 rcu_torture_pipe_update_one()/rcu_torture_writer() data race and concurrency
 bug
To: linke li <lilinke99@qq.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>, "Paul E. McKenney"
 <paulmck@kernel.org>, 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
References: <tencent_B51A9DA220288A95A435E3435A0443BEB007@qq.com>
Content-Language: en-US
From: Joel Fernandes <joel@joelfernandes.org>
In-Reply-To: <tencent_B51A9DA220288A95A435E3435A0443BEB007@qq.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


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")
> 
>  {
>  	int i;
> 
> -	i = rp->rtort_pipe_count;
> +	i = READ_ONCE(rp->rtort_pipe_count);
>  	if (i > RCU_TORTURE_PIPE_LEN)
>  		i = RCU_TORTURE_PIPE_LEN;
>  	atomic_inc(&rcu_torture_wcount[i]);
> -	if (++rp->rtort_pipe_count >= RCU_TORTURE_PIPE_LEN) {
> +	WRITE_ONCE(rp->rtort_pipe_count, i + 1);
> +	if (rp->rtort_pipe_count >= RCU_TORTURE_PIPE_LEN) {
>  		rp->rtort_mbtest = 0;
>  		return true;
>  	}
> 
> 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.
> 
> Also, rp->rtort_pipe_count in the next line should be read using
> READ_ONCE() because of data race.
> 
> Signed-off-by: linke li <lilinke99@qq.com>
> ---
>  kernel/rcu/rcutorture.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 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 = RCU_TORTURE_PIPE_LEN;
>  	atomic_inc(&rcu_torture_wcount[i]);
> -	WRITE_ONCE(rp->rtort_pipe_count, i + 1);
> -	if (rp->rtort_pipe_count >= RCU_TORTURE_PIPE_LEN) {
> +	WRITE_ONCE(rp->rtort_pipe_count, rp->rtort_pipe_count + 1);
> +	if (READ_ONCE(rp->rtort_pipe_count) >= RCU_TORTURE_PIPE_LEN) {

I want to say, I am not convinced with the patch because what's wrong with
writing to an old index?

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".

Anyway, a slightly related/different question:

Should that:
WRITE_ONCE(rp->rtort_pipe_count, rp->rtort_pipe_count + 1);

Be:
WRITE_ONCE(rp->rtort_pipe_count, READ_ONCE(rp->rtort_pipe_count) + 1);

?

thanks,

 - Joel