| Message ID | 20220419162031.1463302-1-harry.van.haaren@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] dpif-netdev-avx512: fix ubsan shift error in bitmasks | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 19 Apr 2022, at 18:20, Harry van Haaren wrote: > The code changes here are to handle (1 << i) shifts where 'i' is the > packet index in the batch, and 1 << 31 is an overflow of the signed '1'. > > Fixed by adding ULL suffix to the 1 character, ensuring compiler knows > the 1 is unsigned (and 32-bits minimum). Undefined Behaviour sanitizer > is now happy with the shifts at runtime. Change looks good to me, but should 1UL not be enough, as the destinations are all 32-bit? > Suggested-by: Ilya Maximets <i.maximets@ovn.org> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > > --- > > Thanks Ilya for the detail in the email - reworked as commit message; > https://mail.openvswitch.org/pipermail/ovs-dev/2022-April/393270.html > > --- > lib/dpif-netdev-avx512.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c > index b7131ba3f..fdefee230 100644 > --- a/lib/dpif-netdev-avx512.c > +++ b/lib/dpif-netdev-avx512.c > @@ -183,7 +183,7 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd, > * classifed by vector mfex else do a scalar miniflow extract > * for that packet. > */ > - bool mfex_hit = !!(mf_mask & (1 << i)); > + bool mfex_hit = !!(mf_mask & (1ULL << i)); > > /* Check for a partial hardware offload match. */ > if (hwol_enabled) { > @@ -204,7 +204,7 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd, > > pkt_meta[i].bytes = dp_packet_size(packet); > phwol_hits++; > - hwol_emc_smc_hitmask |= (1 << i); > + hwol_emc_smc_hitmask |= (1ULL << i); > continue; > } > } > @@ -227,7 +227,7 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd, > if (f) { > rules[i] = &f->cr; > emc_hits++; > - hwol_emc_smc_hitmask |= (1 << i); > + hwol_emc_smc_hitmask |= (1ULL << i); > continue; > } > } > @@ -237,7 +237,7 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd, > if (f) { > rules[i] = &f->cr; > smc_hits++; > - smc_hitmask |= (1 << i); > + smc_hitmask |= (1ULL << i); > continue; > } > } > -- > 2.25.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Wed, Apr 20, 2022 at 10:19 AM Eelco Chaudron <echaudro@redhat.com> wrote: > On 19 Apr 2022, at 18:20, Harry van Haaren wrote: > > > The code changes here are to handle (1 << i) shifts where 'i' is the > > packet index in the batch, and 1 << 31 is an overflow of the signed '1'. > > > > Fixed by adding ULL suffix to the 1 character, ensuring compiler knows > > the 1 is unsigned (and 32-bits minimum). Undefined Behaviour sanitizer > > is now happy with the shifts at runtime. > > Change looks good to me, but should 1UL not be enough, as the destinations are all 32-bit? For storing/comparing to explicit uint32_t variables, either (uint32_t)1 or UINT32_C(1) are more natural. Any reason not to use those?
On 22 Apr 2022, at 10:29, David Marchand wrote: > On Wed, Apr 20, 2022 at 10:19 AM Eelco Chaudron <echaudro@redhat.com> wrote: >> On 19 Apr 2022, at 18:20, Harry van Haaren wrote: >> >>> The code changes here are to handle (1 << i) shifts where 'i' is the >>> packet index in the batch, and 1 << 31 is an overflow of the signed '1'. >>> >>> Fixed by adding ULL suffix to the 1 character, ensuring compiler knows >>> the 1 is unsigned (and 32-bits minimum). Undefined Behaviour sanitizer >>> is now happy with the shifts at runtime. >> >> Change looks good to me, but should 1UL not be enough, as the destinations are all 32-bit? > > For storing/comparing to explicit uint32_t variables, either > (uint32_t)1 or UINT32_C(1) are more natural. > Any reason not to use those? Forgot about those :) They are even better...
> -----Original Message----- > From: Eelco Chaudron <echaudro@redhat.com> > Sent: Friday, April 22, 2022 9:48 AM > To: David Marchand <david.marchand@redhat.com> > Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; ovs-dev@openvswitch.org; > Ilya Maximets <i.maximets@ovn.org> > Subject: Re: [ovs-dev] [PATCH] dpif-netdev-avx512: fix ubsan shift error in bitmasks > > > > On 22 Apr 2022, at 10:29, David Marchand wrote: > > > On Wed, Apr 20, 2022 at 10:19 AM Eelco Chaudron <echaudro@redhat.com> > wrote: > >> On 19 Apr 2022, at 18:20, Harry van Haaren wrote: > >> > >>> The code changes here are to handle (1 << i) shifts where 'i' is the > >>> packet index in the batch, and 1 << 31 is an overflow of the signed '1'. > >>> > >>> Fixed by adding ULL suffix to the 1 character, ensuring compiler knows > >>> the 1 is unsigned (and 32-bits minimum). Undefined Behaviour sanitizer > >>> is now happy with the shifts at runtime. > >> > >> Change looks good to me, but should 1UL not be enough, as the destinations are > all 32-bit? > > > > For storing/comparing to explicit uint32_t variables, either > > (uint32_t)1 or UINT32_C(1) are more natural. > > Any reason not to use those? > > Forgot about those :) They are even better... OK, thanks folks, will respin a v2 with those. Regards, -Harry
diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c index b7131ba3f..fdefee230 100644 --- a/lib/dpif-netdev-avx512.c +++ b/lib/dpif-netdev-avx512.c @@ -183,7 +183,7 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd, * classifed by vector mfex else do a scalar miniflow extract * for that packet. */ - bool mfex_hit = !!(mf_mask & (1 << i)); + bool mfex_hit = !!(mf_mask & (1ULL << i)); /* Check for a partial hardware offload match. */ if (hwol_enabled) { @@ -204,7 +204,7 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd, pkt_meta[i].bytes = dp_packet_size(packet); phwol_hits++; - hwol_emc_smc_hitmask |= (1 << i); + hwol_emc_smc_hitmask |= (1ULL << i); continue; } } @@ -227,7 +227,7 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd, if (f) { rules[i] = &f->cr; emc_hits++; - hwol_emc_smc_hitmask |= (1 << i); + hwol_emc_smc_hitmask |= (1ULL << i); continue; } } @@ -237,7 +237,7 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd, if (f) { rules[i] = &f->cr; smc_hits++; - smc_hitmask |= (1 << i); + smc_hitmask |= (1ULL << i); continue; } }
The code changes here are to handle (1 << i) shifts where 'i' is the packet index in the batch, and 1 << 31 is an overflow of the signed '1'. Fixed by adding ULL suffix to the 1 character, ensuring compiler knows the 1 is unsigned (and 32-bits minimum). Undefined Behaviour sanitizer is now happy with the shifts at runtime. Suggested-by: Ilya Maximets <i.maximets@ovn.org> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> --- Thanks Ilya for the detail in the email - reworked as commit message; https://mail.openvswitch.org/pipermail/ovs-dev/2022-April/393270.html --- lib/dpif-netdev-avx512.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)