On 4/27/22 18:41, Van Haaren, Harry wrote: >> -----Original Message----- >> From: Ferriter, Cian <[email protected]> >> Sent: Wednesday, April 27, 2022 4:58 PM >> To: [email protected] >> Cc: [email protected]; [email protected]; Van Haaren, Harry >> <[email protected]>; [email protected]; Ferriter, Cian >> <[email protected]> >> Subject: [PATCH] dpif-netdev-avx512: Fix overflow of UINT32_C(1). >> >> UINT64_C(1) is required in this bitshift since batch_size can be 32 and >> 1 << 32 overflows UINT32_C(1). >> >> Fixes: ba0a2619ca0c ("dpif-netdev-avx512: Fix ubsan shift error in >> bitmasks.") >> Signed-off-by: Cian Ferriter <[email protected]> > > Ugh, unsigned shift is defined, so UBSan won't flag *this* shift-out issue, > but will flag signed int overflow. Thanks Cian for finding reporting & fixing!
Hmm. "If the value of the right operand is negative or is greater than or equal to the width of the promoted left operand, the behavior is undefined." according to the standard. And UBsan does flag this construction for me if I'm putting it into a different place in code (I don't have avx512 machine at the moment): runtime error: shift exponent 32 is too large for 32-bit type 'unsigned int' AFAICT, on Intel CPU this will also result in a bitmask being zero, causing raw_ctz(0) (__builtin_ctz(0)) call which is also undefined according to compiler docs, but it seem to return 32 as a result. Next the subsequent invalid memory access at packets[32] that Asan should be able to catch. And OVS should also crash, because it highly unlikely for packets[32] to be a valid pointer. > > Acked-by: Harry van Haaren <[email protected]> > >> --- >> The other uses of UINT32_C(1) in the dpif-netdev-avx512 files are valid >> since those shifts are a maximum of 31 in all cases. > > Agreed, thanks for the explaining note. > > <snip patch> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
