On October 31, 2022 10:42:56 AM PDT, Sean Christopherson <seanjc@xxxxxxxxxx> wrote: >On Mon, Oct 31, 2022, Gaosheng Cui wrote: >> Shifting signed 32-bit value by 31 bits is undefined, so changing >> significant bit to unsigned. The UBSAN warning calltrace like below: >> >> UBSAN: shift-out-of-bounds in arch/x86/kvm/reverse_cpuid.h:101:11 >> left shift of 1 by 31 places cannot be represented in type 'int' > >PeterZ is contending that this isn't actually undefined behavior given how the >kernel is compiled[*]. That said, I would be in favor of replacing the open-coded >shift with BIT() to make the code a bit more self-documenting, and that would >naturally fix this maybe-undefined-behavior issue. > >[*] https://lore.kernel.org/all/Y1%2FAaJOcgIc%2FINtv@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx > >> --- >> arch/x86/kvm/reverse_cpuid.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/x86/kvm/reverse_cpuid.h b/arch/x86/kvm/reverse_cpuid.h >> index a19d473d0184..ebd6b621d3b8 100644 >> --- a/arch/x86/kvm/reverse_cpuid.h >> +++ b/arch/x86/kvm/reverse_cpuid.h >> @@ -98,7 +98,7 @@ static __always_inline u32 __feature_bit(int x86_feature) >> x86_feature = __feature_translate(x86_feature); >> >> reverse_cpuid_check(x86_feature / 32); >> - return 1 << (x86_feature & 31); >> + return 1U << (x86_feature & 31); >> } >> >> #define feature_bit(name) __feature_bit(X86_FEATURE_##name) >> -- >> 2.25.1 >> One really ought to change the input to unsigned, though, and I would argue >> 5 would be more idiomatic than / 32; / goes with % whereas >> goes with &; a mishmash is just ugly AF.
