Re: [RFC][PATCH] overflow: Twiddle with struct_size()

Re: [RFC][PATCH] overflow: Twiddle with struct_size()

[Kees Cook]

On Wed, Mar 05, 2025 at 02:43:15PM +0100, Peter Zijlstra wrote:
> Hi Kees,
> 
> I keep getting hit by the struct_size() brigade, and I keep having
> trouble reading that macro.
> 
> I had a wee poke and ended up with the below, WDYT?

Ah, and to clarify, this is just for readability? (There have been
some tweaks to reduce the macro depths and other things in other areas,
so I just wanted to check that wasn't, or was, part of the rationale.)

> (I also tried to create a __must_be_flex_array(), but utterly failed :/)

I spent a lot of time trying to find something for that too. :( If you
do ever find it, please share! :)

> ---
> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> index 0c7e3dcfe867..2123d0e238bb 100644
> --- a/include/linux/overflow.h
> +++ b/include/linux/overflow.h
> @@ -352,9 +352,10 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
>   * Return: number of bytes needed or SIZE_MAX on overflow.
>   */
>  #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)))
> +	(__must_be_array((p)->member) +					\
> +	 __builtin_choose_expr(__is_constexpr(count),			\
> +			       sizeof(*(p)->member) * (count),		\
> +			       size_mul(sizeof(*(p)->member), (count))))

For both, I need to double check that __must_be_array() is
always a constant expression. If not, we can't move it out of the
__builtin_choose_expr(). But if so, then yeah, this is nice.

>  
>  /**
>   * struct_size() - Calculate size of structure with trailing flexible array.
> @@ -367,10 +368,12 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
>   *
>   * Return: number of bytes needed or SIZE_MAX on overflow.
>   */
> -#define struct_size(p, member, count)					\
> -	__builtin_choose_expr(__is_constexpr(count),			\
> -		sizeof(*(p)) + flex_array_size(p, member, count),	\
> -		size_add(sizeof(*(p)), flex_array_size(p, member, count)))
> +#define struct_size(p, member, count)					       \
> +	(__must_be_array((p)->member) +					       \
> +	 __builtin_choose_expr(__is_constexpr(count),			       \
> +			       sizeof(*(p)) + (sizeof((p)->member) * (count)), \

typo: above should be                        (sizeof(*(p)->member))

(hint, to test this code use "./tools/testing/kunit/kunit.py run overflow")

But yeah, this passes the overflow tests which include the constant
expression tests, so __must_be_array() is a constant expression. Whee :)

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

This one I'm not such a fan of. It feels wrong to not use
flex_array_size() here -- we're performing exactly the same
calculation. But it's possible this has cpp complexity reduction
benefits from avoiding the stacking of __builtin_choose_expr() and
__must_be_array() macros...

So, yeah, I think I could live with this. :) Especially if it means we can
start using struct_size() in code you look at. :P

But please include this selftest update too. Since your patch splits the
dependency between struct_size() and flex_array_size(), I'd like to test
them separately now:


diff --git a/lib/tests/overflow_kunit.c b/lib/tests/overflow_kunit.c
index 894691b4411a..ee29f1f8f528 100644
--- a/lib/tests/overflow_kunit.c
+++ b/lib/tests/overflow_kunit.c
@@ -705,18 +705,28 @@ struct __test_flex_array {
 
 static void overflow_size_helpers_test(struct kunit *test)
 {
-	/* Make sure struct_size() can be used in a constant expression. */
-	u8 ce_array[struct_size_t(struct __test_flex_array, data, 55)];
+	/* Verify struct_size() family can be used as constant expressions. */
 	struct __test_flex_array *obj;
+	u8 ce_flex[flex_array_size(obj, data, 22)];
+	u8 ce_array[struct_size(obj, data, 55)];
+	u8 ce_array_t[struct_size_t(typeof(*obj), data, 99)];
 	int count = 0;
 	int var;
 	volatile int unconst = 0;
 
-	/* Verify constant expression against runtime version. */
+	/* Verify constant expression versions against runtime versions. */
+	var = 22;
+	OPTIMIZER_HIDE_VAR(var);
+	KUNIT_EXPECT_EQ(test, sizeof(ce_flex), flex_array_size(obj, data, var));
+
 	var = 55;
 	OPTIMIZER_HIDE_VAR(var);
 	KUNIT_EXPECT_EQ(test, sizeof(ce_array), struct_size(obj, data, var));
 
+	var = 99;
+	OPTIMIZER_HIDE_VAR(var);
+	KUNIT_EXPECT_EQ(test, sizeof(ce_array_t), struct_size_t(typeof(*obj), data, var));
+
 #define check_one_size_helper(expected, func, args...)	do {	\
 	size_t _r = func(args);					\
 	KUNIT_EXPECT_EQ_MSG(test, _r, expected,			\



(And note that the above patch is against -next, where the lib/ kunit
tests have moved into the lib/tests/ subdirectory now.)

-Kees

-- 
Kees Cook

Re: [RFC][PATCH] overflow: Twiddle with struct_size()

[Peter Zijlstra]

On Wed, Mar 05, 2025 at 11:13:00PM -0800, Kees Cook wrote:
> On Wed, Mar 05, 2025 at 02:43:15PM +0100, Peter Zijlstra wrote:
> > Hi Kees,
> > 
> > I keep getting hit by the struct_size() brigade, and I keep having
> > trouble reading that macro.
> > 
> > I had a wee poke and ended up with the below, WDYT?
> 
> Ah, and to clarify, this is just for readability? (There have been
> some tweaks to reduce the macro depths and other things in other areas,
> so I just wanted to check that wasn't, or was, part of the rationale.)

Yeah, the current thing nests that flex array macro thing, with the very
same arguments, which means you then have to go read that.

That combined with that nested constexptr toggle makes it a bit of a
mess.

> > (I also tried to create a __must_be_flex_array(), but utterly failed :/)
> 
> I spent a lot of time trying to find something for that too. :( If you
> do ever find it, please share! :)
> 
> > ---
> > diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> > index 0c7e3dcfe867..2123d0e238bb 100644
> > --- a/include/linux/overflow.h
> > +++ b/include/linux/overflow.h
> > @@ -352,9 +352,10 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
> >   * Return: number of bytes needed or SIZE_MAX on overflow.
> >   */
> >  #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)))
> > +	(__must_be_array((p)->member) +					\
> > +	 __builtin_choose_expr(__is_constexpr(count),			\
> > +			       sizeof(*(p)->member) * (count),		\
> > +			       size_mul(sizeof(*(p)->member), (count))))
> 
> For both, I need to double check that __must_be_array() is
> always a constant expression. If not, we can't move it out of the
> __builtin_choose_expr(). But if so, then yeah, this is nice.

__must_be_array() is BUILD_BUG_ON() which must be a constant expression.

> >  
> >  /**
> >   * struct_size() - Calculate size of structure with trailing flexible array.
> > @@ -367,10 +368,12 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
> >   *
> >   * Return: number of bytes needed or SIZE_MAX on overflow.
> >   */
> > -#define struct_size(p, member, count)					\
> > -	__builtin_choose_expr(__is_constexpr(count),			\
> > -		sizeof(*(p)) + flex_array_size(p, member, count),	\
> > -		size_add(sizeof(*(p)), flex_array_size(p, member, count)))
> > +#define struct_size(p, member, count)					       \
> > +	(__must_be_array((p)->member) +					       \
> > +	 __builtin_choose_expr(__is_constexpr(count),			       \
> > +			       sizeof(*(p)) + (sizeof((p)->member) * (count)), \
> 
> typo: above should be                        (sizeof(*(p)->member))
> 
> (hint, to test this code use "./tools/testing/kunit/kunit.py run overflow")

D'oh. I sent the old version :-(. I actually did a kernel build and
fixed this error, and then went and sent the old one :/

> But yeah, this passes the overflow tests which include the constant
> expression tests, so __must_be_array() is a constant expression. Whee :)
> 
> > +			       size_add(sizeof(*(p)),			       \
> > +					size_mul(sizeof(*(p)->member), count))))
> 
> This one I'm not such a fan of. It feels wrong to not use
> flex_array_size() here -- we're performing exactly the same
> calculation. 

Yes, but now you can read it without also having to untangle
flex_array_size().

I did have a version using size_mad(x,y,z) := (x*y)+z, which is more
compact, but then you have the problem that you forever forget what
order around those 3 arguments are. (x*y)+z vs x+(y*z) etc.

Much like I can never remember the struct_size() argument order (at
least neovim's clangd lsp can somewhat help with that -- if I can get it
tamed).

> But it's possible this has cpp complexity reduction
> benefits from avoiding the stacking of __builtin_choose_expr() and
> __must_be_array() macros...

Just so.

> So, yeah, I think I could live with this. :) Especially if it means we can
> start using struct_size() in code you look at. :P
> 
> But please include this selftest update too. Since your patch splits the
> dependency between struct_size() and flex_array_size(), I'd like to test
> them separately now:

OK, will do. Thanks!