161 lines
6.0 KiB
Diff
161 lines
6.0 KiB
Diff
From 87f191a3a398ae495fa55d70ca5fe5d813bd284f Mon Sep 17 00:00:00 2001
|
|
From: Ilya Maximets <i.maximets@ovn.org>
|
|
Date: Sun, 14 Aug 2022 16:45:59 +0200
|
|
Subject: [PATCH] netdev-offload-tc: Fix the mask for tunnel metadata length.
|
|
|
|
'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().
|
|
|
|
Also, flower always has an exact match on the present.len field
|
|
regardless of its value and regardless of this field being masked
|
|
by OVS flow translation layer while installing the flow. Hence,
|
|
all tunnel flows dumped from TC should have an exact match on
|
|
present.len and also UDPIF flag, because present.len doesn't make
|
|
sense without that flag. Without the change, zero-length options
|
|
match is incorrectly reported as a wildcard match. The side effect
|
|
though is that zero-length match on geneve options is reported even
|
|
for other tunnel types, e.g. vxlan. But that should be fairly
|
|
harmless. To avoid reporting a match on empty geneve options for
|
|
vxlan/etc. tunnels we'll need to check the tunnel port type, there
|
|
is no enough information in the TUNNEL attribute itself.
|
|
|
|
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")
|
|
Reviewed-by: Roi Dayan <roid@nvidia.com>
|
|
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
|
|
|
|
Reference:https://github.com/openvswitch/ovs/commit/87f191a3a398ae495fa55d70ca5fe5d813bd284f
|
|
Conflict:Context adaptation
|
|
|
|
---
|
|
lib/netdev-offload-tc.c | 33 +++++++++++++++++++++++----------
|
|
lib/tc.c | 13 ++++++++++---
|
|
2 files changed, 33 insertions(+), 13 deletions(-)
|
|
|
|
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
|
|
index 68c4c7e..2383fe2 100644
|
|
--- a/lib/netdev-offload-tc.c
|
|
+++ b/lib/netdev-offload-tc.c
|
|
@@ -476,30 +476,42 @@ flower_tun_opt_to_match(struct match *match, struct tc_flower *flower)
|
|
struct geneve_opt *opt, *opt_mask;
|
|
int len, cnt = 0;
|
|
|
|
+ /* Options are always in UDPIF format in the 'flower'. */
|
|
+ match->flow.tunnel.flags |= FLOW_TNL_F_UDPIF;
|
|
+ match->wc.masks.tunnel.flags |= FLOW_TNL_F_UDPIF;
|
|
+
|
|
+ match->flow.tunnel.metadata.present.len =
|
|
+ flower->key.tunnel.metadata.present.len;
|
|
+ /* In the 'flower' mask len is an actual length, not a mask. But in the
|
|
+ * 'match' it is an actual mask, so should be an exact match, because TC
|
|
+ * will always match on the exact value. */
|
|
+ match->wc.masks.tunnel.metadata.present.len = 0xff;
|
|
+
|
|
+ if (!flower->key.tunnel.metadata.present.len) {
|
|
+ /* No options present. */
|
|
+ return;
|
|
+ }
|
|
+
|
|
memcpy(match->flow.tunnel.metadata.opts.gnv,
|
|
flower->key.tunnel.metadata.opts.gnv,
|
|
flower->key.tunnel.metadata.present.len);
|
|
- match->flow.tunnel.metadata.present.len =
|
|
- flower->key.tunnel.metadata.present.len;
|
|
- match->flow.tunnel.flags |= FLOW_TNL_F_UDPIF;
|
|
memcpy(match->wc.masks.tunnel.metadata.opts.gnv,
|
|
flower->mask.tunnel.metadata.opts.gnv,
|
|
flower->mask.tunnel.metadata.present.len);
|
|
|
|
+ /* Fixing up 'length' fields of particular options, since these are
|
|
+ * also not masks, but actual lengths in the 'flower' structure. */
|
|
len = flower->key.tunnel.metadata.present.len;
|
|
while (len) {
|
|
opt = &match->flow.tunnel.metadata.opts.gnv[cnt];
|
|
opt_mask = &match->wc.masks.tunnel.metadata.opts.gnv[cnt];
|
|
|
|
+ /* "Exact" match as set in tun_metadata_to_geneve_mask__(). */
|
|
opt_mask->length = 0x1f;
|
|
|
|
cnt += sizeof(struct geneve_opt) / 4 + opt->length;
|
|
len -= sizeof(struct geneve_opt) + opt->length * 4;
|
|
}
|
|
-
|
|
- match->wc.masks.tunnel.metadata.present.len =
|
|
- flower->mask.tunnel.metadata.present.len;
|
|
- match->wc.masks.tunnel.flags |= FLOW_TNL_F_UDPIF;
|
|
}
|
|
|
|
static int
|
|
@@ -621,9 +633,8 @@ parse_tc_flower_to_match(struct tc_flower *flower,
|
|
if (flower->key.tunnel.tp_dst) {
|
|
match_set_tun_tp_dst(match, flower->key.tunnel.tp_dst);
|
|
}
|
|
- if (flower->key.tunnel.metadata.present.len) {
|
|
- flower_tun_opt_to_match(match, flower);
|
|
- }
|
|
+
|
|
+ flower_tun_opt_to_match(match, flower);
|
|
}
|
|
|
|
act_off = nl_msg_start_nested(buf, OVS_FLOW_ATTR_ACTIONS);
|
|
@@ -1079,6 +1090,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 1e9810f..ef6138f 100644
|
|
--- a/lib/tc.c
|
|
+++ b/lib/tc.c
|
|
@@ -588,15 +588,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;
|
|
@@ -604,6 +606,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
|
|
--
|
|
2.33.0
|
|
|