[v2] overflow: Match arithmetic in both parts of __builtin_choose_expr

[PATCH v2] overflow: Match arithmetic in both parts of __builtin_choose_expr

Posted by Bill Wendling 1 week, 3 days ago

The two parts of '__builtin_choose_expr()' had different arithmetic
expressions. This patch corrects it so that both parts calculate:

  COUNT * sizeof(*MEMBER) + __must_be_array(MEMBER)

In practice, '__must_be_array' is meant to assert if 'MEMBER' isn't an
array and return '0' otherwise. However, symmetry is less prone to
subtle errors (and far more satisfying).

Signed-off-by: Bill Wendling <morbo@google.com>
---
v2: Don't use 'size_add', which just adds more complicated macro cruft to the
    expression. Instead, just adjust the parentheses.
---
 include/linux/overflow.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index a8cb6319b4fb..d0f04436343e 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -418,7 +418,7 @@ static __always_inline size_t __must_check size_sub(size_t minuend, size_t subtr
 #define flex_array_size(p, member, count)				\
 	__builtin_choose_expr(__is_constexpr(count),			\
 		(count) * sizeof(*(p)->member) + __must_be_array((p)->member),	\
-		size_mul(count, sizeof(*(p)->member) + __must_be_array((p)->member)))
+		size_mul(count, sizeof(*(p)->member)) + __must_be_array((p)->member))
 
 /**
  * struct_size() - Calculate size of structure with trailing flexible array.
-- 
2.56.0.385.gd3acb90ef8-goog

Re: [PATCH v2] overflow: Match arithmetic in both parts of __builtin_choose_expr

Posted by Kees Cook 1 week, 2 days ago

On Tue, 13 Oct 2026 20:31:46 +0000, Bill Wendling wrote:
> The two parts of '__builtin_choose_expr()' had different arithmetic
> expressions. This patch corrects it so that both parts calculate:
> 
>   COUNT * sizeof(*MEMBER) + __must_be_array(MEMBER)
> 
> In practice, '__must_be_array' is meant to assert if 'MEMBER' isn't an
> array and return '0' otherwise. However, symmetry is less prone to
> subtle errors (and far more satisfying).
> 
> [...]

Applied to for-next/hardening, thanks!

[1/1] overflow: Match arithmetic in both parts of __builtin_choose_expr
      https://git.kernel.org/kees/c/2f70a9b7e9c7

Take care,

-- 
Kees Cook

Re: [PATCH v2] overflow: Match arithmetic in both parts of __builtin_choose_expr

Posted by David Laight 1 week, 3 days ago

On Tue, 13 Oct 2026 20:31:46 +0000
Bill Wendling <morbo@google.com> wrote:

> The two parts of '__builtin_choose_expr()' had different arithmetic
> expressions. This patch corrects it so that both parts calculate:
> 
>   COUNT * sizeof(*MEMBER) + __must_be_array(MEMBER)
> 
> In practice, '__must_be_array' is meant to assert if 'MEMBER' isn't an
> array and return '0' otherwise. However, symmetry is less prone to
> subtle errors (and far more satisfying).
> 
> Signed-off-by: Bill Wendling <morbo@google.com>
> ---
> v2: Don't use 'size_add', which just adds more complicated macro cruft to the
>     expression. Instead, just adjust the parentheses.
> ---
>  include/linux/overflow.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> index a8cb6319b4fb..d0f04436343e 100644
> --- a/include/linux/overflow.h
> +++ b/include/linux/overflow.h
> @@ -418,7 +418,7 @@ static __always_inline size_t __must_check size_sub(size_t minuend, size_t subtr
>  #define flex_array_size(p, member, count)				\
>  	__builtin_choose_expr(__is_constexpr(count),			\
>  		(count) * sizeof(*(p)->member) + __must_be_array((p)->member),	\
> -		size_mul(count, sizeof(*(p)->member) + __must_be_array((p)->member)))
> +		size_mul(count, sizeof(*(p)->member)) + __must_be_array((p)->member))

Why does this use __builtin_choose_expr().
The types of the two expressions are the same so it might as well use ?:

Also doesn't this optimisation apply to all uses of size_mul() (aka saturating_mul()).
Presumably relying on the compiler to report the compilation time numeric overflow.

David

>  
>  /**
>   * struct_size() - Calculate size of structure with trailing flexible array.

Re: [PATCH v2] overflow: Match arithmetic in both parts of __builtin_choose_expr

Posted by Bill Wendling 1 week, 2 days ago

On Wed, Oct 14, 2026 at 5:38 AM David Laight
<david.laight.linux@gmail.com> wrote:
> On Tue, 13 Oct 2026 20:31:46 +0000
> Bill Wendling <morbo@google.com> wrote:
>
> > The two parts of '__builtin_choose_expr()' had different arithmetic
> > expressions. This patch corrects it so that both parts calculate:
> >
> >   COUNT * sizeof(*MEMBER) + __must_be_array(MEMBER)
> >
> > In practice, '__must_be_array' is meant to assert if 'MEMBER' isn't an
> > array and return '0' otherwise. However, symmetry is less prone to
> > subtle errors (and far more satisfying).
> >
> > Signed-off-by: Bill Wendling <morbo@google.com>
> > ---
> > v2: Don't use 'size_add', which just adds more complicated macro cruft to the
> >     expression. Instead, just adjust the parentheses.
> > ---
> >  include/linux/overflow.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> > index a8cb6319b4fb..d0f04436343e 100644
> > --- a/include/linux/overflow.h
> > +++ b/include/linux/overflow.h
> > @@ -418,7 +418,7 @@ static __always_inline size_t __must_check size_sub(size_t minuend, size_t subtr
> >  #define flex_array_size(p, member, count)                            \
> >       __builtin_choose_expr(__is_constexpr(count),                    \
> >               (count) * sizeof(*(p)->member) + __must_be_array((p)->member),  \
> > -             size_mul(count, sizeof(*(p)->member) + __must_be_array((p)->member)))
> > +             size_mul(count, sizeof(*(p)->member)) + __must_be_array((p)->member))
>
> Why does this use __builtin_choose_expr().
> The types of the two expressions are the same so it might as well use ?:
>
It's to emit only one of the expressions rather than both. If the
'count' is a constant expression, then it's possible for the compiler
to perform the calculation without having to hold onto the "false"
path of the ternary operator. So much more efficient in the long-run.

> Also doesn't this optimisation apply to all uses of size_mul() (aka saturating_mul()).
> Presumably relying on the compiler to report the compilation time numeric overflow.

I'm not familiar with saturating_mul(). I'm sure others on here can talk to it.

-bw

> David
>
> >
> >  /**
> >   * struct_size() - Calculate size of structure with trailing flexible array.
>

Re: [PATCH v2] overflow: Match arithmetic in both parts of __builtin_choose_expr

Posted by David Laight 1 week, 2 days ago

On Wed, 14 Oct 2026 13:39:03 -0700
Bill Wendling <morbo@google.com> wrote:

> On Wed, Oct 14, 2026 at 5:38 AM David Laight
> <david.laight.linux@gmail.com> wrote:
> > On Tue, 13 Oct 2026 20:31:46 +0000
> > Bill Wendling <morbo@google.com> wrote:
> >  
> > > The two parts of '__builtin_choose_expr()' had different arithmetic
> > > expressions. This patch corrects it so that both parts calculate:
> > >
> > >   COUNT * sizeof(*MEMBER) + __must_be_array(MEMBER)
> > >
> > > In practice, '__must_be_array' is meant to assert if 'MEMBER' isn't an
> > > array and return '0' otherwise. However, symmetry is less prone to
> > > subtle errors (and far more satisfying).
> > >
> > > Signed-off-by: Bill Wendling <morbo@google.com>
> > > ---
> > > v2: Don't use 'size_add', which just adds more complicated macro cruft to the
> > >     expression. Instead, just adjust the parentheses.
> > > ---
> > >  include/linux/overflow.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> > > index a8cb6319b4fb..d0f04436343e 100644
> > > --- a/include/linux/overflow.h
> > > +++ b/include/linux/overflow.h
> > > @@ -418,7 +418,7 @@ static __always_inline size_t __must_check size_sub(size_t minuend, size_t subtr
> > >  #define flex_array_size(p, member, count)                            \
> > >       __builtin_choose_expr(__is_constexpr(count),                    \
> > >               (count) * sizeof(*(p)->member) + __must_be_array((p)->member),  \
> > > -             size_mul(count, sizeof(*(p)->member) + __must_be_array((p)->member)))
> > > +             size_mul(count, sizeof(*(p)->member)) + __must_be_array((p)->member))  
> >
> > Why does this use __builtin_choose_expr().
> > The types of the two expressions are the same so it might as well use ?:
> >  
> It's to emit only one of the expressions rather than both. If the
> 'count' is a constant expression, then it's possible for the compiler
> to perform the calculation without having to hold onto the "false"
> path of the ternary operator. So much more efficient in the long-run.

I doubt it.
Even with __builtin_choose_expr() the compile has to do most of the work
for compiling the 'false' path.
If you use ?: then you can also use the much simpler __builtin_constant_p()
and possibly pick up cases where it is only known to be constant later.

> 
> > Also doesn't this optimisation apply to all uses of size_mul() (aka saturating_mul()).
> > Presumably relying on the compiler to report the compilation time numeric overflow.  
> 
> I'm not familiar with saturating_mul(). I'm sure others on here can talk to it.

That is me giving is a better name.
The same is true for size_add().
IMHO saturation size_t calculations is really broken because it allows
through 'bigger than sensible' sizes that still might cause problems
(eg kmalloc() of sizes just below the limit.)

David

> 
> -bw
> 
> > David
> >  
> > >
> > >  /**
> > >   * struct_size() - Calculate size of structure with trailing flexible array.  
> >

Re: [PATCH v2] overflow: Match arithmetic in both parts of __builtin_choose_expr

Posted by Bill Wendling 1 week, 1 day ago

On Thu, Oct 15, 2026 at 3:00 AM David Laight
<david.laight.linux@gmail.com> wrote:
> On Wed, 14 Oct 2026 13:39:03 -0700
> Bill Wendling <morbo@google.com> wrote:
>
> > On Wed, Oct 14, 2026 at 5:38 AM David Laight
> > <david.laight.linux@gmail.com> wrote:
> > > On Tue, 13 Oct 2026 20:31:46 +0000
> > > Bill Wendling <morbo@google.com> wrote:
> > >
> > > > The two parts of '__builtin_choose_expr()' had different arithmetic
> > > > expressions. This patch corrects it so that both parts calculate:
> > > >
> > > >   COUNT * sizeof(*MEMBER) + __must_be_array(MEMBER)
> > > >
> > > > In practice, '__must_be_array' is meant to assert if 'MEMBER' isn't an
> > > > array and return '0' otherwise. However, symmetry is less prone to
> > > > subtle errors (and far more satisfying).
> > > >
> > > > Signed-off-by: Bill Wendling <morbo@google.com>
> > > > ---
> > > > v2: Don't use 'size_add', which just adds more complicated macro cruft to the
> > > >     expression. Instead, just adjust the parentheses.
> > > > ---
> > > >  include/linux/overflow.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> > > > index a8cb6319b4fb..d0f04436343e 100644
> > > > --- a/include/linux/overflow.h
> > > > +++ b/include/linux/overflow.h
> > > > @@ -418,7 +418,7 @@ static __always_inline size_t __must_check size_sub(size_t minuend, size_t subtr
> > > >  #define flex_array_size(p, member, count)                            \
> > > >       __builtin_choose_expr(__is_constexpr(count),                    \
> > > >               (count) * sizeof(*(p)->member) + __must_be_array((p)->member),  \
> > > > -             size_mul(count, sizeof(*(p)->member) + __must_be_array((p)->member)))
> > > > +             size_mul(count, sizeof(*(p)->member)) + __must_be_array((p)->member))
> > >
> > > Why does this use __builtin_choose_expr().
> > > The types of the two expressions are the same so it might as well use ?:
> > >
> > It's to emit only one of the expressions rather than both. If the
> > 'count' is a constant expression, then it's possible for the compiler
> > to perform the calculation without having to hold onto the "false"
> > path of the ternary operator. So much more efficient in the long-run.
>
> I doubt it.
> Even with __builtin_choose_expr() the compile has to do most of the work
> for compiling the 'false' path.
> If you use ?: then you can also use the much simpler __builtin_constant_p()
> and possibly pick up cases where it is only known to be constant later.

In both scenarios the compiler is meant to do the majority of the
work, so I don't care about the compiler doing more work, only that
the work it does results in better code. First,
__builtin_constant_p(n) won't catch as many uses as __is_constexpr(n).
With the ternary operator, the compiler will generate code for both
sides and the predicate, which adds a needless if-then branch to the
CFG. Yes, that will get simplified, but again it's not a benefit over
__builtin_choose_expr(). In the simplest cases, the compilers (both
GCC and Clang) can decide which branch to take in the front-end.
That's possible with the ternary operator, but care needs to be taken
due to possible side-effects. Also, Clang performs far fewer of these
kinds of code transformations in the front-end. For instance, GCC will
perform some inlining in the front-end, making it easier for it to
catch __builtin_constant_p() uses that Clang has to delay.

All of this to say, there is no benefit to using the ternary operator,
even if you want to claim that the compiler is doing more work (which
we don't care about), and potentially worse code being generated, due
to processing that has to occur later on.

> > > Also doesn't this optimisation apply to all uses of size_mul() (aka saturating_mul()).
> > > Presumably relying on the compiler to report the compilation time numeric overflow.
> >
> > I'm not familiar with saturating_mul(). I'm sure others on here can talk to it.
>
> That is me giving is a better name.
> The same is true for size_add().
> IMHO saturation size_t calculations is really broken because it allows
> through 'bigger than sensible' sizes that still might cause problems
> (eg kmalloc() of sizes just below the limit.)
>
Patches welcome. :-)

-bw