Currently, to statically initialize the struct members of the `type`
object created by _DEFINE_FLEX(), the internal `obj` member must be
explicitly referenced at the call site. See:

struct flex {
        int a;
        int b;
        struct foo flex_array[];
};

_DEFINE_FLEX(struct flex, instance, flex_array,
                 FIXED_SIZE, = {
                        .obj = {
                                .a = 0,
                                .b = 1,
                        },
                });

This leaks _DEFINE_FLEX() internal implementation details and make
the helper harder to use and read.

Fix this and allow for a more natural and intuitive C99 init-style:

_DEFINE_FLEX(struct flex, instance, flex_array,
                 FIXED_SIZE, = {
                        .a = 0,
                        .b = 1,
                });

Note that before these changes, the `initializer` argument was optional,
but now it's required.

Also, update "counter" member initialization in DEFINE_FLEX().

Fixes: 26dd68d293fd ("overflow: add DEFINE_FLEX() for on-stack allocs")
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
Changes in v3:
 - Fix changelog text: s/_DEFINE_RAW_FLEX/_DEFINE_FLEX

Changes in v2:
 - Update kernel-doc block for _DEFINE_FLEX(). (Kees)
 - Update changelog text - `initializer` argument is now required.
 - Link: https://lore.kernel.org/all/aBP0b3gfurLFDlwY@kspp/

v1:
 - Link: https://lore.kernel.org/linux-hardening/aBK2TUEeQfCFop9Y@kspp/

 include/linux/overflow.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index 69533e703be5..b50c8b30a14a 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -396,7 +396,7 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
  * @name: Name for a variable to define.
  * @member: Name of the array member.
  * @count: Number of elements in the array; must be compile-time const.
- * @initializer: initializer expression (could be empty for no init).
+ * @initializer: Initializer expression (e.g., pass `= { }` at minimum).
  */
 #define _DEFINE_FLEX(type, name, member, count, initializer...)			\
 	_Static_assert(__builtin_constant_p(count),				\
@@ -404,7 +404,7 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
 	union {									\
 		u8 bytes[struct_size_t(type, member, count)];			\
 		type obj;							\
-	} name##_u initializer;							\
+	} name##_u = { .obj initializer };					\
 	type *name = (type *)&name##_u
 
 /**
@@ -444,7 +444,7 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
  * elements in array @member.
  */
 #define DEFINE_FLEX(TYPE, NAME, MEMBER, COUNTER, COUNT)	\
-	_DEFINE_FLEX(TYPE, NAME, MEMBER, COUNT, = { .obj.COUNTER = COUNT, })
+	_DEFINE_FLEX(TYPE, NAME, MEMBER, COUNT, = { .COUNTER = COUNT, })
 
 /**
  * STACK_FLEX_ARRAY_SIZE() - helper macro for DEFINE_FLEX() family.
-- 
2.43.0

From: Gustavo A. R. Silva <gustavoars@kernel.org>
Date: Thu, 1 May 2025 18:44:43 -0600

Hey Gustavo, Kees,

> Currently, to statically initialize the struct members of the `type`
> object created by _DEFINE_FLEX(), the internal `obj` member must be
> explicitly referenced at the call site. See:
> 
> struct flex {
>         int a;
>         int b;
>         struct foo flex_array[];
> };
> 
> _DEFINE_FLEX(struct flex, instance, flex_array,
>                  FIXED_SIZE, = {
>                         .obj = {
>                                 .a = 0,
>                                 .b = 1,
>                         },
>                 });
> 
> This leaks _DEFINE_FLEX() internal implementation details and make
> the helper harder to use and read.
> 
> Fix this and allow for a more natural and intuitive C99 init-style:
> 
> _DEFINE_FLEX(struct flex, instance, flex_array,
>                  FIXED_SIZE, = {
>                         .a = 0,
>                         .b = 1,
>                 });
> 
> Note that before these changes, the `initializer` argument was optional,
> but now it's required.

Unfortunately this can hurt performance on my setup.
In XDP, we usually place &xdp_buff on the stack. It's 56 bytes. We
initialize it only during the packet processing, not in advance.

In libeth_xdp, see [1], I provide a small extension for this struct.
This extension is 64 byte, plus I added a definition (see
`__LIBETH_XDP_ONSTACK_BUFF()`) to be able to define a bigger one in case
a driver might need more fields there.
The same as with &xdp_buff, it shouldn't be initialized in advance, only
during the packet processing. Otherwise, it can really decrease
performance, you might've seen recent Mellanox report that even
CONFIG_INIT_STACK_ZERO provokes this.

What would be the best option to resolve this? This flex XDP buff on the
stack is fully safe, there are a couple checks that its size does not
exceed the maximum (defined by xdp_buff_xsk) etc. And we really need to
initialize it field-by-field in a loop on a per-packet basis -- we are
sure that there will be no garbage.

It's even worse that most drivers will most likely not want to add any
additional fields, i.e. this flex array at the end will be empty, IOW
they just want a plain libeth_xdp_buff, but I made a unified definition,
with which you can declare them on the stack both with and without
additional fields. So, even if the drivers doesn't want any additional
fields and the flex array is empty, the struct will be zero-initialized
and the same perf hit will apply.

> 
> Also, update "counter" member initialization in DEFINE_FLEX().
> 
> Fixes: 26dd68d293fd ("overflow: add DEFINE_FLEX() for on-stack allocs")
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>

[1]
https://lore.kernel.org/netdev/20250520205920.2134829-9-anthony.l.nguyen@intel.com

Thanks,
Olek

On Fri, May 30, 2025 at 04:59:35PM +0200, Alexander Lobakin wrote:
> Unfortunately this can hurt performance on my setup.
> In XDP, we usually place &xdp_buff on the stack. It's 56 bytes. We
> initialize it only during the packet processing, not in advance.
> 
> In libeth_xdp, see [1], I provide a small extension for this struct.
> This extension is 64 byte, plus I added a definition (see
> `__LIBETH_XDP_ONSTACK_BUFF()`) to be able to define a bigger one in case
> a driver might need more fields there.
> The same as with &xdp_buff, it shouldn't be initialized in advance, only
> during the packet processing. Otherwise, it can really decrease
> performance, you might've seen recent Mellanox report that even
> CONFIG_INIT_STACK_ZERO provokes this.

FYI, you can use __uninitialized to force CONFIG_INIT_STACK_ZERO to
leave an automatic variable uninitialized.

> What would be the best option to resolve this? This flex XDP buff on the
> stack is fully safe, there are a couple checks that its size does not
> exceed the maximum (defined by xdp_buff_xsk) etc. And we really need to
> initialize it field-by-field in a loop on a per-packet basis -- we are
> sure that there will be no garbage.

But yes, this is suddenly not available for _DEFINE_FLEX after the
referenced patch.

> It's even worse that most drivers will most likely not want to add any
> additional fields, i.e. this flex array at the end will be empty, IOW
> they just want a plain libeth_xdp_buff, but I made a unified definition,
> with which you can declare them on the stack both with and without
> additional fields. So, even if the drivers doesn't want any additional
> fields and the flex array is empty, the struct will be zero-initialized
> and the same perf hit will apply.
> [...]
> [1] https://lore.kernel.org/netdev/20250520205920.2134829-9-anthony.l.nguyen@intel.com

Okay, so it sounds like you need the old behavior of _DEFINE_FLEX, *and*
a way to apply attributes (like __uninitialized).

How about replacing _DEFINE_FLEX with:


#define __DEFINE_FLEX(type, name, member, count, trailer...)		\
        _Static_assert(__builtin_constant_p(count),			\
                       "onstack flex array members require compile-time const count"); \
        union {								\
                u8 bytes[struct_size_t(type, member, count)];		\
                type obj;						\
        } name##_u trailer;						\
        type *name = (type *)&name##_u

#define _DEFINE_FLEX(type, name, member, count, initializer...)		\
	__DEFINE_FLEX(type, name, member, count, = { .obj = initializer })


Which would yield this for ___LIBETH_XDP_ONSTACK_BUFF:


#define ___LIBETH_XDP_ONSTACK_BUFF(name, ...)				\
	__DEFINE_FLEX(struct libeth_xdp_buff, name, priv,		\
		      LIBETH_XDP_PRIV_SZ(__VA_ARGS__ + 0),		\
		      __uninitialized);					\
	LIBETH_XDP_ASSERT_PRIV_SZ(__VA_ARGS__ + 0)


Does that look like what you'd want? (Note I didn't actually build this;
I want to make sure the concept is workable...)

-- 
Kees Cook

On Fri, May 30, 2025 at 11:06:01AM -0700, Kees Cook wrote:
> #define __DEFINE_FLEX(type, name, member, count, trailer...)		\
>         _Static_assert(__builtin_constant_p(count),			\
>                        "onstack flex array members require compile-time const count"); \
>         union {								\
>                 u8 bytes[struct_size_t(type, member, count)];		\
>                 type obj;						\
>         } name##_u trailer;						\
>         type *name = (type *)&name##_u
> 
> #define _DEFINE_FLEX(type, name, member, count, initializer...)		\
> 	__DEFINE_FLEX(type, name, member, count, = { .obj = initializer })
> [...]
> Does that look like what you'd want? (Note I didn't actually build this;
> I want to make sure the concept is workable...)

FWIW, this is working as expected: https://godbolt.org/z/P7Go8Tr33

I'll send a proper patch...

-- 
Kees Cook

On Thu, 01 May 2025 18:44:43 -0600, Gustavo A. R. Silva wrote:
> Currently, to statically initialize the struct members of the `type`
> object created by _DEFINE_FLEX(), the internal `obj` member must be
> explicitly referenced at the call site. See:
> 
> struct flex {
>         int a;
>         int b;
>         struct foo flex_array[];
> };
> 
> [...]

I've replaced the v2 and applied this to for-next/hardening, thanks!

[1/1] overflow: Fix direct struct member initialization in _DEFINE_FLEX()
      https://git.kernel.org/kees/c/76cacf008235

Take care,

-- 
Kees Cook