[ovs-dev] [v4 02/12] dpif-netdev: Add auto validation function for miniflow extract

Eelco Chaudron echaudro at redhat.com
Wed Jun 30 09:06:05 UTC 2021



On 29 Jun 2021, at 18:15, Amber, Kumar wrote:

> Hi Eelco ,
>
> Sorry the formatting seems broken on this email thread.
> Replies are inlined .

Thanks, looking forward to the v5 (hopefully once I finished this version of the series. I’m currently at patch 10 ;)

> From: Eelco Chaudron <echaudro at redhat.com>
> Sent: Tuesday, June 29, 2021 7:36 PM
> To: Amber, Kumar <kumar.amber at intel.com>
> Cc: Van Haaren, Harry <harry.van.haaren at intel.com>; dev at openvswitch.org; i.maximets at ovn.org; Stokes, Ian <ian.stokes at intel.com>; Flavio Leitner <fbl at sysclose.org>
> Subject: Re: [ovs-dev] [v4 02/12] dpif-netdev: Add auto validation function for miniflow extract
>
>
> Not sure how you replied, but it’s hard to see which comments are mine, and which are yours.
>
> On 29 Jun 2021, at 14:27, Amber, Kumar wrote:
>
> Hi Eelco,
>
> Thanks Again for reviews , Pls find my replies inline.
>
> From: Eelco Chaudron <echaudro at redhat.com<mailto:echaudro at redhat.com>>
> Sent: Tuesday, June 29, 2021 5:14 PM
> To: Van Haaren, Harry <harry.van.haaren at intel.com<mailto:harry.van.haaren at intel.com>> ; Amber, Kumar <kumar.amber at intel.com<mailto:kumar.amber at intel.com>>
> Cc: dev at openvswitch.org<mailto:dev at openvswitch.org> ; i.maximets at ovn.org<mailto:i.maximets at ovn.org> ; Stokes, Ian <ian.stokes at intel.com<mailto:ian.stokes at intel.com>> ; Flavio Leitner <fbl at sysclose.org<mailto:fbl at sysclose.org>>
> Subject: Re: [ovs-dev] [v4 02/12] dpif-netdev: Add auto validation function for miniflow extract
>
> On 17 Jun 2021, at 18:27, Kumar Amber wrote:
>
> This patch introduced the auto-validation function which
> allows users to compare the batch of packets obtained from
> different miniflow implementations against the linear
> miniflow extract and return a hitmask.
>
> The autovaidator function can be triggered at runtime using the
> following command:
>
> $ ovs-appctl dpif-netdev/miniflow-parser-set autovalidator
>
> Signed-off-by: Kumar Amber <kumar.amber at intel.com<mailto:kumar.amber at intel.com>>
> Co-authored-by: Harry van Haaren <harry.van.haaren at intel.com<mailto:harry.van.haaren at intel.com>>
> Signed-off-by: Harry van Haaren <harry.van.haaren at intel.com<mailto:harry.van.haaren at intel.com>>
> ---
> lib/dpif-netdev-private-extract.c | 141 ++++++++++++++++++++++++++++++
> lib/dpif-netdev-private-extract.h | 15 ++++
> lib/dpif-netdev.c | 2 +-
> 3 files changed, 157 insertions(+), 1 deletion(-)
>
> diff --git a/lib/dpif-netdev-private-extract.c b/lib/dpif-netdev-private-extract.c
> index fcc56ef26..0741c19f9 100644
> --- a/lib/dpif-netdev-private-extract.c
> +++ b/lib/dpif-netdev-private-extract.c
> @@ -32,6 +32,11 @@ VLOG_DEFINE_THIS_MODULE(dpif_netdev_extract);
>
> /* Implementations of available extract options. */
> static struct dpif_miniflow_extract_impl mfex_impls[] = {
> + {
> + .probe = NULL,
> + .extract_func = dpif_miniflow_extract_autovalidator,
> + .name = "autovalidator",
> + },
> {
> .probe = NULL,
> .extract_func = NULL,
> @@ -84,3 +89,139 @@ dpif_miniflow_extract_info_get(struct dpif_miniflow_extract_impl **out_ptr)
> *out_ptr = mfex_impls;
> return ARRAY_SIZE(mfex_impls);
> }
> +
> +uint32_t
> +dpif_miniflow_extract_autovalidator(struct dp_packet_batch *packets,
> + struct netdev_flow_key *keys,
> + uint32_t keys_size, odp_port_t in_port,
> + void *pmd_handle)
> +{
> + const size_t cnt = dp_packet_batch_size(packets);
> + uint16_t good_l2_5_ofs[NETDEV_MAX_BURST];
> + uint16_t good_l3_ofs[NETDEV_MAX_BURST];
> + uint16_t good_l4_ofs[NETDEV_MAX_BURST];
> + uint16_t good_l2_pad_size[NETDEV_MAX_BURST];
> + struct dp_packet *packet;
> + struct dp_netdev_pmd_thread *pmd = pmd_handle;
> + struct dpif_miniflow_extract_impl *miniflow_funcs;
> +
> + int32_t mfunc_count = dpif_miniflow_extract_info_get(&miniflow_funcs);
> + if (mfunc_count < 0) {
>
> In theory 0 could not be returned, but just to cover the corner case can we change this to include zero.
>
> The code has been adapted as per Flavio comments so will not be a concern.
>
> + pmd->miniflow_extract_opt = NULL;
>
> Guess the above needs to be atomic.
>
> Removed based on Flavio comments.
>
> + VLOG_ERR("failed to get miniflow extract function implementations\n");
>
> Capital F to be in sync with your other error messages?
>
> Removed based on Flavio comments.
>
> + return 0;
> + }
> + ovs_assert(keys_size >= cnt);
>
>
> I don’t think we should assert here. Just return an error like above, so in production, we get notified, and this implementation gets disabled.
>
> Actually we do else one would most likely be overwriting the assigned array space for keys and will hit a Seg fault at some point.
>
> And hence we would like to know at the compile time if this is the case.
>
> But this is not a compile time check, it will crash OVS. You could just do this:
>
> if (keys_size < cnt) {
> pmd->miniflow_extract_opt = NULL;
> VLOG_ERR(“Invalid key size supplied etc. etc.\n”);
> return 0;
> }
>
> Or you could process up to key_size packets
>
> Reply:   sure I have taken the first approach in v5 as it safe and avoid any risk of Seg fault in V5.
>
> + struct netdev_flow_key test_keys[NETDEV_MAX_BURST];
> +
> + /* Run scalar miniflow_extract to get default result. */
> + DP_PACKET_BATCH_FOR_EACH (i, packet, packets) {
> + pkt_metadata_init(&packet->md, in_port);
> + miniflow_extract(packet, &keys[i].mf);
> +
> + /* Store known good metadata to compare with optimized metadata. */
> + good_l2_5_ofs[i] = packet->l2_5_ofs;
> + good_l3_ofs[i] = packet->l3_ofs;
> + good_l4_ofs[i] = packet->l4_ofs;
> + good_l2_pad_size[i] = packet->l2_pad_size;
> + }
> +
> + /* Iterate through each version of miniflow implementations. */
> + for (int j = MFEX_IMPL_START_IDX; j < ARRAY_SIZE(mfex_impls); j++) {
> + if (!mfex_impls[j].available) {
> + continue;
> + }
> +
> + /* Reset keys and offsets before each implementation. */
> + memset(test_keys, 0, keys_size * sizeof(struct netdev_flow_key));
> + DP_PACKET_BATCH_FOR_EACH (i, packet, packets) {
> + dp_packet_reset_offsets(packet);
> + }
> + /* Call optimized miniflow for each batch of packet. */
> + uint32_t hit_mask = mfex_impls[j].extract_func(packets, test_keys,
> + keys_size, in_port, pmd_handle);
> +
> + /* Do a miniflow compare for bits, blocks and offsets for all the
> + * classified packets in the hitmask marked by set bits. */
> + while (hit_mask) {
> + /* Index for the set bit. */
> + uint32_t i = __builtin_ctz(hit_mask);
> + /* Set the index in hitmask to Zero. */
> + hit_mask &= (hit_mask - 1);
> +
> + uint32_t failed = 0;
> +
> + /* Check miniflow bits are equal. */
> + if ((keys[i].mf.map.bits[0] != test_keys[i].mf.map.bits[0]) ||
> + (keys[i].mf.map.bits[1] != test_keys[i].mf.map.bits[1])) {
> + VLOG_ERR("Good 0x%llx 0x%llx\tTest 0x%llx 0x%llx\n",
> + keys[i].mf.map.bits[0], keys[i].mf.map.bits[1],
> + test_keys[i].mf.map.bits[0],
> + test_keys[i].mf.map.bits[1]);
> + failed = 1;
> + }
> +
> + if (!miniflow_equal(&keys[i].mf, &test_keys[i].mf)) {
> + uint32_t block_cnt = miniflow_n_values(&keys[i].mf);
> + VLOG_ERR("Autovalidation blocks failed for %s pkt %d",
> + mfex_impls[j].name, i);
> + VLOG_ERR(" Good hexdump:\n");
> + uint64_t *good_block_ptr = (uint64_t *)&keys[i].buf;
> + uint64_t *test_block_ptr = (uint64_t *)&test_keys[i].buf;
> + for (uint32_t b = 0; b < block_cnt; b++) {
> + VLOG_ERR(" %"PRIx64"\n", good_block_ptr[b]);
> + }
> + VLOG_ERR(" Test hexdump:\n");
> + for (uint32_t b = 0; b < block_cnt; b++) {
> + VLOG_ERR(" %"PRIx64"\n", test_block_ptr[b]);
> + }
> + failed = 1;
> + }
> +
> + if ((packets->packets[i]->l2_pad_size != good_l2_pad_size[i]) ||
> + (packets->packets[i]->l2_5_ofs != good_l2_5_ofs[i]) ||
> + (packets->packets[i]->l3_ofs != good_l3_ofs[i]) ||
> + (packets->packets[i]->l4_ofs != good_l4_ofs[i])) {
> + VLOG_ERR("Autovalidation packet offsets failed for %s pkt %d",
> + mfex_impls[j].name, i);
> + VLOG_ERR(" Good offsets: l2_pad_size %u, l2_5_ofs : %u"
> + " l3_ofs %u, l4_ofs %u\n",
> + good_l2_pad_size[i], good_l2_5_ofs[i],
> + good_l3_ofs[i], good_l4_ofs[i]);
> + VLOG_ERR(" Test offsets: l2_pad_size %u, l2_5_ofs : %u"
> + " l3_ofs %u, l4_ofs %u\n",
> + packets->packets[i]->l2_pad_size,
> + packets->packets[i]->l2_5_ofs,
> + packets->packets[i]->l3_ofs,
> + packets->packets[i]->l4_ofs);
> + failed = 1;
> + }
> +
> + if (failed) {
>
> Why stop now!? I think we should run all implementations, as others might need fixing too!
>
> We had the same model as above by you but with so many debug messages flooding made it impossible for us to
>
> Know the root cause and ovs_assert(0); have been removes so will no longer cause a crash but will simply disable the autovalidator
>
> I still would opt for getting all in the log. What is worse than getting a customer to crash (LOG) one failure, and with the provided fix, it will crash/log the next one :(
> And if the logs were not clear enough, maybe they need some rewriting?
>
> Reply:  As pointed out by Flavio in patch 07/12 , I guess we can do the following here as:
>
>            - Remove the ovs_assert()
>
>            - VLOG_ERR() any mismatches
>
>            - Continue processing rest of burst with MFEX Autovalidator
>
> On failures, we disable autovalidator *after* the burst, to avoid spamming output and filling logs.
>
> + /* Having dumped the debug info, disable autovalidator. */
> + VLOG_ERR("Autovalidation failed in %s pkt %d, disabling.\n",
> + mfex_impls[j].name, i);
> + /* Halt OVS here on debug builds. */
> + ovs_assert(0);
> + pmd->miniflow_extract_opt = NULL;
> + break;
> + }
> + }
> + }
> +
> + /* preserve packet correctness by storing back the good offsets in
> + * packets back. */
> + DP_PACKET_BATCH_FOR_EACH (i, packet, packets) {
> + packet->l2_5_ofs = good_l2_5_ofs[i];
> + packet->l3_ofs = good_l3_ofs[i];
> + packet->l4_ofs = good_l4_ofs[i];
> + packet->l2_pad_size = good_l2_pad_size[i];
> + }
> +
> + /* Returning zero implies no packets were hit by autovalidation. This
> + * simplifies unit-tests as changing --enable-mfex-default-autovalidator
> + * would pass/fail. By always returning zero, autovalidator is a little
> + * slower, but we gain consistency in testing.
> + */
> + return 0;
> +}
> diff --git a/lib/dpif-netdev-private-extract.h b/lib/dpif-netdev-private-extract.h
> index b7b0b2be4..455a7b590 100644
> --- a/lib/dpif-netdev-private-extract.h
> +++ b/lib/dpif-netdev-private-extract.h
> @@ -24,6 +24,11 @@
> /* Max size of dpif_miniflow_extract_impl array. */
> #define MFEX_IMPLS_MAX_SIZE (16)
>
> +/* Skip the autovalidator study and null when iterating all available
> + * miniflow implementations.
> + */
> +#define MFEX_IMPL_START_IDX (1)
> +
> /* Forward declarations. */
> struct dp_packet;
> struct miniflow;
> @@ -90,5 +95,15 @@ dpif_miniflow_extract_init(void);
> int32_t
> dpif_miniflow_extract_info_get(struct dpif_miniflow_extract_impl **out_ptr);
>
> Forgot to comment on this in my previous patch:
>
> /* Function pointer prototype to be implemented in the optimized miniflow
>
> * extract code.
> * returns the hitmask of the processed packets on success.
> * returns zero on failure.
>
> */
> typedef uint32_t (*miniflow_extract_func)(struct dp_packet_batch *batch,
> struct netdev_flow_key *keys,
> uint32_t keys_size,
> odp_port_t in_port,
> void *pmd_handle);
>
> Maybe we could add some description of what the input parameters mean, are expected? For example that key_size need to be at least the size of the batch? If this is the case, do we need it, and can we just assume keys should be at least batch size long?
>
> Yes put a comment in there about key size in the description in v5.
>
> But as per convention whenever we pass an array we should also put the size of the array and this also complies with various
>
> Security checkers and also prevents potential exploitations.
>
> Ack
>
> +/* Retrieve the hitmask of the batch of pakcets which is obtained by comparing
> + * different miniflow implementations with linear miniflow extract.
> + * On error, returns a zero.
> + * On success, returns the number of packets in the batch compared.
> + */
> +uint32_t
> +dpif_miniflow_extract_autovalidator(struct dp_packet_batch *batch,
> + struct netdev_flow_key *keys,
> + uint32_t keys_size, odp_port_t in_port,
> + void *pmd_handle);
>
> #endif /* DPIF_NETDEV_AVX512_EXTRACT */
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 567ebd952..4f4ab2790 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1181,8 +1181,8 @@ dpif_miniflow_extract_impl_set(struct unixctl_conn *conn, int argc,
> struct ds reply = DS_EMPTY_INITIALIZER;
> ds_put_format(&reply, "Miniflow implementation set to %s.\n", mfex_name);
> const char *reply_str = ds_cstr(&reply);
> - unixctl_command_reply(conn, reply_str);
> VLOG_INFO("%s", reply_str);
> + unixctl_command_reply(conn, reply_str);
> ds_destroy(&reply);
> }
>
> --
> 2.25.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org<mailto:dev at openvswitch.org>
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev



More information about the dev mailing list