On Tue, Jul 15, 2025 at 10:24:50PM -0700, Nathan Chancellor wrote:
On Wed, Jul 16, 2025 at 07:06:50AM +0200, Greg Kroah-Hartman wrote:
On Tue, Jul 15, 2025 at 01:33:32PM -0700, Nathan Chancellor wrote:
After a recent change in clang to expose uninitialized warnings from const variables [1], there is a warning in cxacru_heavy_init():
drivers/usb/atm/cxacru.c:1104:6: error: variable 'bp' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized] 1104 | if (instance->modem_type->boot_rom_patch) { | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/usb/atm/cxacru.c:1113:39: note: uninitialized use occurs here 1113 | cxacru_upload_firmware(instance, fw, bp); | ^~ drivers/usb/atm/cxacru.c:1104:2: note: remove the 'if' if its condition is always true 1104 | if (instance->modem_type->boot_rom_patch) { | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/usb/atm/cxacru.c:1095:32: note: initialize the variable 'bp' to silence this warning 1095 | const struct firmware *fw, *bp; | ^ | = NULL
This warning occurs in clang's frontend before inlining occurs, so it cannot notice that bp is only used within cxacru_upload_firmware() under the same condition that initializes it in cxacru_heavy_init(). Just initialize bp to NULL to silence the warning without functionally changing the code, which is what happens with modern compilers when they support '-ftrivial-auto-var-init=zero' (CONFIG_INIT_STACK_ALL_ZERO=y).
We generally do not want to paper over compiler bugs, when our code is correct, so why should we do that here? Why not fix clang instead?
I would not really call this a compiler bug. It IS passed uninitialized to this function and while the uninitialized value is not actually used, clang has no way of knowing that at this point in its pipeline, so I don't think warning in this case is unreasonable.
No, I take it back, it is unreasonable :)
At runtime, there never is a uninitialized use of this pointer, the first time it is used, it is intended to be filled in if this is a "boot rom patch": ret = cxacru_find_firmware(instance, "bp", &bp);
Then if that call fails, the function exits, great.
Then later on, this is called: cxacru_upload_firmware(instance, fw, bp); so either bp IS valid, or it's still uninitialized, fair enough.
But then cxacru_upload_firmware() does the same check for "is this a boot rom patch" and only then does it reference the variable.
And when it references it, it does NOT check if it is valid or not, so even if you do pre-initialize this to NULL, surely some other static checker is going to come along and say "Hey, you just dereferenced a NULL pointer, this needs to be fixed!" when that too is just not true at all.
So the logic here is all "safe" for now, and if you set this to NULL, you are just papering over the fact that it is right, AND setting us up to get another patch that actually does nothing, while feeling like the submitter just fixed a security bug, demanding a CVE for an impossible code path :)
So let's leave this for now because:
This type of warning is off for GCC because of how unreliable it was when it is done in the middle end with optimizations. Furthermore, it is my understanding based on [1] that just the passing of an uninitialized variable in this manner is UB.
As gcc can't handle this either, it seems that clang also can't handle it. So turning this on for the kernel surely is going to trip it up in other places than just this one driver.
If you _really_ want to fix this, refactor the code to be more sane and obvious from a C parsing standpoint, but really, it isn't that complex for a human to read and understand, and I see why it was written this way.
As for the UB argument, bah, I don't care, sane compilers will do the right thing, i.e. pass in the uninitialized value, or if we turned on the 0-fill stack option, will be NULL anyway, otherwise why do we have that option if not to "solve" the UB issue?).
thanks,
greg k-h