From 87f191a3a398ae495fa55d70ca5fe5d813bd284f Mon Sep 17 00:00:00 2001 From: Ilya Maximets 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 Signed-off-by: Ilya Maximets 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