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

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

[Bill Wendling]

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

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

In practice, the call to '__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).

Fixes: 230f6fa2c1db ("overflow: Provide constant expression struct_size")
Signed-off-by: Bill Wendling <morbo@google.com>
---
 include/linux/overflow.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index a8cb6319b4fb..265ec265aa3e 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -418,7 +418,8 @@ 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_add(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] overflow: Match arithmetic in both parts of __builtin_choose_expr

[Kees Cook]

On Mon, Oct 12, 2026 at 08:59:19PM +0000, Bill Wendling wrote:
> The two parts of the '__builtin_choose_expr()' had different arithmetic
> expressions. This patch correct it so that both parts calculate:
> 
>   COUNT * sizeof(*MEMBER) + __must_be_array(MEMBER)
> 
> In practice, the call to '__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).
> 
> Fixes: 230f6fa2c1db ("overflow: Provide constant expression struct_size")

A Fixes trailer implies a bug being solved. What problem is there here?
I only see downsides to this change: this adds even more layers of
macros and inlinings to gain the size_add() for a constant integer 0.

?

> -		size_mul(count, sizeof(*(p)->member) + __must_be_array((p)->member)))
> +		size_add(size_mul(count, sizeof(*(p)->member)),		\
> +			 __must_be_array((p)->member)))

-- 
Kees Cook

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

[Bill Wendling]

On Mon, Oct 12, 2026 at 8:30 PM Kees Cook <kees@kernel.org> wrote:
>
> On Mon, Oct 12, 2026 at 08:59:19PM +0000, Bill Wendling wrote:
> > The two parts of the '__builtin_choose_expr()' had different arithmetic
> > expressions. This patch correct it so that both parts calculate:
> >
> >   COUNT * sizeof(*MEMBER) + __must_be_array(MEMBER)
> >
> > In practice, the call to '__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).
> >
> > Fixes: 230f6fa2c1db ("overflow: Provide constant expression struct_size")
>
> A Fixes trailer implies a bug being solved. What problem is there here?
> I only see downsides to this change: this adds even more layers of
> macros and inlinings to gain the size_add() for a constant integer 0.
>
> ?
>
I wasn't sure about the Fixes, sorry about that. Why not put the "+
__must_be_array" stuff outside of the `size_mul` instead, since it's
only there to catch an error? So something like

  (__builtin_choose_expr(<pred>, <true>, <false>) + __must_be_array(...))

It simplifies the macro itself and makes it explicit that the
__must_be_array stuff isn't useful for the actual value.
?

> > -             size_mul(count, sizeof(*(p)->member) + __must_be_array((p)->member)))
> > +             size_add(size_mul(count, sizeof(*(p)->member)),         \
> > +                      __must_be_array((p)->member)))
>
> --
> Kees Cook

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

[Bill Wendling]

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

[David Laight]

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

[Bill Wendling]

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.
>