From: "Paul E. McKenney" <paulmck@kernel.org>
To: Joel Fernandes <joel@joelfernandes.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
David Laight <David.Laight@aculab.com>,
Denis Arefev <arefev@swemel.ru>,
Lai Jiangshan <jiangshanlai@gmail.com>,
Josh Triplett <josh@joshtriplett.org>,
Steven Rostedt <rostedt@goodmis.org>,
"rcu@vger.kernel.org" <rcu@vger.kernel.org>,
"lvc-project@linuxtesting.org" <lvc-project@linuxtesting.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"trufanov@swemel.ru" <trufanov@swemel.ru>,
"vfh@swemel.ru" <vfh@swemel.ru>,
ldufour@linux.ibm.com
Subject: Re: [PATCH v2] The value may overflow
Date: Tue, 5 Sep 2023 15:22:28 -0700 [thread overview]
Message-ID: <285f9ee6-2f45-4b5a-8cd8-92b6f3b017a7@paulmck-laptop> (raw)
In-Reply-To: <CAEXW_YQW_0RD5mmtwnzwik9Q9K9XL9rxHUyb98SLqCJ-840xOQ@mail.gmail.com>
On Tue, Sep 05, 2023 at 04:56:12PM -0400, Joel Fernandes wrote:
> On Tue, Sep 5, 2023 at 4:52 PM Paul E. McKenney <paulmck@kernel.org> wrote:
> >
> > On Tue, Sep 05, 2023 at 04:40:46PM -0400, Joel Fernandes wrote:
> [...]
> > > > > > > So this bug appears to have no effect for default RCU setups, even on very
> > > > > > > large 64-bit systems, which is consistent with field experience. Even if
> > > > > > > this is the case, it still should be fixed, to avoid confusion if nothing
> > > > > > > else. Or just in case someone decides to set CONFIG_RCU_FANOUT_LEAF=32.
> > > > > > > Which actually happened the other day due to someone trusting ChatGPT's
> > > > > > > opinion about RCU Kconfig options...
> > > > > >
> > > > > > And I therefore queued Denis's v3 patch with an edited commit log.
> > > > > > Of course, if anyone sees some other way that the bug could manifest
> > > > > > other than in a 64-bit kernel built with CONFIG_RCU_FANOUT_LEAF greater
> > > > > > than 30 on a system with at least 31 CPUs, please let me know so that
> > > > > > I can adjust.
> > > > > >
> > > > > > Thanx, Paul
> > > > > >
> > > > > > ------------------------------------------------------------------------
> > > > > >
> > > > > > commit ed083b0e22f1396dee3599896249a3f218845298
> > > > > > Author: Denis Arefev <arefev@swemel.ru>
> > > > > > Date: Mon Sep 4 15:21:14 2023 +0300
> > > > > >
> > > > > > Fix srcu_struct node grpmask overflow on 64-bit systems
> > > > > > The value of an arithmetic expression 1 << (cpu - sdp->mynode->grplo)
> > > > >
> > > > > AFAIU, the overflow resides in the "bitwise expression" and not
> > > > > the arithmetic expression.
> > > >
> > > > Rather than quibble about exactly what constitutes arithmetic, I
> > > > updated the first paragraph and added your Reviewed-by as shown
> > > > below. ;-)
> > > >
> > > > > Other than this, please add my
> > > > >
> > > > > Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > >
> > > > Thank you all!!!
> > > >
> > > > ------------------------------------------------------------------------
> > > >
> > > > commit 50477ff756ab99402b1523b7c6be8b5d790d05e7
> > > > Author: Denis Arefev <arefev@swemel.ru>
> > > > Date: Mon Sep 4 15:21:14 2023 +0300
> > > >
> > > > Fix srcu_struct node grpmask overflow on 64-bit systems
> > > >
> > > > The value of a bitwise expression 1 << (cpu - sdp->mynode->grplo)
> > > > is subject to overflow due to a failure to cast operands to a larger
> > > > data type before performing the bitwise operation.
> > > >
> > > > The maximum result of this subtraction is defined by the RCU_FANOUT_LEAF
> > > > Kconfig option, which on 64-bit systems defaults to 16 (resulting in a
> > > > maximum shift of 15), but which can be set up as high as 64 (resulting
> > > > in a maximum shift of 63). A value of 31 can result in sign extension,
> > > > resulting in 0xffffffff80000000 instead of the desired 0x80000000.
> > > > A value of 31 or greater triggers undefined behavior per the C standard.
> > >
> > > Do you mean here "A value of 32 or greater"?
> > >
> > > Only N >= 32 throws warning for:
> > > unsigned long foo = (1 << N);
> > >
> > > N=31 does undesirable sign extension but no warning.
> >
> > Good catch, thank you, and I will update this on my next rebase.
>
> Thanks, and with that the patch looks good to me:
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Thank you very much, and I will apply this on the next rebase.
Thanx, Paul
prev parent reply other threads:[~2023-09-05 22:22 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-04 9:42 [PATCH v2] The value may overflow Denis Arefev
2023-09-04 10:24 ` Mathieu Desnoyers
2023-09-05 9:31 ` David Laight
2023-09-05 12:26 ` Mathieu Desnoyers
2023-09-05 13:26 ` Paul E. McKenney
2023-09-05 13:34 ` Mathieu Desnoyers
2023-09-05 13:56 ` Paul E. McKenney
2023-09-05 14:15 ` David Laight
2023-09-05 14:34 ` Mathieu Desnoyers
2023-09-05 16:40 ` Paul E. McKenney
2023-09-05 19:27 ` Paul E. McKenney
2023-09-05 19:36 ` Mathieu Desnoyers
2023-09-05 19:45 ` Paul E. McKenney
2023-09-05 20:40 ` Joel Fernandes
2023-09-05 20:52 ` Paul E. McKenney
2023-09-05 20:56 ` Joel Fernandes
2023-09-05 22:22 ` Paul E. McKenney [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=285f9ee6-2f45-4b5a-8cd8-92b6f3b017a7@paulmck-laptop \
--to=paulmck@kernel.org \
--cc=David.Laight@aculab.com \
--cc=arefev@swemel.ru \
--cc=jiangshanlai@gmail.com \
--cc=joel@joelfernandes.org \
--cc=josh@joshtriplett.org \
--cc=ldufour@linux.ibm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=lvc-project@linuxtesting.org \
--cc=mathieu.desnoyers@efficios.com \
--cc=rcu@vger.kernel.org \
--cc=rostedt@goodmis.org \
--cc=trufanov@swemel.ru \
--cc=vfh@swemel.ru \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).