linux-kernel - Re: [PATCH v3][next] overflow: Fix direct struct member initialization in _DEFINE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d4eed1e3-6fc0-4372-8ced-ae6a49f3c5c1@intel.com>
Date: Fri, 30 May 2025 16:59:35 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: "Gustavo A. R. Silva" <gustavoars@...nel.org>, Kees Cook <kees@...nel.org>
CC: <linux-hardening@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3][next] overflow: Fix direct struct member
 initialization in _DEFINE_FLEX()

From: Gustavo A. R. Silva <gustavoars@...nel.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@...nel.org>

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

Thanks,
Olek