Re: [ovs-dev] [PATCH v2 1/5] netdev-offload-tc: Fix the mask for tunnel metadata length.

[Roi Dayan via dev]

On 2022-07-29 5:53 PM, Ilya Maximets wrote:

'wc.masks.tunnel.metadata.present.len' must be a mask for the same
field in the flow key, but in the tc_flower structure it's the real
length of metadata masks.

That is correctly handled for the individual opt->length, setting all
the masks to 0x1f like it's done in the tun_metadata_to_geneve_mask__(),
but not handled for the main 'len' field.

Fix that by setting the mask to 0xff like it's done during the flow
translation in xlate_actions() and during the flow dump in the
tun_metadata_from_geneve_nlattr().

Extra checks and comments added around the code to better explain
what is going on.

Fixes: a468645c6d33 ("lib/tc: add geneve with option match offload")
Signed-off-by: Ilya Maximets <i.maxim...@ovn.org>
---
  lib/netdev-offload-tc.c |  7 +++++--
  lib/tc.c                | 13 ++++++++++---
  2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 29d0e63eb..750cc5682 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -606,8 +606,9 @@ flower_tun_opt_to_match(struct match *match, struct 
tc_flower *flower)
          len -= sizeof(struct geneve_opt) + opt->length * 4;
      }

- match->wc.masks.tunnel.metadata.present.len =

-           flower->mask.tunnel.metadata.present.len;
+    /* In the 'flower' mask len is an actual length, not the mask.
+     * But in 'match' it is an actual mask and should be an exact match. */
+    match->wc.masks.tunnel.metadata.present.len = 0xff;
      match->wc.masks.tunnel.flags |= FLOW_TNL_F_UDPIF;
  }

@@ -1574,6 +1575,8 @@ flower_match_to_tun_opt(struct tc_flower *flower, const struct flow_tnl *tnl,

          len -= sizeof(struct geneve_opt) + opt->length * 4;
      }

+ /* Copying from the key and not from the mask, since in the 'flower'

+     * the length for a mask is not a mask, but the actual length. */
      flower->mask.tunnel.metadata.present.len = tnl->metadata.present.len;
  }

diff --git a/lib/tc.c b/lib/tc.c

index 25e66b5c5..0f3f27c11 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -721,15 +721,17 @@ flower_tun_geneve_opt_check_len(struct tun_metadata *key,
      const struct geneve_opt *opt, *opt_mask;
      int len, cnt = 0;

+ if (key->present.len != mask->present.len) {

+            goto bad_length;
+    }
+
      len = key->present.len;
      while (len) {
          opt = &key->opts.gnv[cnt];
          opt_mask = &mask->opts.gnv[cnt];

if (opt->length != opt_mask->length) {

-            VLOG_ERR_RL(&error_rl,
-                        "failed to parse tun options; key/mask length differ");
-            return EINVAL;
+            goto bad_length;
          }

cnt += sizeof(struct geneve_opt) / 4 + opt->length;

@@ -737,6 +739,11 @@ flower_tun_geneve_opt_check_len(struct tun_metadata *key,
      }

return 0;

+
+bad_length:
+    VLOG_ERR_RL(&error_rl,
+                "failed to parse tun options; key/mask length differ");
+    return EINVAL;
  }

static int

reviewed and tested the series with some tunnel traffic.
same test I did on prev patchset.
thanks

Reviewed-by: Roi Dayan <r...@nvidia.com>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev