Thanks for taking time to review the patch!
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
I have made a patch v2 and submitted it, replacing the open-coded shift with BIT().
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.
.
I have changed the input to unsigned in patch v2, and replaced "/ 32" with "argue >> 5".
On 2022/11/1 4:27, H. Peter Anvin wrote:
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.
.
