[lvc-project] [PATCH v2] The value may overflow

[Paul E. McKenney]

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 at 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 at 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 at efficios.com>
> > > >
> > > > Thank you all!!!
> > > >
> > > > ------------------------------------------------------------------------
> > > >
> > > > commit 50477ff756ab99402b1523b7c6be8b5d790d05e7
> > > > Author: Denis Arefev <arefev at 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 at joelfernandes.org>

Thank you very much, and I will apply this on the next rebase.

							Thanx, Paul