netdev - Re: [PATCH] Fix initializing a static union variable

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] [day] [month] [year] [list]

Message-ID: <CALJ9ZPMHCPt-6kf-9McdKYTqs8Vrj9GLhkxObdhjyorgtZQOSg@mail.gmail.com>
Date: Mon, 24 Jun 2024 11:04:51 -0700
From: Yabin Cui <yabinc@...gle.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Herbert Xu <herbert@...dor.apana.org.au>, Nick Desaulniers <ndesaulniers@...gle.com>, 
	Steffen Klassert <steffen.klassert@...unet.com>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Nathan Chancellor <nathan@...nel.org>, Bill Wendling <morbo@...gle.com>, 
	Justin Stitt <justinstitt@...gle.com>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, llvm@...ts.linux.dev
Subject: Re: [PATCH] Fix initializing a static union variable

> In other words, in the kernel we simply depend on initializers working
> reliably and fully. Partly because we've literally been told by
> compiler people that that is what we should do.
>
> So no, this is not about empty initializers. And this is not about
> some C standard verbiage.
>
> This is literally about "the linux kernel expects initializers to
> FULLY initialize variables". Padding, other union members, you name
> it.
>
> If clang doesn't do that, then clang is buggy as far as the kernel is
> concerned, and no amount of standards reading is relevant.
>
> And in particular, no amount of "but empty initializer" is relevant.
>

Thanks for the detailed explanation!
Sorry for limiting the problem to the empty initializer. I didn't
realize the linux
kernel also depends on zero initializing extra bytes when explicitly
initializing
one field of a union type.

> 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.

I also think so. But probably we need to add tests in clang to make sure
it continues to work.

> Hmm. Strange. godbolt says that it happens with clang 17.0.1 (and
> earlier) with a plain -O2.
>
> It just doesn't happen for me. Either this got fixed already and my
> 17.0.6 has the fix, or there's some subtle flag that my test-case uses
> (some config file - I just use "-O2" for local testing like the
> godbolt page did).
>
> But clearly godbolt does  think this happens with released clang versions too.
>

Yes, I also think it happens in both clang trunk and past releases, as tested in
https://godbolt.org/z/vnGqKK43z.
Gladly in the clang bug in
https://github.com/llvm/llvm-project/issues/78034, no one
is against zero initializing the whole union type.
I feel now it's no longer important to have this patch in the kernel.
But we need to
fix this problem in clang and backport the fix to releases we care about.

Thanks,
Yabin