| Message ID | 20221206141800.772789-1-emma.finn@intel.com |
|---|---|
| State | Accepted |
| Commit | 739bcf2263b3dfbc8a855c6e5b4a2b77742dd8db |
| Headers | show |
| Series | [ovs-dev] odp-execute: Fix ipv4 missing clearing of connection tracking fields. | 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 12/6/22 15:18, Emma Finn wrote: > This patch add clearing of connection tracking fields to the > avx512 implementation of the ipv4 action. This patch also extends > the actions autovalidator to include a compare for packet metadata. > > Signed-off-by: Emma Finn <emma.finn@intel.com> > --- > lib/odp-execute-avx512.c | 2 ++ > lib/odp-execute-private.c | 12 ++++++++++++ > 2 files changed, 14 insertions(+) Thanks! That fixes the issue with the conntrack for me. The patch is missing the Fixes tag. I added one. With that, applied and backported to 3.0. However, there is one more difference I spotted between avx512 and the scalar code. It's the 'l4_size' check before writing to the L4 header. In the packet_set_ipv4_addr() we have: if (nh->ip_proto == IPPROTO_TCP && l4_size >= TCP_HEADER_LEN) { ... } else if (nh->ip_proto == IPPROTO_UDP && l4_size >= UDP_HEADER_LEN ) { ... } But we don't have these 'l4_size' checks in avx512 implementations. These checks are there to catch any truncated packets that do not really have a full L4 header in them, so they supposed to prevent writes outside of the actual packet. I'm not sure how to trigger the issue in tests, but since we're trying to keep the implementations functionally identical, we need to add these checks to action_avx512_ipv4_set_addrs(). Or am I missing something? Best regards, Ilya Maximets.
> -----Original Message----- > From: Ilya Maximets <i.maximets@ovn.org> > Sent: Wednesday 7 December 2022 19:09 > To: Finn, Emma <emma.finn@intel.com>; dev@openvswitch.org > Cc: i.maximets@ovn.org; Van Haaren, Harry <harry.van.haaren@intel.com>; > Stokes, Ian <ian.stokes@intel.com>; Eelco Chaudron > <echaudro@redhat.com> > Subject: Re: [PATCH] odp-execute: Fix ipv4 missing clearing of connection > tracking fields. > > On 12/6/22 15:18, Emma Finn wrote: > > This patch add clearing of connection tracking fields to the > > avx512 implementation of the ipv4 action. This patch also extends the > > actions autovalidator to include a compare for packet metadata. > > > > Signed-off-by: Emma Finn <emma.finn@intel.com> > > --- > > lib/odp-execute-avx512.c | 2 ++ > > lib/odp-execute-private.c | 12 ++++++++++++ > > 2 files changed, 14 insertions(+) > > Thanks! That fixes the issue with the conntrack for me. > > The patch is missing the Fixes tag. I added one. With that, applied and > backported to 3.0. > > > However, there is one more difference I spotted between avx512 and the > scalar code. It's the 'l4_size' check before writing to the L4 header. > > In the packet_set_ipv4_addr() we have: > > if (nh->ip_proto == IPPROTO_TCP && l4_size >= TCP_HEADER_LEN) { > ... > } else if (nh->ip_proto == IPPROTO_UDP && l4_size >= UDP_HEADER_LEN ) > { > ... > } > > But we don't have these 'l4_size' checks in avx512 implementations. > > These checks are there to catch any truncated packets that do not really have > a full L4 header in them, so they supposed to prevent writes outside of the > actual packet. > > I'm not sure how to trigger the issue in tests, but since we're trying to keep > the implementations functionally identical, we need to add these checks to > action_avx512_ipv4_set_addrs(). > > Or am I missing something? > Sure, I will add those checks and send another patch for this ipv4 fix. Thanks, Emma > Best regards, Ilya Maximets.
diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c index 6c7713251..66b3998da 100644 --- a/lib/odp-execute-avx512.c +++ b/lib/odp-execute-avx512.c @@ -477,6 +477,8 @@ action_avx512_ipv4_set_addrs(struct dp_packet_batch *batch, th->tcp_csum = tcp_checksum; } + + pkt_metadata_init_conn(&packet->md); } /* Write back the modified IPv4 addresses. */ _mm256_mask_storeu_epi32((void *) nh, 0x1F, v_new_hdr); diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c index f80ae5a23..57be5cfe7 100644 --- a/lib/odp-execute-private.c +++ b/lib/odp-execute-private.c @@ -229,6 +229,18 @@ action_autoval_generic(struct dp_packet_batch *batch, const struct nlattr *a) } } + /* Compare packet metadata. */ + if (memcmp(&good_pkt->md, &test_pkt->md, sizeof good_pkt->md)) { + ds_put_format(&log_msg, "Autovalidation metadata failed\n"); + ds_put_format(&log_msg, "Good packet metadata:\n"); + ds_put_sparse_hex_dump(&log_msg, &good_pkt->md, + sizeof good_pkt->md, 0, false); + ds_put_format(&log_msg, "Test packet metadata:\n"); + ds_put_sparse_hex_dump(&log_msg, &test_pkt->md, + sizeof test_pkt->md, 0, false); + failed = true; + } + if (failed) { VLOG_ERR("Autovalidation of %s failed. Details:\n%s", action_impls[impl].name, ds_cstr(&log_msg));
This patch add clearing of connection tracking fields to the avx512 implementation of the ipv4 action. This patch also extends the actions autovalidator to include a compare for packet metadata. Signed-off-by: Emma Finn <emma.finn@intel.com> --- lib/odp-execute-avx512.c | 2 ++ lib/odp-execute-private.c | 12 ++++++++++++ 2 files changed, 14 insertions(+)