rcutorture: Fix rcu_torture_pipe_update_one()/rcu_torture_writer() data race and concurrency bug
Message ID | tencent_B51A9DA220288A95A435E3435A0443BEB007@qq.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-90469-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7301:fa17:b0:10a:f01:a869 with SMTP id ju23csp1348599dyc; Mon, 4 Mar 2024 03:09:43 -0800 (PST) X-Forwarded-Encrypted: i=3; AJvYcCUFGyQC8Bw165iy+3+vd4gXFCLVmGbxkfi8lrT6pqZ7nmS8dH/Ve7IZkq8j77ECPgqFs1zZCZ1ZxQJuE3VX7vfqrPp40g== X-Google-Smtp-Source: AGHT+IFEuLMlNO3qXEU3gEINWsGtaeP1jdscCvScb96RY6CqasqgteGnbIhja1DLAJMb9IsOPkCh X-Received: by 2002:a05:6402:5114:b0:566:28ae:55d4 with SMTP id m20-20020a056402511400b0056628ae55d4mr7506818edd.39.1709550583133; Mon, 04 Mar 2024 03:09:43 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1709550583; cv=pass; d=google.com; s=arc-20160816; b=0zHFDGzwaY2ET3iHED8+ElQ2YKsdgj/bgIM8dk6KQ4UcUgCgt/ZvFDIYBszKT5rAWO xUuY/ZZ0YX/VKTw/jNQw80EC4XEgQz5hEyTJo1VimjxgVizsOS0yVU4Lh5T8tg4bF2Z5 A++Uv/tudhPLS/057WGZcQLGwAIqk7F88u4XsbpxDbdElLNCMTDUBdLbrSM584cgsd8l Hi5fR8+rDDG7NdlXx2esOLwGf+jzbu+4DETr+ytgx9drHXitDjVNFscZR2Ri76bwvixe EwCHikN5i7Ig9Bf0fjAhd1Hiqhmww8Qw/eOu3YBsZSdKy2O5uhVP4hpRtMgK3q2erR5d yb+w== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:date:subject:cc:to:from :message-id:dkim-signature; bh=sJKxYdRSOaImCb3LknsmQ+6dfHzPm1FfnhFjMeeoBU4=; fh=Z86ek6JfHwbpgQq2M/cdKWDbMk7xcRybPyu5HdLc0mo=; b=0YixTpeIrPlqHTek3fvZKoYBz7MOtjgdlPInW6wtZudmeECTRuD57HFE0+T0/cxpvg X5NevoRuf/Ig3yBleNgd1NrrvLfU1MhNbCek/SRJVfb0VSFHnoEURs01Z0zL65YQomXA tR0puyctQP/F/D1hrsE7McQ3zp0t0zvGjtvOXdNQas2DO85swH6o3QPOKy/pxC84825A yExykvX0Keu/1DkArd/W81kwsBb7m0OgxPl6UESv04hBn1W7Yg4WKlW26DhU42sia326 ImBOUEvpS94M0015z/XyZ03VoSGNc3VsWDs81RtbQtqs/8zHhVgbookCn1TDhnV5I/wr KfYg==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@qq.com header.s=s201512 header.b=hcoYEjrq; arc=pass (i=1 spf=pass spfdomain=qq.com dkim=pass dkdomain=qq.com dmarc=pass fromdomain=qq.com); spf=pass (google.com: domain of linux-kernel+bounces-90469-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) smtp.mailfrom="linux-kernel+bounces-90469-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=qq.com Received: from am.mirrors.kernel.org (am.mirrors.kernel.org. [2604:1380:4601:e00::3]) by mx.google.com with ESMTPS id m8-20020a509988000000b0056752f42d9bsi650726edb.269.2024.03.04.03.09.42 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 04 Mar 2024 03:09:43 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-90469-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3; Authentication-Results: mx.google.com; dkim=pass header.i=@qq.com header.s=s201512 header.b=hcoYEjrq; arc=pass (i=1 spf=pass spfdomain=qq.com dkim=pass dkdomain=qq.com dmarc=pass fromdomain=qq.com); spf=pass (google.com: domain of linux-kernel+bounces-90469-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) smtp.mailfrom="linux-kernel+bounces-90469-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=qq.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by am.mirrors.kernel.org (Postfix) with ESMTPS id 3207F1F275F7 for <ouuuleilei@gmail.com>; Mon, 4 Mar 2024 11:01:26 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id C242B38DF5; Mon, 4 Mar 2024 11:01:10 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=qq.com header.i=@qq.com header.b="hcoYEjrq" Received: from out162-62-58-211.mail.qq.com (out162-62-58-211.mail.qq.com [162.62.58.211]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CBFCA1B814; Mon, 4 Mar 2024 11:01:04 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=162.62.58.211 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709550068; cv=none; b=BmdtvKPxdig2wWl+x6u6+0YPwvvg/XqJCbt0Y0DH+0veEdnfTtVamiJldjRPK8w2iR3SWZ+k/VsTDzO6nhU0+ft3MyFR6amPiB8ws2zp68P4Y9HAgCdgFdArAzo8HwA0/o1DmJ+QSZbtfsKXyJ4RPqa68jA79lBVm74hzstUTBw= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709550068; c=relaxed/simple; bh=/tWTG1VRLQN+1qsichV/pE5kKWXw53KAsrYYtmgyIu8=; h=Message-ID:From:To:Cc:Subject:Date:MIME-Version; b=TeLAYNDZQDnPek7iifc7UtiM+dYDMrJrImFk7cddHwJ3suX6uQ/j0KOM6K8W0s8RfSe/lfTtpa7fNKQXToARaoPRrcwIfZD9KqAuyrzOgPHFbltd08lmP83zz+Gh0zymJAXpZhG5gB/fsSep5/IPfv3s7XQJIo1TkmuqDrfZsvc= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=qq.com; spf=pass smtp.mailfrom=qq.com; dkim=pass (1024-bit key) header.d=qq.com header.i=@qq.com header.b=hcoYEjrq; arc=none smtp.client-ip=162.62.58.211 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=qq.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=qq.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=qq.com; s=s201512; t=1709550054; bh=sJKxYdRSOaImCb3LknsmQ+6dfHzPm1FfnhFjMeeoBU4=; h=From:To:Cc:Subject:Date; b=hcoYEjrqrLhGZDKii23tQJngBSKkcCu+TYXLupgh/TkuvNQJyzgwypZ9cKK5x7DB6 K0xKh4Sd5KgZ/0gEkmw+Ky1znPcnk8G/xYLjUdSZZraQC2ruCTRo3gIlEUxyeFjviJ HbtxZblshINwJKzVtYxSwDxOi+48pi2mIMicH+YI= Received: from localhost.localdomain ([218.94.142.74]) by newxmesmtplogicsvrszb9-1.qq.com (NewEsmtp) with SMTP id DB02C6D3; Mon, 04 Mar 2024 18:54:48 +0800 X-QQ-mid: xmsmtpt1709549688tysp3hq2h Message-ID: <tencent_B51A9DA220288A95A435E3435A0443BEB007@qq.com> X-QQ-XMAILINFO: MIOa0ndEC0Bb0Dw3+QvYIBQLmDa4FNQ57dvlB9X9JLGoLNdLUi505MF+0f+BDP Q4drGGSIWkoY21yqYMKwkkxokup1iuVasFKR7+rI3kzSOoQk4Ko55sWSmK0gwXmTUb9sLF/Urbuo fV/+A81Dm9M33fGp48FuQYqj9llnJGFNH1ZnO6gcPxeeCwyG1GAdRygfvv00qb6XOyNggcnsG11A QI9vJi8TMz4KDAV1593eSTHwl23Hmbq4k+w3bVB58dJmrJ7Z0hL87uVL9feN2qvkBU/9sK3UXzNj ZTyOZNKY0otTeHigEevx4Oqn7G/X76fsgmr6pCtaLv0qEeZpEHKirhb1w99/eFSug+b1h97kfmEq mdmYicKMi5gOPXUOCbQ+Isl+mxz8Ag2cda2t2PX2meHF6jghSeowOXVIMDw1ZurCSYy9f6v4RX7N Q7omWZbghgwwjmeGLWRmHctilJWDyjBB+m3djLkBtHeF2zZ3emeFVTiXAljXJcBgna3+PCwIMjrs llEPSdt6M2FYWugvcaG1DwwHWZ0bsx+jxfsK1KDBQK/PeFAraLM1QiHldrItgo2o8r7rdVoAbZkW lQKbqLHtwnEBQcRmzaYX1Gj+bhS/GBmYGQNQwA4HXAHucZT28KxLt/vc9w9NRoAGW4GNN19Rw/C2 qEC/DI/ebX81cfSHy2vOk4AHXBvTaEnKsrb3EwyOrkq3hZ47q1jEBQ8FanlHkIE9ItT/nGGMnSKN h3lJ6BcspWjDH7QnojL7XT9XlsI1tth8H7TnxU5BNHGd/kK76wKfk5FuarxhruU7sv5H7mLK/1nW 8QKy67x+JGdJKWDsGAlZJ8sS39m3pdf5Aas1FxvQVYVhZb9knfrtgEFMgypC7SvCVBMyglL2ZYtf GOplBsrGpK9Gse1NHN1MOyMeArddh6R1Yi3Sg9CJ2vGyGJqCb32UsV5Rddl+zW3AJjadWXL2V3O/ mtbNlIdH5RfcIkwaP4+awTZsP8RNMzQ1qYkZn9z0tqRJGh8q/a+buujsQ8m25/p1Qvgn7hWDLQsi PNnAoQVb9JvVfSRnURUOheiIjFioIr/IkioegauA== X-QQ-XMRINFO: NyFYKkN4Ny6FSmKK/uo/jdU= From: linke li <lilinke99@qq.com> To: Cc: lilinke99@qq.com, 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>, Joel Fernandes <joel@joelfernandes.org>, 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 Subject: [PATCH] rcutorture: Fix rcu_torture_pipe_update_one()/rcu_torture_writer() data race and concurrency bug Date: Mon, 4 Mar 2024 18:54:46 +0800 X-OQ-MSGID: <20240304105446.35673-1-lilinke99@qq.com> X-Mailer: git-send-email 2.39.3 (Apple Git-145) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1792593711995547358 X-GMAIL-MSGID: 1792593711995547358 |
Series |
rcutorture: Fix rcu_torture_pipe_update_one()/rcu_torture_writer() data race and concurrency bug
|
|
Commit Message
linke li
March 4, 2024, 10:54 a.m. UTC
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(-)
Comments
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
On Mon, Mar 04, 2024 at 11:19:21AM -0500, Joel Fernandes wrote: > > > 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); > > ? Thank you both! At first glance, I would argue for something like this: ------------------------------------------------------------------------ static bool rcu_torture_pipe_update_one(struct rcu_torture *rp) { int i; struct rcu_torture_reader_check *rtrcp = READ_ONCE(rp->rtort_chkp); if (rtrcp) { WRITE_ONCE(rp->rtort_chkp, NULL); smp_store_release(&rtrcp->rtc_ready, 1); // Pair with smp_load_acquire(). } i = READ_ONCE(rp->rtort_pipe_count) + 1; if (i > RCU_TORTURE_PIPE_LEN) i = RCU_TORTURE_PIPE_LEN; atomic_inc(&rcu_torture_wcount[i]); WRITE_ONCE(rp->rtort_pipe_count, i); if (i >= RCU_TORTURE_PIPE_LEN) { rp->rtort_mbtest = 0; return true; } return false; } ------------------------------------------------------------------------ That is, move the increment to the read and replace the re-read with the value "i" that was just written. Thoughts? Thanx, Paul
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) { rp->rtort_mbtest = 0; return true; }