Message ID | 20230503203236.1587590-1-mathieu.desnoyers@efficios.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp1598652vqo; Wed, 3 May 2023 13:45:13 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ5dx342sSqq3Ea20UaAcAB13pbYBJNqKm9ydqrD9LSGGegQAyFPQ/RQ2DfxLfXaslaTgryC X-Received: by 2002:a05:6a00:174a:b0:63b:54e4:871b with SMTP id j10-20020a056a00174a00b0063b54e4871bmr4541594pfc.8.1683146713215; Wed, 03 May 2023 13:45:13 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1683146713; cv=none; d=google.com; s=arc-20160816; b=XmvRkj8QEFjkpCmceu6UuHVBPjlnAzNudMwr2NBB2u8Qi8u95zv+VYyqBCrpaYYJUI Loz5PEE8f0HVxlgJBb8cLnOznruo1V+jC5oaecGEap9Ne6jqBQUiTarzampUd/0OwWIn uKDAy00x4S5J6KilTx5jwjCOWhJSttk57kc6AiziaLOTHXpjyKgB7gTb/eX/rQ123Ucl VpPEe2UyFn7K4RW3hqcp2xefORyYW5QWZh87JXZoFg47mONrxbfvQZ7QJ0clszFAq41V tF1pqBTpNLjWmiJR0eLvsmr/hxcQrbntH5HlaxNk1o/yd8yg4mCav05WpNGhBdVG6HD0 mpQQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=zhlVzz3iJZ2trbry6h7RrmdBmyEna9hpRb4LNAHBWTw=; b=ZWaMeeXMfpo3SSyA3ubebYt2DGR5ExotPMcNdwoQu7jEocrd7ErI7lHJ9/l5s0CvGU +eujRgL/ovdg1OjKTGkVf5OYDR7n6gBceadsYpmAygWrBNabinGa7vR5zilApqTzm8KW A5ynMhPCS6PnTriLEUIS1vvXrtNmiijFCdE/cq4QSiK1dJCJGciUrqBdtbGeykJibKmt 6XVLWvQCjzYaHWx600301Ygz+q4H8yVvS+20okhaR4BpsAOjQZnC9+nOsn1N/PeQhej9 VCdCmRx8cX0og7lWXZRhUqMTuJ4X3lu0tMsTqHqv5lsmuz643V+Ydr47MM0aVNA670PM B8Rw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@efficios.com header.s=smtpout1 header.b=no1YqwIX; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=efficios.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id f16-20020a63f110000000b0052161765a25si3806046pgi.288.2023.05.03.13.44.57; Wed, 03 May 2023 13:45:13 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@efficios.com header.s=smtpout1 header.b=no1YqwIX; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=efficios.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229738AbjECUcs (ORCPT <rfc822;lhua1029@gmail.com> + 99 others); Wed, 3 May 2023 16:32:48 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54180 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229441AbjECUcr (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 3 May 2023 16:32:47 -0400 Received: from smtpout.efficios.com (unknown [IPv6:2607:5300:203:b2ee::31e5]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 835E0198C for <linux-kernel@vger.kernel.org>; Wed, 3 May 2023 13:32:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=efficios.com; s=smtpout1; t=1683145963; bh=qNB6O5mVECCPuKgtGErPGROmbcclghgZAShV5yBxuaI=; h=From:To:Cc:Subject:Date:From; b=no1YqwIX/6oqGAIE4BX0MFBH9H00tUF8S1a7Rp3ST8/qqgk+RD0qNCU6dUWy7Up55 87HM522afz/NFXZUW+wWRlNHFRjoKu6wPtnzU8W79UU1ruLrptpAMrnXdurov8Afws FFpRaOOMVqHrc02dFUXt2s5GP+hlMfx04N+XtXde07EyDIH6yTeGgBnXFdjM7N/wc/ 55DXbnsj32SPXO9VdZdvQZimthqCMNyltnUfUgnUDBBCfb97PmVSZxxv9YUrX7MlBM 5QfRdpBQl/XfS1NbqH06nNOOfAcdy8e16/fhC+MM0JURh+MeIxJ4MEUVjW2cJqaOek 7akX+418JyjWg== Received: from localhost.localdomain (192-222-143-198.qc.cable.ebox.net [192.222.143.198]) by smtpout.efficios.com (Postfix) with ESMTPSA id 4QBTCb416Dz11fW; Wed, 3 May 2023 16:32:43 -0400 (EDT) From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> To: "Paul E. McKenney" <paulmck@kernel.org> Cc: linux-kernel@vger.kernel.org, Mathieu Desnoyers <mathieu.desnoyers@efficios.com>, Joel Fernandes <joel@joelfernandes.org>, Josh Triplett <josh@joshtriplett.org>, Boqun Feng <boqun.feng@gmail.com>, Steven Rostedt <rostedt@goodmis.org>, Lai Jiangshan <jiangshanlai@gmail.com>, Zqiang <qiang1.zhang@intel.com> Subject: [RFC PATCH] rcu: rcupdate.h: Add missing parentheses around macro pointer dereference Date: Wed, 3 May 2023 16:32:36 -0400 Message-Id: <20230503203236.1587590-1-mathieu.desnoyers@efficios.com> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-1.3 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RDNS_NONE,SPF_HELO_NONE, SPF_PASS,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1764907248161685410?= X-GMAIL-MSGID: =?utf-8?q?1764907248161685410?= |
Series |
[RFC] rcu: rcupdate.h: Add missing parentheses around macro pointer dereference
|
|
Commit Message
Mathieu Desnoyers
May 3, 2023, 8:32 p.m. UTC
linux/rcupdate.h macros use the *p parameter without parentheses, e.g.:
typeof(*p)
rather than
typeof(*(p))
The following test-case shows how it can generate confusion due to C
operator precedence being reversed compared to the expectations:
#define m(p) \
do { \
__typeof__(*p) v = 0; \
} while (0)
void fct(unsigned long long *p1)
{
m(p1 + 1); /* works */
m(1 + p1); /* broken */
}
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Joel Fernandes <joel@joelfernandes.org>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: Zqiang <qiang1.zhang@intel.com>
---
include/linux/rcupdate.h | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
Comments
On Wed, May 03, 2023 at 04:32:36PM -0400, Mathieu Desnoyers wrote: > linux/rcupdate.h macros use the *p parameter without parentheses, e.g.: > > typeof(*p) > > rather than > > typeof(*(p)) > > The following test-case shows how it can generate confusion due to C > operator precedence being reversed compared to the expectations: > > #define m(p) \ > do { \ > __typeof__(*p) v = 0; \ > } while (0) > > void fct(unsigned long long *p1) > { > m(p1 + 1); /* works */ > m(1 + p1); /* broken */ > } > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > Cc: "Paul E. McKenney" <paulmck@kernel.org> > Cc: Joel Fernandes <joel@joelfernandes.org> > Cc: Josh Triplett <josh@joshtriplett.org> > Cc: Boqun Feng <boqun.feng@gmail.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Lai Jiangshan <jiangshanlai@gmail.com> > Cc: Zqiang <qiang1.zhang@intel.com> Reviewed-by: Boqun Feng <boqun.feng@gmail.com> Regards, Boqun > --- > include/linux/rcupdate.h | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > index dcd2cf1e8326..1565012fa47f 100644 > --- a/include/linux/rcupdate.h > +++ b/include/linux/rcupdate.h > @@ -430,16 +430,16 @@ static inline void rcu_preempt_sleep_check(void) { } > > #ifdef __CHECKER__ > #define rcu_check_sparse(p, space) \ > - ((void)(((typeof(*p) space *)p) == p)) > + ((void)(((typeof(*(p)) space *)p) == p)) > #else /* #ifdef __CHECKER__ */ > #define rcu_check_sparse(p, space) > #endif /* #else #ifdef __CHECKER__ */ > > #define __unrcu_pointer(p, local) \ > ({ \ > - typeof(*p) *local = (typeof(*p) *__force)(p); \ > + typeof(*(p)) *local = (typeof(*(p)) *__force)(p); \ > rcu_check_sparse(p, __rcu); \ > - ((typeof(*p) __force __kernel *)(local)); \ > + ((typeof(*(p)) __force __kernel *)(local)); \ > }) > /** > * unrcu_pointer - mark a pointer as not being RCU protected > @@ -452,29 +452,29 @@ static inline void rcu_preempt_sleep_check(void) { } > > #define __rcu_access_pointer(p, local, space) \ > ({ \ > - typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \ > + typeof(*(p)) *local = (typeof(*(p)) *__force)READ_ONCE(p); \ > rcu_check_sparse(p, space); \ > - ((typeof(*p) __force __kernel *)(local)); \ > + ((typeof(*(p)) __force __kernel *)(local)); \ > }) > #define __rcu_dereference_check(p, local, c, space) \ > ({ \ > /* Dependency order vs. p above. */ \ > - typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \ > + typeof(*(p)) *local = (typeof(*(p)) *__force)READ_ONCE(p); \ > RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_check() usage"); \ > rcu_check_sparse(p, space); \ > - ((typeof(*p) __force __kernel *)(local)); \ > + ((typeof(*(p)) __force __kernel *)(local)); \ > }) > #define __rcu_dereference_protected(p, local, c, space) \ > ({ \ > RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_protected() usage"); \ > rcu_check_sparse(p, space); \ > - ((typeof(*p) __force __kernel *)(p)); \ > + ((typeof(*(p)) __force __kernel *)(p)); \ > }) > #define __rcu_dereference_raw(p, local) \ > ({ \ > /* Dependency order vs. p above. */ \ > typeof(p) local = READ_ONCE(p); \ > - ((typeof(*p) __force __kernel *)(local)); \ > + ((typeof(*(p)) __force __kernel *)(local)); \ > }) > #define rcu_dereference_raw(p) __rcu_dereference_raw(p, __UNIQUE_ID(rcu)) > > -- > 2.25.1 >
On Wed, 3 May 2023 16:32:36 -0400 Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > linux/rcupdate.h macros use the *p parameter without parentheses, e.g.: > > typeof(*p) > > rather than > > typeof(*(p)) > > The following test-case shows how it can generate confusion due to C > operator precedence being reversed compared to the expectations: > > #define m(p) \ > do { \ > __typeof__(*p) v = 0; \ > } while (0) > > void fct(unsigned long long *p1) > { > m(p1 + 1); /* works */ > m(1 + p1); /* broken */ > } > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > Cc: "Paul E. McKenney" <paulmck@kernel.org> > Cc: Joel Fernandes <joel@joelfernandes.org> > Cc: Josh Triplett <josh@joshtriplett.org> > Cc: Boqun Feng <boqun.feng@gmail.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Lai Jiangshan <jiangshanlai@gmail.com> > Cc: Zqiang <qiang1.zhang@intel.com> > --- > include/linux/rcupdate.h | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > index dcd2cf1e8326..1565012fa47f 100644 > --- a/include/linux/rcupdate.h > +++ b/include/linux/rcupdate.h > @@ -430,16 +430,16 @@ static inline void rcu_preempt_sleep_check(void) { } > > #ifdef __CHECKER__ > #define rcu_check_sparse(p, space) \ > - ((void)(((typeof(*p) space *)p) == p)) > + ((void)(((typeof(*(p)) space *)p) == p)) Hmm, should we have that be: ((void)(((typeof(*(p)) space *)(p)) == (p))) In case of the 1 + p1, which would end up as: ((void)(((typeof(*(1 + p1)) __rcu *)1 + p1 == 1 + p1; I don't know how that __rcu get's passed around via the + statement there, so it may be fine. May not even make sense to have that. But I like to error on more parenthesis. ;-) The rest looks fine. Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org> -- Steve > #else /* #ifdef __CHECKER__ */ > #define rcu_check_sparse(p, space) > #endif /* #else #ifdef __CHECKER__ */ > > #define __unrcu_pointer(p, local) \ > ({ \ > - typeof(*p) *local = (typeof(*p) *__force)(p); \ > + typeof(*(p)) *local = (typeof(*(p)) *__force)(p); \ > rcu_check_sparse(p, __rcu); \ > - ((typeof(*p) __force __kernel *)(local)); \ > + ((typeof(*(p)) __force __kernel *)(local)); \ > }) > /** > * unrcu_pointer - mark a pointer as not being RCU protected > @@ -452,29 +452,29 @@ static inline void rcu_preempt_sleep_check(void) { } > > #define __rcu_access_pointer(p, local, space) \ > ({ \ > - typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \ > + typeof(*(p)) *local = (typeof(*(p)) *__force)READ_ONCE(p); \ > rcu_check_sparse(p, space); \ > - ((typeof(*p) __force __kernel *)(local)); \ > + ((typeof(*(p)) __force __kernel *)(local)); \ > }) > #define __rcu_dereference_check(p, local, c, space) \ > ({ \ > /* Dependency order vs. p above. */ \ > - typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \ > + typeof(*(p)) *local = (typeof(*(p)) *__force)READ_ONCE(p); \ > RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_check() usage"); \ > rcu_check_sparse(p, space); \ > - ((typeof(*p) __force __kernel *)(local)); \ > + ((typeof(*(p)) __force __kernel *)(local)); \ > }) > #define __rcu_dereference_protected(p, local, c, space) \ > ({ \ > RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_protected() usage"); \ > rcu_check_sparse(p, space); \ > - ((typeof(*p) __force __kernel *)(p)); \ > + ((typeof(*(p)) __force __kernel *)(p)); \ > }) > #define __rcu_dereference_raw(p, local) \ > ({ \ > /* Dependency order vs. p above. */ \ > typeof(p) local = READ_ONCE(p); \ > - ((typeof(*p) __force __kernel *)(local)); \ > + ((typeof(*(p)) __force __kernel *)(local)); \ > }) > #define rcu_dereference_raw(p) __rcu_dereference_raw(p, __UNIQUE_ID(rcu)) >
On Wed, May 3, 2023 at 4:32 PM Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > linux/rcupdate.h macros use the *p parameter without parentheses, e.g.: > > typeof(*p) > > rather than > > typeof(*(p)) > > The following test-case shows how it can generate confusion due to C > operator precedence being reversed compared to the expectations: > > #define m(p) \ > do { \ > __typeof__(*p) v = 0; \ > } while (0) > > void fct(unsigned long long *p1) > { > m(p1 + 1); /* works */ > m(1 + p1); /* broken */ > } > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > Cc: "Paul E. McKenney" <paulmck@kernel.org> > Cc: Joel Fernandes <joel@joelfernandes.org> > Cc: Josh Triplett <josh@joshtriplett.org> > Cc: Boqun Feng <boqun.feng@gmail.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Lai Jiangshan <jiangshanlai@gmail.com> > Cc: Zqiang <qiang1.zhang@intel.com> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org> thanks, - Joel > --- > include/linux/rcupdate.h | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > index dcd2cf1e8326..1565012fa47f 100644 > --- a/include/linux/rcupdate.h > +++ b/include/linux/rcupdate.h > @@ -430,16 +430,16 @@ static inline void rcu_preempt_sleep_check(void) { } > > #ifdef __CHECKER__ > #define rcu_check_sparse(p, space) \ > - ((void)(((typeof(*p) space *)p) == p)) > + ((void)(((typeof(*(p)) space *)p) == p)) > #else /* #ifdef __CHECKER__ */ > #define rcu_check_sparse(p, space) > #endif /* #else #ifdef __CHECKER__ */ > > #define __unrcu_pointer(p, local) \ > ({ \ > - typeof(*p) *local = (typeof(*p) *__force)(p); \ > + typeof(*(p)) *local = (typeof(*(p)) *__force)(p); \ > rcu_check_sparse(p, __rcu); \ > - ((typeof(*p) __force __kernel *)(local)); \ > + ((typeof(*(p)) __force __kernel *)(local)); \ > }) > /** > * unrcu_pointer - mark a pointer as not being RCU protected > @@ -452,29 +452,29 @@ static inline void rcu_preempt_sleep_check(void) { } > > #define __rcu_access_pointer(p, local, space) \ > ({ \ > - typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \ > + typeof(*(p)) *local = (typeof(*(p)) *__force)READ_ONCE(p); \ > rcu_check_sparse(p, space); \ > - ((typeof(*p) __force __kernel *)(local)); \ > + ((typeof(*(p)) __force __kernel *)(local)); \ > }) > #define __rcu_dereference_check(p, local, c, space) \ > ({ \ > /* Dependency order vs. p above. */ \ > - typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \ > + typeof(*(p)) *local = (typeof(*(p)) *__force)READ_ONCE(p); \ > RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_check() usage"); \ > rcu_check_sparse(p, space); \ > - ((typeof(*p) __force __kernel *)(local)); \ > + ((typeof(*(p)) __force __kernel *)(local)); \ > }) > #define __rcu_dereference_protected(p, local, c, space) \ > ({ \ > RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_protected() usage"); \ > rcu_check_sparse(p, space); \ > - ((typeof(*p) __force __kernel *)(p)); \ > + ((typeof(*(p)) __force __kernel *)(p)); \ > }) > #define __rcu_dereference_raw(p, local) \ > ({ \ > /* Dependency order vs. p above. */ \ > typeof(p) local = READ_ONCE(p); \ > - ((typeof(*p) __force __kernel *)(local)); \ > + ((typeof(*(p)) __force __kernel *)(local)); \ > }) > #define rcu_dereference_raw(p) __rcu_dereference_raw(p, __UNIQUE_ID(rcu)) > > -- > 2.25.1 >
On Wed, May 03, 2023 at 06:06:40PM -0400, Steven Rostedt wrote: > On Wed, 3 May 2023 16:32:36 -0400 > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > > linux/rcupdate.h macros use the *p parameter without parentheses, e.g.: > > > > typeof(*p) > > > > rather than > > > > typeof(*(p)) > > > > The following test-case shows how it can generate confusion due to C > > operator precedence being reversed compared to the expectations: > > > > #define m(p) \ > > do { \ > > __typeof__(*p) v = 0; \ > > } while (0) > > > > void fct(unsigned long long *p1) > > { > > m(p1 + 1); /* works */ > > m(1 + p1); /* broken */ > > } > > > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > > Cc: "Paul E. McKenney" <paulmck@kernel.org> > > Cc: Joel Fernandes <joel@joelfernandes.org> > > Cc: Josh Triplett <josh@joshtriplett.org> > > Cc: Boqun Feng <boqun.feng@gmail.com> > > Cc: Steven Rostedt <rostedt@goodmis.org> > > Cc: Lai Jiangshan <jiangshanlai@gmail.com> > > Cc: Zqiang <qiang1.zhang@intel.com> > > --- > > include/linux/rcupdate.h | 18 +++++++++--------- > > 1 file changed, 9 insertions(+), 9 deletions(-) > > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > > index dcd2cf1e8326..1565012fa47f 100644 > > --- a/include/linux/rcupdate.h > > +++ b/include/linux/rcupdate.h > > @@ -430,16 +430,16 @@ static inline void rcu_preempt_sleep_check(void) { } > > > > #ifdef __CHECKER__ > > #define rcu_check_sparse(p, space) \ > > - ((void)(((typeof(*p) space *)p) == p)) > > + ((void)(((typeof(*(p)) space *)p) == p)) > > Hmm, should we have that be: > ((void)(((typeof(*(p)) space *)(p)) == (p))) > > In case of the 1 + p1, which would end up as: > > ((void)(((typeof(*(1 + p1)) __rcu *)1 + p1 == 1 + p1; > > I don't know how that __rcu get's passed around via the + statement there, > so it may be fine. May not even make sense to have that. But I like to > error on more parenthesis. ;-) > > The rest looks fine. > > Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org> Thank you all! I applied Steve's suggested change with attribution as shown below. Please let me know if I messed anything up. Thanx, Paul ------------------------------------------------------------------------ commit d3d734216c88fb7c13205dc62178ff5011da415b Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Date: Wed May 3 16:32:36 2023 -0400 rcu: Add missing parentheses around rcu_dereference() "p" parameter linux/rcupdate.h macros use the *p parameter without parentheses, e.g.: typeof(*p) rather than typeof(*(p)) The following test-case shows how it can generate confusion due to C operator precedence being reversed compared to the expectations: #define m(p) \ do { \ __typeof__(*p) v = 0; \ } while (0) void fct(unsigned long long *p1) { m(p1 + 1); /* works */ m(1 + p1); /* broken */ } [ paulmck: Apply Steve Rostedt additional () feedback. ] Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Cc: "Paul E. McKenney" <paulmck@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Josh Triplett <josh@joshtriplett.org> Cc: Boqun Feng <boqun.feng@gmail.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Lai Jiangshan <jiangshanlai@gmail.com> Cc: Zqiang <qiang1.zhang@intel.com> Reviewed-by: Boqun Feng <boqun.feng@gmail.com> Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org> Signed-off-by: Paul E. McKenney <paulmck@kernel.org> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h index ddd42efc6224..cb938a89a923 100644 --- a/include/linux/rcupdate.h +++ b/include/linux/rcupdate.h @@ -405,16 +405,16 @@ static inline void rcu_preempt_sleep_check(void) { } #ifdef __CHECKER__ #define rcu_check_sparse(p, space) \ - ((void)(((typeof(*p) space *)p) == p)) + ((void)(((typeof(*(p)) space *)(p)) == (p))) #else /* #ifdef __CHECKER__ */ #define rcu_check_sparse(p, space) #endif /* #else #ifdef __CHECKER__ */ #define __unrcu_pointer(p, local) \ ({ \ - typeof(*p) *local = (typeof(*p) *__force)(p); \ + typeof(*(p)) *local = (typeof(*(p)) *__force)(p); \ rcu_check_sparse(p, __rcu); \ - ((typeof(*p) __force __kernel *)(local)); \ + ((typeof(*(p)) __force __kernel *)(local)); \ }) /** * unrcu_pointer - mark a pointer as not being RCU protected @@ -427,29 +427,29 @@ static inline void rcu_preempt_sleep_check(void) { } #define __rcu_access_pointer(p, local, space) \ ({ \ - typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \ + typeof(*(p)) *local = (typeof(*(p)) *__force)READ_ONCE(p); \ rcu_check_sparse(p, space); \ - ((typeof(*p) __force __kernel *)(local)); \ + ((typeof(*(p)) __force __kernel *)(local)); \ }) #define __rcu_dereference_check(p, local, c, space) \ ({ \ /* Dependency order vs. p above. */ \ - typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \ + typeof(*(p)) *local = (typeof(*(p)) *__force)READ_ONCE(p); \ RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_check() usage"); \ rcu_check_sparse(p, space); \ - ((typeof(*p) __force __kernel *)(local)); \ + ((typeof(*(p)) __force __kernel *)(local)); \ }) #define __rcu_dereference_protected(p, local, c, space) \ ({ \ RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_protected() usage"); \ rcu_check_sparse(p, space); \ - ((typeof(*p) __force __kernel *)(p)); \ + ((typeof(*(p)) __force __kernel *)(p)); \ }) #define __rcu_dereference_raw(p, local) \ ({ \ /* Dependency order vs. p above. */ \ typeof(p) local = READ_ONCE(p); \ - ((typeof(*p) __force __kernel *)(local)); \ + ((typeof(*(p)) __force __kernel *)(local)); \ }) #define rcu_dereference_raw(p) __rcu_dereference_raw(p, __UNIQUE_ID(rcu))
On 2023-05-04 20:28, Paul E. McKenney wrote: > On Wed, May 03, 2023 at 06:06:40PM -0400, Steven Rostedt wrote: >> On Wed, 3 May 2023 16:32:36 -0400 >> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: >> >>> linux/rcupdate.h macros use the *p parameter without parentheses, e.g.: >>> >>> typeof(*p) >>> >>> rather than >>> >>> typeof(*(p)) >>> >>> The following test-case shows how it can generate confusion due to C >>> operator precedence being reversed compared to the expectations: >>> >>> #define m(p) \ >>> do { \ >>> __typeof__(*p) v = 0; \ >>> } while (0) >>> >>> void fct(unsigned long long *p1) >>> { >>> m(p1 + 1); /* works */ >>> m(1 + p1); /* broken */ >>> } >>> >>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> >>> Cc: "Paul E. McKenney" <paulmck@kernel.org> >>> Cc: Joel Fernandes <joel@joelfernandes.org> >>> Cc: Josh Triplett <josh@joshtriplett.org> >>> Cc: Boqun Feng <boqun.feng@gmail.com> >>> Cc: Steven Rostedt <rostedt@goodmis.org> >>> Cc: Lai Jiangshan <jiangshanlai@gmail.com> >>> Cc: Zqiang <qiang1.zhang@intel.com> >>> --- >>> include/linux/rcupdate.h | 18 +++++++++--------- >>> 1 file changed, 9 insertions(+), 9 deletions(-) >>> >>> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h >>> index dcd2cf1e8326..1565012fa47f 100644 >>> --- a/include/linux/rcupdate.h >>> +++ b/include/linux/rcupdate.h >>> @@ -430,16 +430,16 @@ static inline void rcu_preempt_sleep_check(void) { } >>> >>> #ifdef __CHECKER__ >>> #define rcu_check_sparse(p, space) \ >>> - ((void)(((typeof(*p) space *)p) == p)) >>> + ((void)(((typeof(*(p)) space *)p) == p)) >> >> Hmm, should we have that be: >> ((void)(((typeof(*(p)) space *)(p)) == (p))) >> >> In case of the 1 + p1, which would end up as: >> >> ((void)(((typeof(*(1 + p1)) __rcu *)1 + p1 == 1 + p1; >> >> I don't know how that __rcu get's passed around via the + statement there, >> so it may be fine. May not even make sense to have that. But I like to >> error on more parenthesis. ;-) >> >> The rest looks fine. >> >> Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org> > > Thank you all! I applied Steve's suggested change with attribution > as shown below. Please let me know if I messed anything up. Hi Paul, I've done a new version of that patch which fixes other issues in rcupdate.h in the next round. Can you hold merging this until I remove the "RFC PATCH" tag please ? My goal is to gather feedback first to make sure everyone is OK with the overall changes across headers, so everything can become consistent. Thanks, Mathieu > > Thanx, Paul > > ------------------------------------------------------------------------ > > commit d3d734216c88fb7c13205dc62178ff5011da415b > Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > Date: Wed May 3 16:32:36 2023 -0400 > > rcu: Add missing parentheses around rcu_dereference() "p" parameter > > linux/rcupdate.h macros use the *p parameter without parentheses, e.g.: > > typeof(*p) > > rather than > > typeof(*(p)) > > The following test-case shows how it can generate confusion due to C > operator precedence being reversed compared to the expectations: > > #define m(p) \ > do { \ > __typeof__(*p) v = 0; \ > } while (0) > > void fct(unsigned long long *p1) > { > m(p1 + 1); /* works */ > m(1 + p1); /* broken */ > } > > [ paulmck: Apply Steve Rostedt additional () feedback. ] > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > Cc: "Paul E. McKenney" <paulmck@kernel.org> > Cc: Joel Fernandes <joel@joelfernandes.org> > Cc: Josh Triplett <josh@joshtriplett.org> > Cc: Boqun Feng <boqun.feng@gmail.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Lai Jiangshan <jiangshanlai@gmail.com> > Cc: Zqiang <qiang1.zhang@intel.com> > Reviewed-by: Boqun Feng <boqun.feng@gmail.com> > Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org> > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > index ddd42efc6224..cb938a89a923 100644 > --- a/include/linux/rcupdate.h > +++ b/include/linux/rcupdate.h > @@ -405,16 +405,16 @@ static inline void rcu_preempt_sleep_check(void) { } > > #ifdef __CHECKER__ > #define rcu_check_sparse(p, space) \ > - ((void)(((typeof(*p) space *)p) == p)) > + ((void)(((typeof(*(p)) space *)(p)) == (p))) > #else /* #ifdef __CHECKER__ */ > #define rcu_check_sparse(p, space) > #endif /* #else #ifdef __CHECKER__ */ > > #define __unrcu_pointer(p, local) \ > ({ \ > - typeof(*p) *local = (typeof(*p) *__force)(p); \ > + typeof(*(p)) *local = (typeof(*(p)) *__force)(p); \ > rcu_check_sparse(p, __rcu); \ > - ((typeof(*p) __force __kernel *)(local)); \ > + ((typeof(*(p)) __force __kernel *)(local)); \ > }) > /** > * unrcu_pointer - mark a pointer as not being RCU protected > @@ -427,29 +427,29 @@ static inline void rcu_preempt_sleep_check(void) { } > > #define __rcu_access_pointer(p, local, space) \ > ({ \ > - typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \ > + typeof(*(p)) *local = (typeof(*(p)) *__force)READ_ONCE(p); \ > rcu_check_sparse(p, space); \ > - ((typeof(*p) __force __kernel *)(local)); \ > + ((typeof(*(p)) __force __kernel *)(local)); \ > }) > #define __rcu_dereference_check(p, local, c, space) \ > ({ \ > /* Dependency order vs. p above. */ \ > - typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \ > + typeof(*(p)) *local = (typeof(*(p)) *__force)READ_ONCE(p); \ > RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_check() usage"); \ > rcu_check_sparse(p, space); \ > - ((typeof(*p) __force __kernel *)(local)); \ > + ((typeof(*(p)) __force __kernel *)(local)); \ > }) > #define __rcu_dereference_protected(p, local, c, space) \ > ({ \ > RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_protected() usage"); \ > rcu_check_sparse(p, space); \ > - ((typeof(*p) __force __kernel *)(p)); \ > + ((typeof(*(p)) __force __kernel *)(p)); \ > }) > #define __rcu_dereference_raw(p, local) \ > ({ \ > /* Dependency order vs. p above. */ \ > typeof(p) local = READ_ONCE(p); \ > - ((typeof(*p) __force __kernel *)(local)); \ > + ((typeof(*(p)) __force __kernel *)(local)); \ > }) > #define rcu_dereference_raw(p) __rcu_dereference_raw(p, __UNIQUE_ID(rcu)) >
On Fri, May 05, 2023 at 09:15:12AM -0400, Mathieu Desnoyers wrote: > On 2023-05-04 20:28, Paul E. McKenney wrote: > > On Wed, May 03, 2023 at 06:06:40PM -0400, Steven Rostedt wrote: > > > On Wed, 3 May 2023 16:32:36 -0400 > > > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > > > > > > linux/rcupdate.h macros use the *p parameter without parentheses, e.g.: > > > > > > > > typeof(*p) > > > > > > > > rather than > > > > > > > > typeof(*(p)) > > > > > > > > The following test-case shows how it can generate confusion due to C > > > > operator precedence being reversed compared to the expectations: > > > > > > > > #define m(p) \ > > > > do { \ > > > > __typeof__(*p) v = 0; \ > > > > } while (0) > > > > > > > > void fct(unsigned long long *p1) > > > > { > > > > m(p1 + 1); /* works */ > > > > m(1 + p1); /* broken */ > > > > } > > > > > > > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > > > > Cc: "Paul E. McKenney" <paulmck@kernel.org> > > > > Cc: Joel Fernandes <joel@joelfernandes.org> > > > > Cc: Josh Triplett <josh@joshtriplett.org> > > > > Cc: Boqun Feng <boqun.feng@gmail.com> > > > > Cc: Steven Rostedt <rostedt@goodmis.org> > > > > Cc: Lai Jiangshan <jiangshanlai@gmail.com> > > > > Cc: Zqiang <qiang1.zhang@intel.com> > > > > --- > > > > include/linux/rcupdate.h | 18 +++++++++--------- > > > > 1 file changed, 9 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > > > > index dcd2cf1e8326..1565012fa47f 100644 > > > > --- a/include/linux/rcupdate.h > > > > +++ b/include/linux/rcupdate.h > > > > @@ -430,16 +430,16 @@ static inline void rcu_preempt_sleep_check(void) { } > > > > #ifdef __CHECKER__ > > > > #define rcu_check_sparse(p, space) \ > > > > - ((void)(((typeof(*p) space *)p) == p)) > > > > + ((void)(((typeof(*(p)) space *)p) == p)) > > > > > > Hmm, should we have that be: > > > ((void)(((typeof(*(p)) space *)(p)) == (p))) > > > > > > In case of the 1 + p1, which would end up as: > > > > > > ((void)(((typeof(*(1 + p1)) __rcu *)1 + p1 == 1 + p1; > > > > > > I don't know how that __rcu get's passed around via the + statement there, > > > so it may be fine. May not even make sense to have that. But I like to > > > error on more parenthesis. ;-) > > > > > > The rest looks fine. > > > > > > Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org> > > > > Thank you all! I applied Steve's suggested change with attribution > > as shown below. Please let me know if I messed anything up. > > Hi Paul, > > I've done a new version of that patch which fixes other issues in rcupdate.h > in the next round. Can you hold merging this until I remove the "RFC PATCH" > tag please ? My goal is to gather feedback first to make sure everyone is OK > with the overall changes across headers, so everything can become > consistent. Hello, Mathieu, Very well, I have removed it. Thanx, Paul > Thanks, > > Mathieu > > > > > Thanx, Paul > > > > ------------------------------------------------------------------------ > > > > commit d3d734216c88fb7c13205dc62178ff5011da415b > > Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > > Date: Wed May 3 16:32:36 2023 -0400 > > > > rcu: Add missing parentheses around rcu_dereference() "p" parameter > > linux/rcupdate.h macros use the *p parameter without parentheses, e.g.: > > typeof(*p) > > rather than > > typeof(*(p)) > > The following test-case shows how it can generate confusion due to C > > operator precedence being reversed compared to the expectations: > > #define m(p) \ > > do { \ > > __typeof__(*p) v = 0; \ > > } while (0) > > void fct(unsigned long long *p1) > > { > > m(p1 + 1); /* works */ > > m(1 + p1); /* broken */ > > } > > [ paulmck: Apply Steve Rostedt additional () feedback. ] > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > > Cc: "Paul E. McKenney" <paulmck@kernel.org> > > Cc: Joel Fernandes <joel@joelfernandes.org> > > Cc: Josh Triplett <josh@joshtriplett.org> > > Cc: Boqun Feng <boqun.feng@gmail.com> > > Cc: Steven Rostedt <rostedt@goodmis.org> > > Cc: Lai Jiangshan <jiangshanlai@gmail.com> > > Cc: Zqiang <qiang1.zhang@intel.com> > > Reviewed-by: Boqun Feng <boqun.feng@gmail.com> > > Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org> > > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > > index ddd42efc6224..cb938a89a923 100644 > > --- a/include/linux/rcupdate.h > > +++ b/include/linux/rcupdate.h > > @@ -405,16 +405,16 @@ static inline void rcu_preempt_sleep_check(void) { } > > #ifdef __CHECKER__ > > #define rcu_check_sparse(p, space) \ > > - ((void)(((typeof(*p) space *)p) == p)) > > + ((void)(((typeof(*(p)) space *)(p)) == (p))) > > #else /* #ifdef __CHECKER__ */ > > #define rcu_check_sparse(p, space) > > #endif /* #else #ifdef __CHECKER__ */ > > #define __unrcu_pointer(p, local) \ > > ({ \ > > - typeof(*p) *local = (typeof(*p) *__force)(p); \ > > + typeof(*(p)) *local = (typeof(*(p)) *__force)(p); \ > > rcu_check_sparse(p, __rcu); \ > > - ((typeof(*p) __force __kernel *)(local)); \ > > + ((typeof(*(p)) __force __kernel *)(local)); \ > > }) > > /** > > * unrcu_pointer - mark a pointer as not being RCU protected > > @@ -427,29 +427,29 @@ static inline void rcu_preempt_sleep_check(void) { } > > #define __rcu_access_pointer(p, local, space) \ > > ({ \ > > - typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \ > > + typeof(*(p)) *local = (typeof(*(p)) *__force)READ_ONCE(p); \ > > rcu_check_sparse(p, space); \ > > - ((typeof(*p) __force __kernel *)(local)); \ > > + ((typeof(*(p)) __force __kernel *)(local)); \ > > }) > > #define __rcu_dereference_check(p, local, c, space) \ > > ({ \ > > /* Dependency order vs. p above. */ \ > > - typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \ > > + typeof(*(p)) *local = (typeof(*(p)) *__force)READ_ONCE(p); \ > > RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_check() usage"); \ > > rcu_check_sparse(p, space); \ > > - ((typeof(*p) __force __kernel *)(local)); \ > > + ((typeof(*(p)) __force __kernel *)(local)); \ > > }) > > #define __rcu_dereference_protected(p, local, c, space) \ > > ({ \ > > RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_protected() usage"); \ > > rcu_check_sparse(p, space); \ > > - ((typeof(*p) __force __kernel *)(p)); \ > > + ((typeof(*(p)) __force __kernel *)(p)); \ > > }) > > #define __rcu_dereference_raw(p, local) \ > > ({ \ > > /* Dependency order vs. p above. */ \ > > typeof(p) local = READ_ONCE(p); \ > > - ((typeof(*p) __force __kernel *)(local)); \ > > + ((typeof(*(p)) __force __kernel *)(local)); \ > > }) > > #define rcu_dereference_raw(p) __rcu_dereference_raw(p, __UNIQUE_ID(rcu)) > > -- > Mathieu Desnoyers > EfficiOS Inc. > https://www.efficios.com >
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h index dcd2cf1e8326..1565012fa47f 100644 --- a/include/linux/rcupdate.h +++ b/include/linux/rcupdate.h @@ -430,16 +430,16 @@ static inline void rcu_preempt_sleep_check(void) { } #ifdef __CHECKER__ #define rcu_check_sparse(p, space) \ - ((void)(((typeof(*p) space *)p) == p)) + ((void)(((typeof(*(p)) space *)p) == p)) #else /* #ifdef __CHECKER__ */ #define rcu_check_sparse(p, space) #endif /* #else #ifdef __CHECKER__ */ #define __unrcu_pointer(p, local) \ ({ \ - typeof(*p) *local = (typeof(*p) *__force)(p); \ + typeof(*(p)) *local = (typeof(*(p)) *__force)(p); \ rcu_check_sparse(p, __rcu); \ - ((typeof(*p) __force __kernel *)(local)); \ + ((typeof(*(p)) __force __kernel *)(local)); \ }) /** * unrcu_pointer - mark a pointer as not being RCU protected @@ -452,29 +452,29 @@ static inline void rcu_preempt_sleep_check(void) { } #define __rcu_access_pointer(p, local, space) \ ({ \ - typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \ + typeof(*(p)) *local = (typeof(*(p)) *__force)READ_ONCE(p); \ rcu_check_sparse(p, space); \ - ((typeof(*p) __force __kernel *)(local)); \ + ((typeof(*(p)) __force __kernel *)(local)); \ }) #define __rcu_dereference_check(p, local, c, space) \ ({ \ /* Dependency order vs. p above. */ \ - typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \ + typeof(*(p)) *local = (typeof(*(p)) *__force)READ_ONCE(p); \ RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_check() usage"); \ rcu_check_sparse(p, space); \ - ((typeof(*p) __force __kernel *)(local)); \ + ((typeof(*(p)) __force __kernel *)(local)); \ }) #define __rcu_dereference_protected(p, local, c, space) \ ({ \ RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_protected() usage"); \ rcu_check_sparse(p, space); \ - ((typeof(*p) __force __kernel *)(p)); \ + ((typeof(*(p)) __force __kernel *)(p)); \ }) #define __rcu_dereference_raw(p, local) \ ({ \ /* Dependency order vs. p above. */ \ typeof(p) local = READ_ONCE(p); \ - ((typeof(*p) __force __kernel *)(local)); \ + ((typeof(*(p)) __force __kernel *)(local)); \ }) #define rcu_dereference_raw(p) __rcu_dereference_raw(p, __UNIQUE_ID(rcu))