[PATCH v2] Fix initializing a static union variable

[PATCH v2] Fix initializing a static union variable

[Yabin Cui]

saddr_wildcard is a static union variable initialized with {}.

Empty brace initialization of union types is unspecified prior to C23,
and even in C23, it doesn't guarantee zero initialization of all fields
(see sections 4.5 and 6.2 in
https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2900.htm).

Clang currently only initializes the first field to zero, leaving other
fields undefined. This can lead to unexpected behavior and optimizations
that produce random values (with some optimization flags).
See https://godbolt.org/z/hxnT1PTWo.

The issue has been reported to Clang upstream (
https://github.com/llvm/llvm-project/issues/78034#issuecomment-2183233517).
This commit mitigates the problem by avoiding empty brace initialization
in saddr_wildcard.

Fixes: 08ec9af1c062 ("xfrm: Fix xfrm_state_find() wrt. wildcard source address.")
Signed-off-by: Yabin Cui <yabinc@google.com>

---

Changes in v2:
- Update commit message to add/update links.

---
 net/xfrm/xfrm_state.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 649bb739df0d..9bc69d703e5c 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1139,7 +1139,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 		struct xfrm_policy *pol, int *err,
 		unsigned short family, u32 if_id)
 {
-	static xfrm_address_t saddr_wildcard = { };
+	static const xfrm_address_t saddr_wildcard;
 	struct net *net = xp_net(pol);
 	unsigned int h, h_wildcard;
 	struct xfrm_state *x, *x0, *to_put;
-- 
2.45.2.741.gdbec12cfda-goog

Re: [PATCH v2] Fix initializing a static union variable

[Nick Desaulniers]

On Fri, Jun 21, 2024 at 2:18 PM Yabin Cui <yabinc@google.com> wrote:
>
> saddr_wildcard is a static union variable initialized with {}.
>
> Empty brace initialization of union types is unspecified prior to C23,
> and even in C23, it doesn't guarantee zero initialization of all fields
> (see sections 4.5 and 6.2 in
> https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2900.htm).
>
> Clang currently only initializes the first field to zero, leaving other
> fields undefined. This can lead to unexpected behavior and optimizations
> that produce random values (with some optimization flags).
> See https://godbolt.org/z/hxnT1PTWo.
>
> The issue has been reported to Clang upstream (
> https://github.com/llvm/llvm-project/issues/78034#issuecomment-2183233517).
> This commit mitigates the problem by avoiding empty brace initialization
> in saddr_wildcard.

Thanks for the patch. The links add a lot more context.

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>
> Fixes: 08ec9af1c062 ("xfrm: Fix xfrm_state_find() wrt. wildcard source address.")
> Signed-off-by: Yabin Cui <yabinc@google.com>
>
> ---
>
> Changes in v2:
> - Update commit message to add/update links.
>
> ---
>  net/xfrm/xfrm_state.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 649bb739df0d..9bc69d703e5c 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -1139,7 +1139,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
>                 struct xfrm_policy *pol, int *err,
>                 unsigned short family, u32 if_id)
>  {
> -       static xfrm_address_t saddr_wildcard = { };
> +       static const xfrm_address_t saddr_wildcard;
>         struct net *net = xp_net(pol);
>         unsigned int h, h_wildcard;
>         struct xfrm_state *x, *x0, *to_put;
> --
> 2.45.2.741.gdbec12cfda-goog
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2] Fix initializing a static union variable

[Herbert Xu]

On Fri, Jun 21, 2024 at 02:18:19PM -0700, Yabin Cui wrote:
> saddr_wildcard is a static union variable initialized with {}.
> 
> Empty brace initialization of union types is unspecified prior to C23,
> and even in C23, it doesn't guarantee zero initialization of all fields
> (see sections 4.5 and 6.2 in
> https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2900.htm).

What about all the other places in the kernel that use the same
idiom? A grep shows that there are more than a hundred spots in
the kernel where {} is used to initialise a union.

$ git grep '=[[:blank:]]*{}' | grep union | wc -l
123
$

Also what if the union is embedded into a struct.  Does it get
initialised fully or not? If not then you've got over 5000 {}
initialisations to work through.

$ git grep '=[[:blank:]]*{}' | wc -l
5102
$

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2] Fix initializing a static union variable

[Linus Torvalds]

On Fri, 21 Jun 2024 at 15:36, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Fri, Jun 21, 2024 at 02:18:19PM -0700, Yabin Cui wrote:
> > saddr_wildcard is a static union variable initialized with {}.
> >
> > Empty brace initialization of union types is unspecified prior to C23,
> > and even in C23, it doesn't guarantee zero initialization of all fields
> > (see sections 4.5 and 6.2 in
> > https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2900.htm).
>
> What about all the other places in the kernel that use the same
> idiom? A grep shows that there are more than a hundred spots in
> the kernel where {} is used to initialise a union.

The important part is not what the standards text says - we happily
use things like inline asms that are entirely outside the standard -
but that apparently clang silently generates bogus code.

And from my admittedly _very_ limited testing, it's not that clang
always gets this wrong, but gets this wrong for a very particular
case: where the first field is smaller than the other fields.

And when the union is embedded in a struct, the struct initialization
seems to be ok from a quick test, but I might have screwed that test
up.

Now, it's still a worry, but I just wanted to point out that it's not
necessarily that *every* case is problematic.

Also, the problem Yabin found isn't actually limited to the empty
initializer. It happens even when you have an explicit zero in there.
All you need is _any_ initializer that doesn't initialize the whole
size.

End result: the "empty initializer" is a red herring and only relevant
to that standards paperwork.

So empty initializers are not relevant to the actual bug in question,
and I actually think that commit message is actively misleading in
trying to make it be about some "Linux isn't following standatrds".

But that also means that searching for empty initializers isn't going
to find all potential problem spots.

Notice how the suggested kernel patch was to remove the initializer
entirely, and just rely on "static variables are always zero" instead.

I don't know how to detect this problem case sanely, since the clang
bug occurs with non-static variables too, and with an actual non-empty
initializer too.

           Linus