| Message ID | 20240621211819.1690234-1-yabinc@google.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] Fix initializing a static union variable | expand |
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 >
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,
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
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;
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(-)