From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id C18B7C77B7A for ; Tue, 23 May 2023 04:22:05 +0000 (UTC) Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4QQLkN1ks1z3f6g for ; Tue, 23 May 2023 14:22:04 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=ibm.com header.i=@ibm.com header.a=rsa-sha256 header.s=pp1 header.b=bQwLuF65; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=linux.ibm.com (client-ip=148.163.156.1; helo=mx0a-001b2d01.pphosted.com; envelope-from=rmclure@linux.ibm.com; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=ibm.com header.i=@ibm.com header.a=rsa-sha256 header.s=pp1 header.b=bQwLuF65; dkim-atps=neutral Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4QQFYc1vN2z3bgs for ; Tue, 23 May 2023 10:29:07 +1000 (AEST) Received: from pps.filterd (m0353729.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 34N06QDI030404; Tue, 23 May 2023 00:28:56 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=from : message-id : content-type : mime-version : subject : date : in-reply-to : cc : to : references; s=pp1; bh=zxyuYxBUuKgz0Sr4csNLIOjU+M8a1l9lBA+JIWN9Mds=; b=bQwLuF654Bac6+VAwWbIGMsW0qi5rH7GEovHTT8y5IszzNHYYJsBKUYI6y1TtfCKZwLK OzKeeY/cFlKSBqQdILJNdZF5ZcaouAAp231aWKCH9mrMfEAvlJHCOt8rphLDS3UA96R6 XLsjl5Y/rmSV6o/YR49qjJcKr6uJtrXyeyCzyj01l85CMp7btcNkIVV+mAYGlZCxVLuz o34iUlCP2kMTBcBH9LfOQvTjK1yAaLOqiQIObaUPYm61ALMvVkYyEsHvajxbUhe7Fvex hCH22QElfZKAOVaWQ59ASwiTzMh6ZVRIMe+AxKLIAoZL+Th0GacM/U1qlrHRYyHTUdYq Jw== Received: from ppma04fra.de.ibm.com (6a.4a.5195.ip4.static.sl-reverse.com [149.81.74.106]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3qrjfv8nb3-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 23 May 2023 00:28:55 +0000 Received: from pps.filterd (ppma04fra.de.ibm.com [127.0.0.1]) by ppma04fra.de.ibm.com (8.17.1.19/8.17.1.19) with ESMTP id 34MNXagd015012; Tue, 23 May 2023 00:28:52 GMT Received: from smtprelay01.fra02v.mail.ibm.com ([9.218.2.227]) by ppma04fra.de.ibm.com (PPS) with ESMTPS id 3qppcf10df-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 23 May 2023 00:28:52 +0000 Received: from smtpav06.fra02v.mail.ibm.com (smtpav06.fra02v.mail.ibm.com [10.20.54.105]) by smtprelay01.fra02v.mail.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 34N0SoLM18875076 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 23 May 2023 00:28:50 GMT Received: from smtpav06.fra02v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 6FBDC20043; Tue, 23 May 2023 00:28:50 +0000 (GMT) Received: from smtpav06.fra02v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 825DA20040; Tue, 23 May 2023 00:28:49 +0000 (GMT) Received: from ozlabs.au.ibm.com (unknown [9.192.253.14]) by smtpav06.fra02v.mail.ibm.com (Postfix) with ESMTP; Tue, 23 May 2023 00:28:49 +0000 (GMT) Received: from smtpclient.apple (haven.au.ibm.com [9.192.254.114]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.au.ibm.com (Postfix) with ESMTPSA id B1FB9600BB; Tue, 23 May 2023 10:28:45 +1000 (AEST) From: Rohan McLure Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_7B8A09EB-50F3-4FA0-942C-27AD7E6B05F1" Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.500.231\)) Subject: Re: [PATCH v2 03/11] asm-generic/mmiowb: Mark accesses to fix KCSAN warnings Date: Tue, 23 May 2023 10:28:35 +1000 In-Reply-To: <20230510033117.1395895-4-rmclure@linux.ibm.com> To: Rohan McLure , linuxppc-dev References: <20230510033117.1395895-1-rmclure@linux.ibm.com> <20230510033117.1395895-4-rmclure@linux.ibm.com> X-Mailer: Apple Mail (2.3731.500.231) X-TM-AS-GCONF: 00 X-Proofpoint-GUID: RZrtdWdTf1IZDyvluuabf6-uWab24mbk X-Proofpoint-ORIG-GUID: RZrtdWdTf1IZDyvluuabf6-uWab24mbk X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.573,FMLib:17.11.176.26 definitions=2023-05-22_18,2023-05-22_03,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 mlxlogscore=999 impostorscore=0 spamscore=0 lowpriorityscore=0 adultscore=0 suspectscore=0 mlxscore=0 bulkscore=0 clxscore=1015 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2304280000 definitions=main-2305220203 X-Mailman-Approved-At: Tue, 23 May 2023 14:21:17 +1000 X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: gautam@linux.ibm.com, arnd@arndb.de Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" --Apple-Mail=_7B8A09EB-50F3-4FA0-942C-27AD7E6B05F1 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote: > Prior to this patch, data races are detectable by KCSAN of the = following > forms: >=20 > [1] Asynchronous calls to mmiowb_set_pending() from an interrupt = context > or otherwise outside of a critical section > [2] Interrupted critical sections, where the interrupt will itself > acquire a lock >=20 > In case [1], calling context does not need an mmiowb() call to be > issued, otherwise it would do so itself. Such calls to > mmiowb_set_pending() are either idempotent or no-ops. >=20 > In case [2], irrespective of when the interrupt occurs, the interrupt > will acquire and release its locks prior to its return, nesting_count > will continue balanced. In the worst case, the interrupted critical > section during a mmiowb_spin_unlock() call observes an mmiowb to be > pending and afterward is interrupted, leading to an extraneous call to > mmiowb(). This data race is clearly innocuous. >=20 > Mark all potentially asynchronous memory accesses with READ_ONCE or > WRITE_ONCE, including increments and decrements to nesting_count. This > has the effect of removing KCSAN warnings at consumer's callsites. >=20 > Signed-off-by: Rohan McLure > Reported-by: Michael Ellerman > Reported-by: Gautam Menghani > Tested-by: Gautam Menghani > Acked-by: Arnd Bergmann > --- > v2: Remove extraneous READ_ONCE in mmiowb_set_pending for = nesting_count > --- > include/asm-generic/mmiowb.h | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) >=20 > diff --git a/include/asm-generic/mmiowb.h = b/include/asm-generic/mmiowb.h > index 5698fca3bf56..6dea28c8835b 100644 > --- a/include/asm-generic/mmiowb.h > +++ b/include/asm-generic/mmiowb.h > @@ -37,25 +37,29 @@ static inline void mmiowb_set_pending(void) > struct mmiowb_state *ms =3D __mmiowb_state(); >=20 > if (likely(ms->nesting_count)) > - ms->mmiowb_pending =3D ms->nesting_count; > + WRITE_ONCE(ms->mmiowb_pending, ms->nesting_count); > } >=20 > static inline void mmiowb_spin_lock(void) > { > struct mmiowb_state *ms =3D __mmiowb_state(); > - ms->nesting_count++; > + > + /* Increment need not be atomic. Nestedness is balanced over = interrupts. */ > + WRITE_ONCE(ms->nesting_count, READ_ONCE(ms->nesting_count) + 1); > } >=20 > static inline void mmiowb_spin_unlock(void) > { > struct mmiowb_state *ms =3D __mmiowb_state(); > + u16 pending =3D READ_ONCE(ms->mmiowb_pending); >=20 > - if (unlikely(ms->mmiowb_pending)) { > - ms->mmiowb_pending =3D 0; > + WRITE_ONCE(ms->mmiowb_pending, 0); > + if (unlikely(pending)) { > mmiowb(); > } >=20 > - ms->nesting_count--; > + /* Decrement need not be atomic. Nestedness is balanced over = interrupts. */ > + WRITE_ONCE(ms->nesting_count, READ_ONCE(ms->nesting_count) - 1); Still think the nesting_counts don't need WRITE_ONCE/READ_ONCE. data_race() maybe but I don't know if it's even classed as a data race. How does KCSAN handle/annotate preempt_count, for example? Thanks, Nick= --Apple-Mail=_7B8A09EB-50F3-4FA0-942C-27AD7E6B05F1 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=us-ascii On Wed May 10, 2023 at = 1:31 PM AEST, Rohan McLure wrote:
Prior to this patch, data races are detectable = by KCSAN of the following
forms:

[1] Asynchronous calls to = mmiowb_set_pending() from an interrupt context
   or = otherwise outside of a critical section
[2] Interrupted critical = sections, where the interrupt will itself
   acquire a = lock

In case [1], calling context does not need an mmiowb() call = to be
issued, otherwise it would do so itself. Such calls = to
mmiowb_set_pending() are either idempotent or no-ops.

In = case [2], irrespective of when the interrupt occurs, the = interrupt
will acquire and release its locks prior to its return, = nesting_count
will continue balanced. In the worst case, the = interrupted critical
section during a mmiowb_spin_unlock() call = observes an mmiowb to be
pending and afterward is interrupted, = leading to an extraneous call to
mmiowb(). This data race is clearly = innocuous.

Mark all potentially asynchronous memory accesses with = READ_ONCE or
WRITE_ONCE, including increments and decrements to = nesting_count. This
has the effect of removing KCSAN warnings at = consumer's callsites.

Signed-off-by: Rohan McLure = <rmclure@linux.ibm.com>
Reported-by: Michael Ellerman = <mpe@ellerman.id.au>
Reported-by: Gautam Menghani = <gautam@linux.ibm.com>
Tested-by: Gautam Menghani = <gautam@linux.ibm.com>
Acked-by: Arnd Bergmann = <arnd@arndb.de>
---
v2: Remove extraneous READ_ONCE in = mmiowb_set_pending for = nesting_count
---
include/asm-generic/mmiowb.h | 14 = +++++++++-----
1 file changed, 9 insertions(+), 5 = deletions(-)

diff --git a/include/asm-generic/mmiowb.h = b/include/asm-generic/mmiowb.h
index 5698fca3bf56..6dea28c8835b = 100644
--- a/include/asm-generic/mmiowb.h
+++ = b/include/asm-generic/mmiowb.h
@@ -37,25 +37,29 @@ static inline void = mmiowb_set_pending(void)
struct mmiowb_state *ms =3D = __mmiowb_state();

if = (likely(ms->nesting_count))
- ms->mmiowb_pending =3D = ms->nesting_count;
+ WRITE_ONCE(ms->mmiowb_pending, = ms->nesting_count);
}

static inline void = mmiowb_spin_lock(void)
{
struct mmiowb_state *ms =3D = __mmiowb_state();
- ms->nesting_count++;
+
+ /* = Increment need not be atomic. Nestedness is balanced over interrupts. = */
+ = WRITE_ONCE(ms->nesting_count, READ_ONCE(ms->nesting_count) = + 1);
}

static inline void = mmiowb_spin_unlock(void)
{
struct mmiowb_state *ms =3D = __mmiowb_state();
+ u16 pending =3D = READ_ONCE(ms->mmiowb_pending);

- if = (unlikely(ms->mmiowb_pending)) {
- ms->mmiowb_pending =3D = 0;
+ = WRITE_ONCE(ms->mmiowb_pending, 0);
+ if = (unlikely(pending)) {
mmiowb();
= }

- ms->nesting_count--;
+ /* Decrement need not be atomic. = Nestedness is balanced over interrupts. */
+ = WRITE_ONCE(ms->nesting_count, READ_ONCE(ms->nesting_count) = - 1);

Still = think the nesting_counts don't need WRITE_ONCE/READ_ONCE.
data_race() maybe but I don't know if = it's even classed as a data
race. How does KCSAN handle/annotate = preempt_count, for example?

Thanks,
Nick= --Apple-Mail=_7B8A09EB-50F3-4FA0-942C-27AD7E6B05F1--