| Message ID | 20240827160143.184945-1-mkp@redhat.com |
|---|---|
| State | Accepted, archived |
| Commit | d7a9a9eb624fa64564ce016a138e527cd059f38b |
| Delegated to: | Eelco Chaudron |
| Headers | show |
| Series | [ovs-dev] userspace: Correctly set ip offload flag in native tunneling. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
On 27 Aug 2024, at 18:01, Mike Pattrick wrote: > Coverity identified the following issue > > CID 425094: (#1 of 1): Unchecked return value (CHECKED_RETURN) > 4. check_return: Calling dp_packet_hwol_tx_ip_csum without checking > return value (as is done elsewhere 9 out of 11 times). > > This appears to be a true positive, the fields getter was called instead > of its setter. > > Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.") > Reported-by: Eelco Chaudron <echaudro@redhat.com> > Signed-off-by: Mike Pattrick <mkp@redhat.com> Thanks for the patch! The fix looks good to me. Is there any reason why none of the CI tests picked this up? I guess we need real hardware to verify this. Cheers, Eelco Acked-by: Eelco Chaudron <echaudro@redhat.com> > --- > lib/netdev-native-tnl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c > index 16c56608d..529d64fe1 100644 > --- a/lib/netdev-native-tnl.c > +++ b/lib/netdev-native-tnl.c > @@ -254,7 +254,7 @@ dp_packet_tnl_ol_process(struct dp_packet *packet, > > if (IP_VER(ip->ip_ihl_ver) == 4) { > dp_packet_hwol_set_tx_ipv4(packet); > - dp_packet_hwol_tx_ip_csum(packet); > + dp_packet_hwol_set_tx_ip_csum(packet); > } else if (IP_VER(ip->ip_ihl_ver) == 6) { > dp_packet_hwol_set_tx_ipv6(packet); > } > -- > 2.43.5
Eelco Chaudron <echaudro@redhat.com> writes: > On 27 Aug 2024, at 18:01, Mike Pattrick wrote: > >> Coverity identified the following issue >> >> CID 425094: (#1 of 1): Unchecked return value (CHECKED_RETURN) >> 4. check_return: Calling dp_packet_hwol_tx_ip_csum without checking >> return value (as is done elsewhere 9 out of 11 times). >> >> This appears to be a true positive, the fields getter was called instead >> of its setter. >> >> Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.") >> Reported-by: Eelco Chaudron <echaudro@redhat.com> >> Signed-off-by: Mike Pattrick <mkp@redhat.com> > > Thanks for the patch! The fix looks good to me. Is there any reason > why none of the CI tests picked this up? I guess we need real hardware > to verify this. I guess we have identified a coverage gap. :) > Cheers, > > Eelco > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > > >> --- >> lib/netdev-native-tnl.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c >> index 16c56608d..529d64fe1 100644 >> --- a/lib/netdev-native-tnl.c >> +++ b/lib/netdev-native-tnl.c >> @@ -254,7 +254,7 @@ dp_packet_tnl_ol_process(struct dp_packet *packet, >> >> if (IP_VER(ip->ip_ihl_ver) == 4) { >> dp_packet_hwol_set_tx_ipv4(packet); >> - dp_packet_hwol_tx_ip_csum(packet); >> + dp_packet_hwol_set_tx_ip_csum(packet); >> } else if (IP_VER(ip->ip_ihl_ver) == 6) { >> dp_packet_hwol_set_tx_ipv6(packet); >> } >> -- >> 2.43.5 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Wed, Aug 28, 2024 at 10:32:01AM -0400, Aaron Conole wrote: > Eelco Chaudron <echaudro@redhat.com> writes: > > > On 27 Aug 2024, at 18:01, Mike Pattrick wrote: > > > >> Coverity identified the following issue > >> > >> CID 425094: (#1 of 1): Unchecked return value (CHECKED_RETURN) > >> 4. check_return: Calling dp_packet_hwol_tx_ip_csum without checking > >> return value (as is done elsewhere 9 out of 11 times). > >> > >> This appears to be a true positive, the fields getter was called instead > >> of its setter. > >> > >> Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.") > >> Reported-by: Eelco Chaudron <echaudro@redhat.com> > >> Signed-off-by: Mike Pattrick <mkp@redhat.com> > > > > Thanks for the patch! The fix looks good to me. Is there any reason > > why none of the CI tests picked this up? I guess we need real hardware > > to verify this. > > I guess we have identified a coverage gap. :) I'm inclined to think that can be addressed as a follow-up. > > Cheers, > > > > Eelco > > > > Acked-by: Eelco Chaudron <echaudro@redhat.com> Acked-by: Simon Horman <horms@ovn.org> Eelco, will you will handle applying this (or not)?
On 29 Aug 2024, at 10:43, Simon Horman wrote: > On Wed, Aug 28, 2024 at 10:32:01AM -0400, Aaron Conole wrote: >> Eelco Chaudron <echaudro@redhat.com> writes: >> >>> On 27 Aug 2024, at 18:01, Mike Pattrick wrote: >>> >>>> Coverity identified the following issue >>>> >>>> CID 425094: (#1 of 1): Unchecked return value (CHECKED_RETURN) >>>> 4. check_return: Calling dp_packet_hwol_tx_ip_csum without checking >>>> return value (as is done elsewhere 9 out of 11 times). >>>> >>>> This appears to be a true positive, the fields getter was called instead >>>> of its setter. >>>> >>>> Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.") >>>> Reported-by: Eelco Chaudron <echaudro@redhat.com> >>>> Signed-off-by: Mike Pattrick <mkp@redhat.com> >>> >>> Thanks for the patch! The fix looks good to me. Is there any reason >>> why none of the CI tests picked this up? I guess we need real hardware >>> to verify this. >> >> I guess we have identified a coverage gap. :) > > I'm inclined to think that can be addressed as a follow-up. > >>> Cheers, >>> >>> Eelco >>> >>> Acked-by: Eelco Chaudron <echaudro@redhat.com> > > Acked-by: Simon Horman <horms@ovn.org> > > Eelco, will you will handle applying this (or not)? Thanks Simon, yes I’ll apply it.
On 27 Aug 2024, at 18:01, Mike Pattrick wrote: > Coverity identified the following issue > > CID 425094: (#1 of 1): Unchecked return value (CHECKED_RETURN) > 4. check_return: Calling dp_packet_hwol_tx_ip_csum without checking > return value (as is done elsewhere 9 out of 11 times). > > This appears to be a true positive, the fields getter was called instead > of its setter. > > Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.") > Reported-by: Eelco Chaudron <echaudro@redhat.com> > Signed-off-by: Mike Pattrick <mkp@redhat.com> Thanks to Mike and the reviewers. This patch has been applied and backported till 3.3. Cheers, Eelco > --- > lib/netdev-native-tnl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c > index 16c56608d..529d64fe1 100644 > --- a/lib/netdev-native-tnl.c > +++ b/lib/netdev-native-tnl.c > @@ -254,7 +254,7 @@ dp_packet_tnl_ol_process(struct dp_packet *packet, > > if (IP_VER(ip->ip_ihl_ver) == 4) { > dp_packet_hwol_set_tx_ipv4(packet); > - dp_packet_hwol_tx_ip_csum(packet); > + dp_packet_hwol_set_tx_ip_csum(packet); > } else if (IP_VER(ip->ip_ihl_ver) == 6) { > dp_packet_hwol_set_tx_ipv6(packet); > } > -- > 2.43.5
Simon Horman <horms@ovn.org> writes: > On Wed, Aug 28, 2024 at 10:32:01AM -0400, Aaron Conole wrote: >> Eelco Chaudron <echaudro@redhat.com> writes: >> >> > On 27 Aug 2024, at 18:01, Mike Pattrick wrote: >> > >> >> Coverity identified the following issue >> >> >> >> CID 425094: (#1 of 1): Unchecked return value (CHECKED_RETURN) >> >> 4. check_return: Calling dp_packet_hwol_tx_ip_csum without checking >> >> return value (as is done elsewhere 9 out of 11 times). >> >> >> >> This appears to be a true positive, the fields getter was called instead >> >> of its setter. >> >> >> >> Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.") >> >> Reported-by: Eelco Chaudron <echaudro@redhat.com> >> >> Signed-off-by: Mike Pattrick <mkp@redhat.com> >> > >> > Thanks for the patch! The fix looks good to me. Is there any reason >> > why none of the CI tests picked this up? I guess we need real hardware >> > to verify this. >> >> I guess we have identified a coverage gap. :) > > I'm inclined to think that can be addressed as a follow-up. Yes - if it truly requires some real hardware to address we will need to add such a test, but that wouldn't hold up applying a fix. >> > Cheers, >> > >> > Eelco >> > >> > Acked-by: Eelco Chaudron <echaudro@redhat.com> > > Acked-by: Simon Horman <horms@ovn.org> > > Eelco, will you will handle applying this (or not)?
diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c index 16c56608d..529d64fe1 100644 --- a/lib/netdev-native-tnl.c +++ b/lib/netdev-native-tnl.c @@ -254,7 +254,7 @@ dp_packet_tnl_ol_process(struct dp_packet *packet, if (IP_VER(ip->ip_ihl_ver) == 4) { dp_packet_hwol_set_tx_ipv4(packet); - dp_packet_hwol_tx_ip_csum(packet); + dp_packet_hwol_set_tx_ip_csum(packet); } else if (IP_VER(ip->ip_ihl_ver) == 6) { dp_packet_hwol_set_tx_ipv6(packet); }
Coverity identified the following issue CID 425094: (#1 of 1): Unchecked return value (CHECKED_RETURN) 4. check_return: Calling dp_packet_hwol_tx_ip_csum without checking return value (as is done elsewhere 9 out of 11 times). This appears to be a true positive, the fields getter was called instead of its setter. Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.") Reported-by: Eelco Chaudron <echaudro@redhat.com> Signed-off-by: Mike Pattrick <mkp@redhat.com> --- lib/netdev-native-tnl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)