[PATCH net-next 00/10] tc flower: tunnel key partitioning for RX queue steering

[PATCH net-next 00/10] tc flower: tunnel key partitioning for RX queue steering

[Alexandre Cassen]

RSS cant spread GTP-U traffic between a few endpoints. The outer headers
barely vary and mlx5 does not hash the TEID. One flower rule per TEID does
not scale.

Add a mask to the flower enc_key_id key and offload it on mlx5e NIC flows
with skbedit queue_mapping. One rule then steers a block of tunnel keys to
an RX queue. This "tunnel key partitioning" supports fine-grained session
placement. In a 5G mobile network, the UPF can place a session on a CPU by
allocating its TEID from the right block [1]. Any scheduling algorithm can
then pick that block.

  tc filter add dev eth0 ingress protocol ip flower skip_sw \
      ip_proto udp dst_port 2152 enc_key_id 0x40000000/0xc0000000 \
      action skbedit queue_mapping 1 skip_sw

This series generalize an earlier GTP-U work [2] to other tunnels that the
ConnectX family can steer: GRE, VXLAN and GENEVE.

Patches 1 to 5 refuse a partial mask or a key wider than the VNI where a
driver cant honor them. Patches 6 and 7 add the flower mask and its test.
Patch 8 adds queue mapping to mlx5e NIC flows. Patch 9 adds the tunnel key
match to mlx5e NIC and eswitch flows without a tunnel device. Patch 10
documents the result.

Without a tunnel device nothing decapsulates. enc_key_id reads the tunnel
header on the wire like on cxgb4 and hns3. On mlx5 the plain ip_proto and
dst_port keys select the match register. A rule on the uplink representor
used to ignore enc_key_id and now matches it or gets refused. I can rework
patch 9 on a dedicated key if that looks cleaner.

Tested on ConnectX-7 with firmware 28.48.1000 in NIC mode. Also tested on
the uplink and VF representors in dmfs, smfs and hmfs. This includes
redirect to a VF. GTP-U needs flex parser profile 3 and GENEVE profile 0.
The bnxt, cxgb4 and nfp patches are build tested only. The selftest passes
under User-Mode Linux and fails on a kernel without the mask.

Multiple AI tools were used to test this feature. They found the problems
fixed by patches 1 to 5.

The iproute2 support is posted separately.

[1] https://gtp-guard.org/articles/5g-upf-pump-up-the-volume/#benefits-of-range-partitioning
[2] https://github.com/acassen/research/tree/main/UPF-flow-steering

Alexandre Cassen (10):
  bnxt_en: require an exact tunnel id in TC flows
  cxgb4: flower: refuse a tunnel key wider than the VNI
  nfp: flower: refuse a tunnel key wider than the VNI
  net/mlx5e: TC, refuse a tunnel key wider than the VNI
  net/mlx5e: TC, reject a partial tunnel key with tunnel mapping
  net/sched: cls_flower: add enc_key_id mask
  selftests: forwarding: tc_flower: test the enc_key_id mask
  net/mlx5e: TC, offload RX queue mapping in NIC flows
  net/mlx5e: TC, match the tunnel key without a tunnel device
  docs: networking: describe tunnel key partitioning in TC queue
    filtering

 Documentation/netlink/specs/tc.yaml           |  4 +
 Documentation/networking/tc-queue-filters.rst | 22 +++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c  |  7 ++
 .../ethernet/chelsio/cxgb4/cxgb4_tc_flower.c  | 11 +++
 .../net/ethernet/mellanox/mlx5/core/Makefile  |  6 +-
 .../mellanox/mlx5/core/en/tc/act/act.c        |  1 +
 .../mellanox/mlx5/core/en/tc/act/act.h        |  1 +
 .../mellanox/mlx5/core/en/tc/act/rx_queue.c   | 49 +++++++++++
 .../ethernet/mellanox/mlx5/core/en/tc_tun.c   | 88 +++++++++++++++++++
 .../ethernet/mellanox/mlx5/core/en/tc_tun.h   | 14 +++
 .../mellanox/mlx5/core/en/tc_tun_geneve.c     | 46 ++++++++++
 .../mellanox/mlx5/core/en/tc_tun_gre.c        | 31 +++++++
 .../mellanox/mlx5/core/en/tc_tun_gtp.c        | 36 ++++++++
 .../mellanox/mlx5/core/en/tc_tun_vxlan.c      | 45 ++++++++++
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   | 19 ++++
 .../net/ethernet/mellanox/mlx5/core/en_tc.h   |  1 +
 .../ethernet/netronome/nfp/flower/offload.c   | 21 +++++
 include/uapi/linux/pkt_cls.h                  |  2 +
 net/sched/cls_flower.c                        |  5 +-
 .../selftests/net/forwarding/tc_flower.sh     | 57 ++++++++++++
 20 files changed, 462 insertions(+), 4 deletions(-)
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/rx_queue.c
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_gtp.c

-- 
2.43.0

[PATCH net-next 01/10] bnxt_en: require an exact tunnel id in TC flows

[Alexandre Cassen]

bnxt programs the value of enc_key_id and never reads its mask. Flower is
about to accept partial masks. bnxt would offload them as an exact match
on the masked key. Reject them like the other exact fields.

Found by an AI code review agent while auditing the users of enc_key_id.
Build tested only.

Signed-off-by: Alexandre Cassen <acassen@corp.free.fr>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
index c6b4a14c0e6e..e493c85ecf28 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
@@ -1081,6 +1081,13 @@ static bool bnxt_tc_can_offload(struct bnxt *bp, struct bnxt_tc_flow *flow)
 		return false;
 	}
 
+	/* Currently tunnel id cannot be partial wildcard */
+	if ((flow->flags & BNXT_TC_FLOW_FLAGS_TUNL_ID) &&
+	    flow->tun_mask.tun_id != key32_to_tunnel_id(cpu_to_be32(U32_MAX))) {
+		netdev_info(bp->dev, "Wildcard match unsupported for tunnel id\n");
+		return false;
+	}
+
 	/* Currently Ethertype must be set */
 	if (!is_exactmatch(&flow->l2_mask.ether_type,
 			   sizeof(flow->l2_mask.ether_type))) {
-- 
2.43.0

[PATCH net-next 02/10] cxgb4: flower: refuse a tunnel key wider than the VNI

[Alexandre Cassen]

cxgb4 holds a 24 bit VNI and truncates the key and the mask of enc_key_id.
An exact key above bit 23 already selects the wrong VNI. Flower is about
to accept partial masks. A mask that only covers bits above 23 would then
clear the VNI mask and encap_vld. The rule would match any traffic its
other keys allow. Refuse a key that no VNI can match.

Found by an AI code review agent while auditing the users of enc_key_id.
Build tested only.

Signed-off-by: Alexandre Cassen <acassen@corp.free.fr>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
index 3307e5042681..d9a509be5c51 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
@@ -316,6 +316,17 @@ static int cxgb4_validate_flow_match(struct netlink_ext_ack *extack,
 		return -EOPNOTSUPP;
 	}
 
+	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ENC_KEYID)) {
+		struct flow_match_enc_keyid match;
+
+		flow_rule_match_enc_keyid(rule, &match);
+		if (be32_to_cpu(match.key->keyid) >> ENCAP_VNI_BITWIDTH) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Tunnel key does not fit the 24 bit VNI");
+			return -EOPNOTSUPP;
+		}
+	}
+
 	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_BASIC)) {
 		struct flow_match_basic match;
 
-- 
2.43.0

Re: [PATCH net-next 02/10] cxgb4: flower: refuse a tunnel key wider than the VNI

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] cxgb4: unvalidated tunnel key mask in
  cxgb4_validate_flow_match()
- [Low] The commit message says the bug is already reachable today: "An
  exact key above bit 23 already selects the wrong VNI".

Pre-existing issues:
- [Medium] This bug was not introduced by this patch, but it produces the
  same symptom the commit message gives as its motivation ("The rule…

--

cxgb4: flower: refuse a tunnel key wider than the VNI

This patch adds a check to cxgb4_validate_flow_match(). Flower rules whose
enc_key_id key has bits set above bit 23 are now rejected. Before this, the
driver silently truncated such keys into the 24 bit vni bitfield.

> An exact key above bit 23 already selects the wrong VNI.

[Severity: Low]
The commit message says the truncation can already be hit today. Should
this carry a Fixes: tag, and possibly go through net?

At the baseline, cls_flower parses TCA_FLOWER_KEY_ENC_KEY_ID with
TCA_FLOWER_UNSPEC as the mask, so the mask is all ones. After that,
cxgb4_process_flow_match() stores the key and the mask into the 24 bit
fields:

drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c:cxgb4_process_flow_match() {
    ...
		fs->val.vni = be32_to_cpu(match.key->keyid);
		fs->mask.vni = be32_to_cpu(match.mask->keyid);
		if (fs->mask.vni) {
			fs->val.encap_vld = 1;
			fs->mask.encap_vld = 1;
		}
    ...
}

The field is declared as uint32_t vni:ENCAP_VNI_BITWIDTH in struct
ch_filter_tuple. So enc_key_id 0x01000005 becomes vni 0x5 with a vni mask
of 0xffffff. The rule's action then applies to VNI 5 traffic.

The truncation seems to date back to:

Fixes: 98f3697f8d41 ("cxgb4: add tc flower match support for tunnel VNI")

No later patch in the series touches drivers/net/ethernet/chelsio/, so
nothing else adds the tag.

> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
> index 3307e50426819..d9a509be5c517 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
> @@ -316,6 +316,17 @@ static int cxgb4_validate_flow_match(struct netlink_ext_ack *extack,
>  		return -EOPNOTSUPP;
>  	}
>  
> +	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ENC_KEYID)) {
> +		struct flow_match_enc_keyid match;
> +
> +		flow_rule_match_enc_keyid(rule, &match);
> +		if (be32_to_cpu(match.key->keyid) >> ENCAP_VNI_BITWIDTH) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Tunnel key does not fit the 24 bit VNI");
> +			return -EOPNOTSUPP;
> +		}
> +	}

[Severity: Medium]
Does this check cover the case the commit message describes? It only
looks at match.key->keyid. The hazard named in the changelog is a mask
that only covers bits above 23.

cls_flower stores the key already ANDed with the mask. So a rule with
enc_key_id 0 and a mask of 0xff000000 reaches
cxgb4_validate_flow_match() with match.key->keyid == 0 and
match.mask->keyid == cpu_to_be32(0xff000000). Shifting the key gives 0,
and the rule is accepted.

cxgb4_process_flow_match() then does:

	fs->mask.vni = be32_to_cpu(match.mask->keyid);

This truncates into the 24 bit vni bitfield of struct
ch_filter_tuple and leaves fs->mask.vni == 0. The if (fs->mask.vni)
block is skipped, so fs->val.encap_vld and fs->mask.encap_vld stay 0.
__cxgb4_set_filter() then never reaches t4_alloc_encap_mac_filt().

The hardware filter ends up with no tunnel constraint at all. It
matches any traffic its other keys allow, including non-encapsulated
packets. That is the failure mode the commit message says this patch
prevents, and it opens up once the later patch in the series lets
flower accept partial enc_key_id masks.

Simply testing match.mask->keyid >> ENCAP_VNI_BITWIDTH would not work
either. Exact matches today arrive with an all ones mask from
TCA_FLOWER_UNSPEC, so every existing rule would be rejected.

Would it make sense to also refuse a non-zero mask whose low
ENCAP_VNI_BITWIDTH bits are all zero? Or is there a better way to
express "match any VNI" for this hardware?

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. It
causes the same symptom the commit message names ("The rule would match
any traffic its other keys allow"), but it depends on the firmware
configuration.

validate_filter() in cxgb4_filter.c checks encap_vld only against fconf
VNIC_ID_F and the chip version:

	    unsupported(fconf, VNIC_ID_F, fs->val.encap_vld,
			fs->mask.encap_vld) ||
    ...
	if (fs->val.encap_vld &&
	    CHELSIO_CHIP_VERSION(adapter->params.chip) < CHELSIO_T6)
		return -EOPNOTSUPP;

It never checks that iconf (TP_INGRESS_CONFIG) has USE_ENC_IDX_F set.

In __cxgb4_set_filter(), the VNI is programmed only in the USE_ENC_IDX_F
branch:

	iconf = adapter->params.tp.ingress_config;
	if (iconf & VNIC_F) {
		f->fs.val.ovlan = (fs->val.pf << 13) | fs->val.vf;
		f->fs.mask.ovlan = (fs->mask.pf << 13) | fs->mask.vf;
		f->fs.val.ovlan_vld = fs->val.pfvf_vld;
		f->fs.mask.ovlan_vld = fs->mask.pfvf_vld;
	} else if (iconf & USE_ENC_IDX_F) {
		if (f->fs.val.encap_vld) {
			...
			ret = t4_alloc_encap_mac_filt(adapter, pi->viid,
			...

If iconf has VNIC_F, ovlan_vld is overwritten with pfvf_vld, which is 0
for a flower rule. If iconf has neither bit, nothing is programmed.

set_filter_wr() has no VNI field, so tunnel information reaches hardware
only through ovlan/ovlan_vld. In both cases the FW_FILTER_WR is sent
without any VNI constraint.

On a T6 with such a firmware config, could a rule like "flower skip_sw
enc_key_id 5 action drop" be reported as offloaded with a VNI match, but
drop all ingress traffic on the port that matches its other keys?

Flower doesn't reach the hash filter path for these rules, since
is_filter_exact_match() returns false when fs->mask.encap_vld is set.

One possible fix would be for validate_filter() to reject encap_vld
unless (iconf & USE_ENC_IDX_F).

No later patch in the series touches drivers/net/ethernet/chelsio/.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109131039.959438-1-acassen%40corp.free.fr

[PATCH net-next 03/10] nfp: flower: refuse a tunnel key wider than the VNI

[Alexandre Cassen]

VXLAN and GENEVE rules shift the key and the mask of enc_key_id into a 24
bit VNI. An exact key above bit 23 already selects the wrong VNI. Flower
is about to accept partial masks. A mask that only covers bits above 23
would then match any VNI. Refuse a key that no VNI can match. GRE keeps
its 32 bit key.

Found by an AI code review agent while auditing the users of enc_key_id.
Build tested only.

Signed-off-by: Alexandre Cassen <acassen@corp.free.fr>
---
 .../ethernet/netronome/nfp/flower/offload.c   | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index 161190200dbd..e5f6f4f464ad 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -150,6 +150,23 @@ static bool nfp_flower_check_higher_than_l3(struct flow_rule *rule)
 	       flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ICMP);
 }
 
+static int
+nfp_flower_check_tun_vni(struct flow_rule *rule,
+			 struct netlink_ext_ack *extack)
+{
+	struct flow_match_enc_keyid enc_keyid;
+
+	if (!flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ENC_KEYID))
+		return 0;
+
+	flow_rule_match_enc_keyid(rule, &enc_keyid);
+	if (be32_to_cpu(enc_keyid.key->keyid) < VXLAN_N_VID)
+		return 0;
+
+	NL_SET_ERR_MSG_MOD(extack, "unsupported offload: tunnel key does not fit the 24 bit VNI");
+	return -EOPNOTSUPP;
+}
+
 static int
 nfp_flower_calc_opt_layer(struct flow_dissector_key_enc_opts *enc_opts,
 			  u32 *key_layer_two, int *key_size, bool ipv6,
@@ -411,6 +428,10 @@ nfp_flower_calculate_key_layers(struct nfp_app *app,
 				NL_SET_ERR_MSG_MOD(extack, "unsupported offload: ingress netdev does not match the expected tunnel type");
 				return -EOPNOTSUPP;
 			}
+
+			err = nfp_flower_check_tun_vni(rule, extack);
+			if (err)
+				return err;
 		}
 	}
 
-- 
2.43.0

Re: [PATCH net-next 03/10] nfp: flower: refuse a tunnel key wider than the VNI

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In drivers/net/ethernet/netronome/nfp/flower/offload.c, the new
  nfp_flower_check_tun_vni() fixes a bug that already exists.

--

nfp: flower: refuse a tunnel key wider than the VNI

This patch adds nfp_flower_check_tun_vni() so that VXLAN and GENEVE flower
rules are refused when the enc_key_id key does not fit in the 24 bit VNI.
The check runs in nfp_flower_calculate_key_layers() after the UDP tunnel
type is known. GRE keeps its 32 bit key.

> diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
> index 161190200dbd0..e5f6f4f464ad6 100644
> --- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
> +++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
> @@ -150,6 +150,23 @@ static bool nfp_flower_check_higher_than_l3(struct flow_rule *rule)
>  	       flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ICMP);
>  }
>
> +static int
> +nfp_flower_check_tun_vni(struct flow_rule *rule,
> +			 struct netlink_ext_ack *extack)
> +{
> +	struct flow_match_enc_keyid enc_keyid;
> +
> +	if (!flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ENC_KEYID))
> +		return 0;
> +
> +	flow_rule_match_enc_keyid(rule, &enc_keyid);
> +	if (be32_to_cpu(enc_keyid.key->keyid) < VXLAN_N_VID)
> +		return 0;

[Severity: Low]
The commit message says "An exact key above bit 23 already selects the
wrong VNI", so this check also fixes a bug that exists today. Should the
patch carry a Fixes: tag and a stable Cc so that stable trees get it too?

Before cls_flower accepts an enc_key_id mask, fl_set_key() passes
TCA_FLOWER_UNSPEC as the mask attribute. That means any enc_key_id that is
present always gets an exact mask:

net/sched/cls_flower.c:fl_set_key() {
    ...
	fl_set_key_val(tb, &key->enc_key_id.keyid, TCA_FLOWER_KEY_ENC_KEY_ID,
		       &mask->enc_key_id.keyid, TCA_FLOWER_UNSPEC,
		       sizeof(key->enc_key_id.keyid));
    ...
}

nfp_flower_compile_tun_udp_key() then shifts the key and the mask left by
NFP_FL_TUN_VNI_OFFSET (8), which throws away bits 24-31:

drivers/net/ethernet/netronome/nfp/flower/match.c:nfp_flower_compile_tun_udp_key() {
    ...
		vni = be32_to_cpu((match.key->keyid & match.mask->keyid)) <<
		      NFP_FL_TUN_VNI_OFFSET;
		*key |= cpu_to_be32(vni);
		vni = be32_to_cpu(match.mask->keyid) << NFP_FL_TUN_VNI_OFFSET;
		*key_msk |= cpu_to_be32(vni);
    ...
}

So on current and stable kernels, a VXLAN or GENEVE rule with enc_key_id
0x01000005 gets programmed into the NIC as VNI 5 with mask 0xffffff00. The
hardware then matches VNI 5 traffic. In software the rule never matches,
because VXLAN and GENEVE tunnel ids are only 24 bits wide.

The Fixes: tag would need to point at the commit that first added
VXLAN/GENEVE VNI matching to nfp flower. Commit 77a5f73a8b33 ("nfp: flower:
move udp tunnel key match compilation to helper function") only moved this
code and says it makes no functional change, so the original commit is
older than that one.

> +
> +	NL_SET_ERR_MSG_MOD(extack, "unsupported offload: tunnel key does not fit the 24 bit VNI");
> +	return -EOPNOTSUPP;
> +}
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109131039.959438-1-acassen%40corp.free.fr

[PATCH net-next 04/10] net/mlx5e: TC, refuse a tunnel key wider than the VNI

[Alexandre Cassen]

VXLAN and GENEVE flows truncate the key and the mask of enc_key_id to a 24
bit VNI. An exact key above bit 23 already selects the wrong VNI. Flower
is about to accept partial masks. A mask that only covers bits above 23
would then match any VNI. Refuse a key that no VNI can match. GRE keeps
its 32 bit key.

Found by an AI code review agent while auditing the users of enc_key_id.
Tested on ConnectX-7 with firmware 28.48.1000 in switchdev mode on VXLAN,
GENEVE and gretap devices.

Signed-off-by: Alexandre Cassen <acassen@corp.free.fr>
---
 drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h       | 3 +++
 .../net/ethernet/mellanox/mlx5/core/en/tc_tun_geneve.c    | 8 ++++++++
 drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c | 8 ++++++++
 3 files changed, 19 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h
index 6873c1201803..d7c778bbb2ef 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h
@@ -13,6 +13,9 @@
 
 #ifdef CONFIG_MLX5_ESWITCH
 
+/* VXLAN and GENEVE match registers hold a 24 bit VNI */
+#define MLX5E_TC_TUN_N_VNI BIT(24)
+
 enum {
 	MLX5E_TC_TUNNEL_TYPE_UNKNOWN,
 	MLX5E_TC_TUNNEL_TYPE_VXLAN,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_geneve.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_geneve.c
index bf969212cc77..ee04fcdca6ff 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_geneve.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_geneve.c
@@ -141,6 +141,14 @@ static int mlx5e_tc_tun_parse_geneve_vni(struct mlx5e_priv *priv,
 	if (!enc_keyid.mask->keyid)
 		return 0;
 
+	if (be32_to_cpu(enc_keyid.key->keyid) >= MLX5E_TC_TUN_N_VNI) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Tunnel key does not fit the 24 bit VNI");
+		netdev_warn(priv->netdev,
+			    "Tunnel key does not fit the 24 bit VNI\n");
+		return -EOPNOTSUPP;
+	}
+
 	if (!MLX5_CAP_ESW_FLOWTABLE_FDB(priv->mdev, ft_field_support.outer_geneve_vni)) {
 		NL_SET_ERR_MSG_MOD(extack, "Matching on GENEVE VNI is not supported");
 		netdev_warn(priv->netdev, "Matching on GENEVE VNI is not supported\n");
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
index 467fbe43b89e..33911e72d20d 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
@@ -173,6 +173,14 @@ static int mlx5e_tc_tun_parse_vxlan(struct mlx5e_priv *priv,
 	if (!enc_keyid.mask->keyid)
 		return 0;
 
+	if (be32_to_cpu(enc_keyid.key->keyid) >= MLX5E_TC_TUN_N_VNI) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Tunnel key does not fit the 24 bit VNI");
+		netdev_warn(priv->netdev,
+			    "Tunnel key does not fit the 24 bit VNI\n");
+		return -EOPNOTSUPP;
+	}
+
 	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ENC_OPTS)) {
 		int err;
 
-- 
2.43.0

Re: [PATCH net-next 04/10] net/mlx5e: TC, refuse a tunnel key wider than the VNI

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message says this is a fix for a bug that exists
  today: "An exact key above bit 23 already selects the wrong VNI." The…

--

net/mlx5e: TC, refuse a tunnel key wider than the VNI

This patch makes mlx5e_tc_tun_parse_vxlan() and
mlx5e_tc_tun_parse_geneve_vni() reject an enc_key_id whose key does not
fit in the 24 bit VNI match field. Such rules now fail with -EOPNOTSUPP
and an extack message. The 32 bit GRE key handling is left as is.

> VXLAN and GENEVE flows truncate the key and the mask of enc_key_id to a 24
> bit VNI. An exact key above bit 23 already selects the wrong VNI. Flower
> is about to accept partial masks. A mask that only covers bits above 23
> would then match any VNI. Refuse a key that no VNI can match. GRE keeps
> its 32 bit key.

[ ... ]

> Signed-off-by: Alexandre Cassen <acassen@corp.free.fr>

[Severity: Low]
The commit message says the problem exists today ("An exact key above bit
23 already selects the wrong VNI"). Should the patch carry a Fixes: tag,
and maybe go through net?

The truncation does look reachable in the current tree. Flower parses
TCA_FLOWER_KEY_ENC_KEY_ID with TCA_FLOWER_UNSPEC as the mask type, so every
enc_key_id rule is an exact 32 bit match. The parsers then write the key
into 24 bit fields:

mlx5e_tc_tun_parse_vxlan() {
    ...
	MLX5_SET(fte_match_set_misc, misc_v, vxlan_vni,
		 be32_to_cpu(enc_keyid.key->keyid));
    ...
}

mlx5e_tc_tun_parse_geneve_vni() {
    ...
	MLX5_SET(fte_match_set_misc, misc_v, geneve_vni, be32_to_cpu(enc_keyid.key->keyid));
    ...
}

In mlx5_ifc.h, vxlan_vni and geneve_vni are 0x18 bits wide, so MLX5_SET()
masks off bits 24-31.

The VXLAN-GBP path in mlx5e_tc_tun_parse_vxlan() also loses those bits. It
shifts the key left by 8 into the 32 bit tunnel_header_1:

	MLX5_SET(fte_match_set_misc5, misc_v, tunnel_header_1,
		 be32_to_cpu(enc_keyid.key->keyid) << 8);

As a result, a skip_sw rule with enc_key_id 0x01000001 is offloaded as a
match on VNI 1. Software never matches that rule.

Because this fixes a mismatch that already exists, a Fixes: tag would tell
backporters which stable trees are affected. These look like the
candidate targets:

  9272e3df3023 ("net/mlx5e: Geneve, Add support for encap/decap flows
  offload") for the geneve_vni MLX5_SET()

  the original en_tc.c commit that added VXLAN VNI matching, for the
  vxlan_vni MLX5_SET(). d386939a327d ("net/mlx5e: Rearrange tc tunnel code
  in a modular way") later moved that code into tc_tun_vxlan.c.

  eacc77a73275 ("net/mlx5e: Use custom tunnel header for vxlan gbp") for
  the tunnel_header_1 path

Would it also make sense to send this to net as a standalone fix, rather
than untagged as part of the net-next series that adds flower partial
masks?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109131039.959438-1-acassen%40corp.free.fr

[PATCH net-next 05/10] net/mlx5e: TC, reject a partial tunnel key with tunnel mapping

[Alexandre Cassen]

Tunnel mapping stores the matched tunnel key. The metadata of a packet
missing in hardware after a goto chain or a sample is restored from it.
A partial enc_key_id mask would restore a wrong tunnel id. Flower is about
to accept such masks.

Tunnel options with a partial class or type are already rejected here.
Rules without tunnel mapping keep working.

Found by an AI code review agent while auditing the users of enc_key_id.
Tested on ConnectX-7 with firmware 28.48.1000 in switchdev mode.

Signed-off-by: Alexandre Cassen <acassen@corp.free.fr>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 708b7987ea4c..28f09d234835 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -2257,6 +2257,7 @@ static int mlx5e_get_flow_tunnel_id(struct mlx5e_priv *priv,
 	struct tunnel_match_enc_opts tun_enc_opts;
 	struct mlx5_rep_uplink_priv *uplink_priv;
 	struct mlx5_flow_attr *attr = flow->attr;
+	struct flow_match_enc_keyid enc_keyid;
 	struct mlx5e_rep_priv *uplink_rpriv;
 	struct tunnel_match_key tunnel_key;
 	bool enc_opts_is_dont_care = true;
@@ -2265,6 +2266,16 @@ static int mlx5e_get_flow_tunnel_id(struct mlx5e_priv *priv,
 	u32 value, mask;
 	int err;
 
+	/* The mapping restores the tunnel id of a packet missing in hardware */
+	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ENC_KEYID)) {
+		flow_rule_match_enc_keyid(rule, &enc_keyid);
+		if (enc_keyid.mask->keyid != cpu_to_be32(U32_MAX)) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Partial match of tunnel key in chains isn't supported");
+			return -EOPNOTSUPP;
+		}
+	}
+
 	esw = priv->mdev->priv.eswitch;
 	uplink_rpriv = mlx5_eswitch_get_uplink_priv(esw, REP_ETH);
 	uplink_priv = &uplink_rpriv->uplink_priv;
-- 
2.43.0

Re: [PATCH net-next 05/10] net/mlx5e: TC, reject a partial tunnel key with tunnel mapping

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new check in mlx5e_get_flow_tunnel_id()
  (drivers/net/ethernet/mellanox/mlx5/core/en_tc.c) rejects any…

--

net/mlx5e: TC, reject a partial tunnel key with tunnel mapping

This commit adds a check to mlx5e_get_flow_tunnel_id() that rejects an
enc_key_id mask other than all-ones when tunnel mapping is used. The
mapping is used to restore tunnel metadata for packets that miss in
hardware after a goto chain or a sample. This is preparation for flower
accepting partial enc_key_id masks.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index 708b7987ea4cd..28f09d234835d 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c

[ ... ]

> @@ -2265,6 +2266,16 @@ static int mlx5e_get_flow_tunnel_id(struct mlx5e_priv *priv,
>  	u32 value, mask;
>  	int err;
>  
> +	/* The mapping restores the tunnel id of a packet missing in hardware */
> +	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ENC_KEYID)) {
> +		flow_rule_match_enc_keyid(rule, &enc_keyid);
> +		if (enc_keyid.mask->keyid != cpu_to_be32(U32_MAX)) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Partial match of tunnel key in chains isn't supported");
> +			return -EOPNOTSUPP;
> +		}
> +	}

[Severity: Low]
Should the full-mask width here depend on the tunnel type?

On VXLAN and GENEVE the tunnel key is a 24-bit VNI. The parsers in the
same driver already refuse wider keys and program the 24-bit match
fields:

drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c:mlx5e_tc_tun_parse_vxlan() {
    ...
	if (be32_to_cpu(enc_keyid.key->keyid) >= MLX5E_TC_TUN_N_VNI) {
    ...
	MLX5_SET(fte_match_set_misc, misc_c, vxlan_vni,
		 be32_to_cpu(enc_keyid.mask->keyid));
    ...
}

mlx5e_tc_tun_parse_geneve_vni() does the same with geneve_vni.

That means a mask of 0x00ffffff programs the same hardware criteria as
0xffffffff. The masked key stored in the tunnel mapping is also equal to
the full VNI, so mlx5e_tc_restore_tunnel() would restore the correct
tunnel id.

Once the later series commit "net/sched: cls_flower: add enc_key_id mask"
lets userspace send a mask, a rule like enc_key_id N/0xffffff on a VXLAN
or GENEVE device would be refused with -EOPNOTSUPP here. This happens in
any chain > 0, and in chain 0 with goto or sample. Is that intended?

The extack text "Partial match of tunnel key in chains isn't supported"
also seems misleading for that case, because the mask covers the whole
VNI.

The earlier series commit "net/mlx5e: TC, refuse a tunnel key wider than
the VNI" treats 24 bits as the full key for VXLAN and GENEVE, but this
check uses 32 bits. Only GRE, with its 32-bit gre_key, seems to need a
full 32-bit mask. This check is still the same at the end of the series.

> +
>  	esw = priv->mdev->priv.eswitch;
>  	uplink_rpriv = mlx5_eswitch_get_uplink_priv(esw, REP_ETH);
>  	uplink_priv = &uplink_rpriv->uplink_priv;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109131039.959438-1-acassen%40corp.free.fr

[PATCH net-next 06/10] net/sched: cls_flower: add enc_key_id mask

[Alexandre Cassen]

Flower only matches enc_key_id exactly. A block of tunnel keys takes one
filter per key. Add an optional mask. Without one the match stays exact.

  tc filter add dev <dev> ingress protocol ip flower \
      enc_key_id 0x100/0xf00 action ...

cxgb4, mlx5 and nfp program the mask. sfc does when its firmware supports
it. hns3 and ice reject a partial one. The previous patches make bnxt and
mlx5 tunnel mapping reject it too. i40e and iavf never program the value
of enc_key_id, with or without this patch.

Hardware holding a 24 bit VNI truncates the mask too. This is correct when
the key fits the VNI. The previous patches enforce this on cxgb4, nfp and
mlx5.

Signed-off-by: Alexandre Cassen <acassen@corp.free.fr>
---
 Documentation/netlink/specs/tc.yaml | 4 ++++
 include/uapi/linux/pkt_cls.h        | 2 ++
 net/sched/cls_flower.c              | 5 +++--
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/Documentation/netlink/specs/tc.yaml b/Documentation/netlink/specs/tc.yaml
index b6ccc645b0e3..a20fa42e6715 100644
--- a/Documentation/netlink/specs/tc.yaml
+++ b/Documentation/netlink/specs/tc.yaml
@@ -2908,6 +2908,10 @@ attribute-sets:
         byte-order: big-endian
         enum: flower-key-ctrl-flags
         enum-as-flags: true
+      -
+        name: key-enc-key-id-mask
+        type: u32
+        byte-order: big-endian
   -
     name: flower-key-enc-opts-attrs
     name-prefix: tca-flower-key-enc-opts-
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 28d94b11d1aa..fcaf06c6892d 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -560,6 +560,8 @@ enum {
 	TCA_FLOWER_KEY_ENC_FLAGS,	/* be32 */
 	TCA_FLOWER_KEY_ENC_FLAGS_MASK,	/* be32 */
 
+	TCA_FLOWER_KEY_ENC_KEY_ID_MASK,	/* be32 */
+
 	__TCA_FLOWER_MAX,
 };
 
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 23eba4a00a94..dd248a335402 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -661,6 +661,7 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
 	[TCA_FLOWER_KEY_VLAN_PRIO]	= { .type = NLA_U8 },
 	[TCA_FLOWER_KEY_VLAN_ETH_TYPE]	= { .type = NLA_U16 },
 	[TCA_FLOWER_KEY_ENC_KEY_ID]	= { .type = NLA_U32 },
+	[TCA_FLOWER_KEY_ENC_KEY_ID_MASK] = { .type = NLA_U32 },
 	[TCA_FLOWER_KEY_ENC_IPV4_SRC]	= { .type = NLA_U32 },
 	[TCA_FLOWER_KEY_ENC_IPV4_SRC_MASK] = { .type = NLA_U32 },
 	[TCA_FLOWER_KEY_ENC_IPV4_DST]	= { .type = NLA_U32 },
@@ -2110,7 +2111,7 @@ static int fl_set_key(struct net *net, struct nlattr *tca_opts,
 	}
 
 	fl_set_key_val(tb, &key->enc_key_id.keyid, TCA_FLOWER_KEY_ENC_KEY_ID,
-		       &mask->enc_key_id.keyid, TCA_FLOWER_UNSPEC,
+		       &mask->enc_key_id.keyid, TCA_FLOWER_KEY_ENC_KEY_ID_MASK,
 		       sizeof(key->enc_key_id.keyid));
 
 	fl_set_key_val(tb, &key->enc_tp.src, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT,
@@ -3680,7 +3681,7 @@ static int fl_dump_key(struct sk_buff *skb, struct net *net,
 		goto nla_put_failure;
 
 	if (fl_dump_key_val(skb, &key->enc_key_id, TCA_FLOWER_KEY_ENC_KEY_ID,
-			    &mask->enc_key_id, TCA_FLOWER_UNSPEC,
+			    &mask->enc_key_id, TCA_FLOWER_KEY_ENC_KEY_ID_MASK,
 			    sizeof(key->enc_key_id)) ||
 	    fl_dump_key_val(skb, &key->enc_tp.src,
 			    TCA_FLOWER_KEY_ENC_UDP_SRC_PORT,
-- 
2.43.0

Re: [PATCH net-next 06/10] net/sched: cls_flower: add enc_key_id mask

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] net/sched: cls_flower: 24-bit VNI mask truncation in
  fl_set_key() offload

Pre-existing issues:
- [Medium] i40e and iavf accept an enc_key_id match but never program the
  tenant ID into hardware.

--

net/sched: cls_flower: add enc_key_id mask

This adds an optional TCA_FLOWER_KEY_ENC_KEY_ID_MASK attribute to flower,
so one filter can match a block of tunnel keys. When no mask is given, the
enc_key_id match stays exact. The new attribute is also added to the policy,
the dump path and the tc.yaml netlink spec.

> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index 23eba4a00a947..dd248a335402a 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c

[ ... ]

> @@ -2110,7 +2111,7 @@ static int fl_set_key(struct net *net, struct nlattr *tca_opts,
>  	}
>  
>  	fl_set_key_val(tb, &key->enc_key_id.keyid, TCA_FLOWER_KEY_ENC_KEY_ID,
> -		       &mask->enc_key_id.keyid, TCA_FLOWER_UNSPEC,
> +		       &mask->enc_key_id.keyid, TCA_FLOWER_KEY_ENC_KEY_ID_MASK,
>  		       sizeof(key->enc_key_id.keyid));

[Severity: Medium]
The commit message says truncating the mask to a 24 bit VNI is correct
when the key fits the VNI. The earlier patches in the series only check
the key. Doesn't that miss a mask with no bits set in the low 24 bits?

Take enc_key_id 0x0/0xff000000. It passes the check in
cxgb4_validate_flow_match(), because be32_to_cpu(match.key->keyid) >>
ENCAP_VNI_BITWIDTH is 0. cxgb4_process_flow_match() then stores
be32_to_cpu(match.mask->keyid) into the 24 bit fs->mask.vni field, which
ends up as 0.

fs->val.encap_vld and fs->mask.encap_vld are only set under
if (fs->mask.vni). So the rule is programmed without the encapsulation
qualifier, and t4_alloc_encap_mac_filt() is skipped. The hardware filter
can then match non-encapsulated traffic on the port, which the user
never asked for.

mlx5e_tc_tun_parse_vxlan() has the same shape. It only bails out when
enc_keyid.mask->keyid is zero, and only checks the key against
MLX5E_TC_TUN_N_VNI. It then writes the truncated mask into vxlan_vni in
misc_c. That gives a zero VNI mask while MLX5_MATCH_MISC_PARAMETERS is
still set in spec->match_criteria_enable, so the VNI is no longer
constrained at all. The same applies to
mlx5e_tc_tun_parse_outer_key_vxlan(), added earlier in this series.

Before this patch the mask was always all ones whenever enc_key_id was
present, so the low 24 bits could never be zero. This patch is what
makes the mismatch reachable from userspace.

Should these drivers reject a mask whose low 24 bits are all zero? Or
should cxgb4 base encap_vld on the full 32 bit mask rather than the
truncated fs->mask.vni? Either way, it looks like the "key fits the VNI"
checks from the earlier patches need to look at the mask as well.

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. With
this change, partial masks can now reach i40e and iavf. Neither driver
programs the tenant ID into hardware, yet both report the offload as
successful.

In i40e_parse_cls_flower(), any non-zero mask, including a partial one,
sets the tenant ID field flag:

drivers/net/ethernet/intel/i40e/i40e_main.c:i40e_parse_cls_flower() {
    ...
	if (match.mask->keyid != 0)
		field_flags |= I40E_CLOUD_FIELD_TEN_ID;

	filter->tenant_id = be32_to_cpu(match.key->keyid);
    ...
}

But i40e_set_cld_element() memsets the AQ element and never writes
cld->tenant_id:

drivers/net/ethernet/intel/i40e/i40e_main.c:i40e_set_cld_element() {
    ...
	/* tenant_id is not supported by FW now, once the support is enabled
	 * fill the cld->tenant_id with cpu_to_le32(filter->tenant_id)
	 */
	if (filter->tenant_id)
		return;
}

iavf_parse_cls_flower() has the same problem. It sets
IAVF_CLOUD_FIELD_TEN_ID for any non-zero mask but never sends a tenant ID
value to the PF.

As a result, the hardware filter installed on these drivers does not match
the requested tunnel key, whether the mask is exact or partial. An exact key
is ignored in the same way today, so a partial mask does not add a new kind
of mismatch.

The commit message already says this accurately:

  "i40e and iavf never program the value of enc_key_id, with or without
  this patch."

Should i40e_parse_cls_flower() and iavf_parse_cls_flower() reject an
enc_key_id match with -EOPNOTSUPP, at least until firmware support for the
tenant ID exists? That would stop the offload from appearing to succeed
while ignoring the key. It would probably fit best as a separate fix.

>  
>  	fl_set_key_val(tb, &key->enc_tp.src, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT,

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109131039.959438-1-acassen%40corp.free.fr

[PATCH net-next 07/10] selftests: forwarding: tc_flower: test the enc_key_id mask

[Alexandre Cassen]

A filter on a block of tunnel keys must match a key inside the block and
no key outside it. It must also dump its mask. A second filter proves that
the frame outside the block reaches the device.

When tc does not know the mask or a device refuses it under skip_sw, the
test only logs this. Reporting a skip would turn the exit status of the
whole script into a skip.

Signed-off-by: Alexandre Cassen <acassen@corp.free.fr>
---
 .../selftests/net/forwarding/tc_flower.sh     | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/tools/testing/selftests/net/forwarding/tc_flower.sh b/tools/testing/selftests/net/forwarding/tc_flower.sh
index b58909a93112..9d9f109ab895 100755
--- a/tools/testing/selftests/net/forwarding/tc_flower.sh
+++ b/tools/testing/selftests/net/forwarding/tc_flower.sh
@@ -3,6 +3,7 @@
 
 ALL_TESTS="match_dst_mac_test match_src_mac_test match_dst_ip_test \
 	match_src_ip_test match_ip_flags_test match_pcp_test match_vlan_test \
+	match_enc_key_id_mask_test \
 	match_ip_tos_test match_indev_test match_ip_ttl_test
 	match_mpls_label_test \
 	match_mpls_tc_test match_mpls_bos_test match_mpls_ttl_test \
@@ -280,6 +281,62 @@ match_vlan_test()
 	log_test "VLAN match ($tcflags)"
 }
 
+enc_key_id_mask_check()
+{
+	tc filter show dev gt-ex ingress pref 1 | grep -q "enc_key_id 256/0xf00"
+	check_err $? "Did not dump the mask"
+
+	$MZ gt1 -c 1 -p 64 -a "$(mac_get gt1)" -b $h2mac -t ip -q
+
+	tc_check_packets "dev gt-ex ingress" 101 1
+	check_err $? "Did not match a key inside the mask"
+
+	$MZ gt2 -c 1 -p 64 -a "$(mac_get gt2)" -b $h2mac -t ip -q
+
+	tc_check_packets "dev gt-ex ingress" 102 1
+	check_err $? "Did not receive the key outside the mask"
+
+	tc_check_packets "dev gt-ex ingress" 101 1
+	check_err $? "Matched a key outside the mask"
+
+	log_test "enc_key_id mask match ($tcflags)"
+}
+
+match_enc_key_id_mask_test()
+{
+	RET=0
+
+	if ! tc filter add dev $h2 ingress flower help 2>&1 | grep -q MASKED-KEY-ID; then
+		echo "SKIP: iproute2 too old; tc is missing the enc_key_id mask"
+		return 0
+	fi
+
+	tunnel_create gt1 gretap 192.0.2.1 192.0.2.2 dev $h1 key 300
+	tunnel_create gt2 gretap 192.0.2.1 192.0.2.2 dev $h1 key 600
+	ip link set dev gt1 master v$h1
+	ip link set dev gt2 master v$h1
+	ip link add gt-ex type gretap external # To collect tunnel info
+	ip link set dev gt-ex up
+	ip link set dev gt-ex master v$h2
+	tc qdisc add dev gt-ex clsact
+
+	# A device may refuse a partial mask but software must take it
+	tc filter add dev gt-ex ingress protocol ip pref 1 handle 101 flower \
+		$tcflags enc_key_id 256/0xf00 action drop
+	if [[ $? -ne 0 && $tcflags == "skip_sw" ]]; then
+		log_info "Device refuses the enc_key_id mask"
+	else
+		tc filter add dev gt-ex ingress protocol ip pref 2 handle 102 \
+			flower $tcflags enc_key_id 600 action drop
+		enc_key_id_mask_check
+	fi
+
+	tc qdisc del dev gt-ex clsact
+	tunnel_destroy gt-ex
+	tunnel_destroy gt2
+	tunnel_destroy gt1
+}
+
 match_ip_tos_test()
 {
 	RET=0
-- 
2.43.0

Re: [PATCH net-next 07/10] selftests: forwarding: tc_flower: test the enc_key_id mask

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] When tcflags=skip_sw, match_enc_key_id_mask_test()
  (tools/testing/selftests/net/forwarding/tc_flower.sh) never tests…

--

selftests: forwarding: tc_flower: test the enc_key_id mask

This adds match_enc_key_id_mask_test() to tc_flower.sh. The test puts a
flower filter with enc_key_id 256/0xf00 on a gretap external device. It
checks three things: the mask shows up in the dump, a key inside the mask
matches, and a key outside the mask only hits a second exact-match filter.
Under skip_sw, if adding the masked filter fails, the test only logs it.

> diff --git a/tools/testing/selftests/net/forwarding/tc_flower.sh b/tools/testing/selftests/net/forwarding/tc_flower.sh
> index b58909a931129..9d9f109ab8954 100755
> --- a/tools/testing/selftests/net/forwarding/tc_flower.sh
> +++ b/tools/testing/selftests/net/forwarding/tc_flower.sh

[ ... ]

> +match_enc_key_id_mask_test()
> +{

[ ... ]

> +	ip link add gt-ex type gretap external # To collect tunnel info
> +	ip link set dev gt-ex up
> +	ip link set dev gt-ex master v$h2
> +	tc qdisc add dev gt-ex clsact
> +
> +	# A device may refuse a partial mask but software must take it
> +	tc filter add dev gt-ex ingress protocol ip pref 1 handle 101 flower \
> +		$tcflags enc_key_id 256/0xf00 action drop
> +	if [[ $? -ne 0 && $tcflags == "skip_sw" ]]; then
> +		log_info "Device refuses the enc_key_id mask"

[Severity: Medium]
Can the skip_sw pass ever reach enc_key_id_mask_check(), or does it always
end up in this branch?

gt-ex is a gretap external device, and ip_gre has no ndo_setup_tc. When
clsact is added on it, tcf_block_offload_bind() calls
tcf_block_offload_cmd(). That function sets up the indirect block and then
always returns an error:

net/sched/cls_api.c:tcf_block_offload_cmd() {
    ...
	flow_indr_dev_setup_offload(dev, sch, TC_SETUP_BLOCK, block, &bo,
				    tc_block_indr_cleanup);
	tcf_block_setup(block, &bo);

	return -EOPNOTSUPP;
}

As a result, tcf_block_offload_bind() does block->nooffloaddevcnt++ for
the gt-ex block.

After that, fl_hw_replace_filter() calls tc_setup_cb_add() with
err_stop set to skip_sw. tc_setup_cb_add() then rejects the filter before
any driver callback is called:

net/sched/cls_api.c:tc_setup_cb_add() {
    ...
	if (block->nooffloaddevcnt && err_stop) {
		ok_count = -EOPNOTSUPP;
		goto err_unlock;
	}
    ...
}

So every skip_sw flower filter on gt-ex would fail, whatever the mask is.
An exact enc_key_id 256 would fail the same way.

On any setup where tc_offload_check passes, the skip_sw pass would print
"Device refuses the enc_key_id mask". It would then skip filter 102 and
enc_key_id_mask_check(), and report no result.

The comment above, the log_info() text and the commit message all say
the mask is the reason:

  When tc does not know the mask or a device refuses it under skip_sw, the
  test only logs this.

Is that accurate, given that no skip_sw filter can be installed on gt-ex
at all?

This also means the skip_sw half never tests the hardware mask
programming that this series adds to cxgb4, mlx5 and nfp. A regression
there would look like the same info line.

Even without the block-level check, nfp_flower_calculate_key_layers()
looks like it would reject this filter for another reason: it matches only
enc_key_id, with no ENC_CONTROL or enc_dst_ip match. Again, that has
nothing to do with the mask.

Should the skip_sw case be dropped, or moved to a block that can actually
be offloaded?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109131039.959438-1-acassen%40corp.free.fr

[PATCH net-next 08/10] net/mlx5e: TC, offload RX queue mapping in NIC flows

[Alexandre Cassen]

skbedit queue_mapping steers a flow to one RX queue. ice, hns3 and
octeontx2 already offload it. Add it to mlx5e NIC flows. The rule points
to the direct TIR of the channel like ethtool steering rules.

The direct TIR takes over the destination of the rule. The action must
come last and cannot follow goto or mirred. A goto would also unbalance
the reference count of its chain table.

Reducing the channel count below the queue of a rule drops its traffic
until the channel returns. The direct RQT of an inactive channel points to
the drop RQ. ethtool refuses such a change for its own rules but nothing
tracks the queues of TC rules. A channel change racing with the range
check of a new rule has the same result. Direct TIRs exist up to the
maximum channel count. The rule always points to a valid one.

Tested on ConnectX-7 with firmware 28.48.1000.

Signed-off-by: Alexandre Cassen <acassen@corp.free.fr>
---
 .../net/ethernet/mellanox/mlx5/core/Makefile  |  3 +-
 .../mellanox/mlx5/core/en/tc/act/act.c        |  1 +
 .../mellanox/mlx5/core/en/tc/act/act.h        |  1 +
 .../mellanox/mlx5/core/en/tc/act/rx_queue.c   | 49 +++++++++++++++++++
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   |  4 ++
 .../net/ethernet/mellanox/mlx5/core/en_tc.h   |  1 +
 6 files changed, 58 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/rx_queue.c

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Makefile b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
index a1871259210d..f8d1696ab5eb 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Makefile
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
@@ -56,7 +56,8 @@ mlx5_core-$(CONFIG_MLX5_CLS_ACT)     += en/tc/act/act.o en/tc/act/drop.o en/tc/a
 					en/tc/act/vlan.o en/tc/act/vlan_mangle.o en/tc/act/mpls.o \
 					en/tc/act/mirred.o en/tc/act/mirred_nic.o \
 					en/tc/act/ct.o en/tc/act/sample.o en/tc/act/ptype.o \
-					en/tc/act/redirect_ingress.o en/tc/act/police.o
+					en/tc/act/redirect_ingress.o en/tc/act/police.o \
+					en/tc/act/rx_queue.o
 
 ifneq ($(CONFIG_MLX5_TC_CT),)
 	mlx5_core-y			     += en/tc_ct.o en/tc/ct_fs_dmfs.o
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/act.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/act.c
index 0380a04c3691..9b5d7d5ee271 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/act.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/act.c
@@ -41,6 +41,7 @@ static struct mlx5e_tc_act *tc_acts_nic[NUM_FLOW_ACTIONS] = {
 	[FLOW_ACTION_ADD] = &mlx5e_tc_act_pedit,
 	[FLOW_ACTION_CSUM] = &mlx5e_tc_act_csum,
 	[FLOW_ACTION_MARK] = &mlx5e_tc_act_mark,
+	[FLOW_ACTION_RX_QUEUE_MAPPING] = &mlx5e_tc_act_rx_queue,
 	[FLOW_ACTION_CT] = &mlx5e_tc_act_ct,
 };
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/act.h b/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/act.h
index 2e528b2c34d6..e6df5f0f27a7 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/act.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/act.h
@@ -94,6 +94,7 @@ extern struct mlx5e_tc_act mlx5e_tc_act_sample;
 extern struct mlx5e_tc_act mlx5e_tc_act_ptype;
 extern struct mlx5e_tc_act mlx5e_tc_act_redirect_ingress;
 extern struct mlx5e_tc_act mlx5e_tc_act_police;
+extern struct mlx5e_tc_act mlx5e_tc_act_rx_queue;
 
 struct mlx5e_tc_act *
 mlx5e_tc_act_get(enum flow_action_id act_id,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/rx_queue.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/rx_queue.c
new file mode 100644
index 000000000000..869da3cb703a
--- /dev/null
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/rx_queue.c
@@ -0,0 +1,49 @@
+// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
+
+#include "act.h"
+#include "en/tc_priv.h"
+#include "en/rx_res.h"
+
+static bool
+tc_act_can_offload_rx_queue(struct mlx5e_tc_act_parse_state *parse_state,
+			    const struct flow_action_entry *act,
+			    int act_index,
+			    struct mlx5_flow_attr *attr)
+{
+	struct netlink_ext_ack *extack = parse_state->extack;
+	struct mlx5e_tc_flow *flow = parse_state->flow;
+
+	/* The direct TIR takes over the destination of the rule */
+	if (!flow_action_is_last_entry(parse_state->flow_action, act) ||
+	    attr->dest_chain || flow_flag_test(flow, HAIRPIN)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "RX queue mapping must be the last action and cannot follow goto or mirred");
+		return false;
+	}
+
+	if (act->rx_queue >= flow->priv->channels.params.num_channels) {
+		NL_SET_ERR_MSG_MOD(extack, "RX queue index out of range");
+		return false;
+	}
+
+	return true;
+}
+
+static int
+tc_act_parse_rx_queue(struct mlx5e_tc_act_parse_state *parse_state,
+		      const struct flow_action_entry *act,
+		      struct mlx5e_priv *priv,
+		      struct mlx5_flow_attr *attr)
+{
+	attr->nic_attr->rx_tirn = mlx5e_rx_res_get_tirn_direct(priv->rx_res,
+							       act->rx_queue);
+	attr->action |= MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
+
+	return 0;
+}
+
+struct mlx5e_tc_act mlx5e_tc_act_rx_queue = {
+	.can_offload = tc_act_can_offload_rx_queue,
+	.parse_action = tc_act_parse_rx_queue,
+	.is_terminating_action = true,
+};
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 28f09d234835..3823ccc60dd3 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -1267,6 +1267,10 @@ mlx5e_add_offloaded_nic_rule(struct mlx5e_priv *priv,
 		dest[dest_ix].type = MLX5_FLOW_DESTINATION_TYPE_TIR;
 		dest[dest_ix].tir_num = nic_attr->hairpin_tirn;
 		dest_ix++;
+	} else if (nic_attr->rx_tirn) {
+		dest[dest_ix].type = MLX5_FLOW_DESTINATION_TYPE_TIR;
+		dest[dest_ix].tir_num = nic_attr->rx_tirn;
+		dest_ix++;
 	} else if (attr->action & MLX5_FLOW_CONTEXT_ACTION_FWD_DEST) {
 		dest[dest_ix].type = MLX5_FLOW_DESTINATION_TYPE_FLOW_TABLE;
 		if (attr->dest_chain) {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h
index e1b8cb78369f..254ea76fcbe9 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h
@@ -66,6 +66,7 @@ struct mlx5e_tc_update_priv {
 struct mlx5_nic_flow_attr {
 	u32 flow_tag;
 	u32 hairpin_tirn;
+	u32 rx_tirn;
 	struct mlx5_flow_table *hairpin_ft;
 };
 
-- 
2.43.0

Re: [PATCH net-next 08/10] net/mlx5e: TC, offload RX queue mapping in NIC flows

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The terminating-action guard in tc_act_can_offload_rx_queue()
  (drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/rx_queue.c) is…
- [Low] tc_act_can_offload_rx_queue() reads
  `flow->priv->channels.params.num_channels` (a u16) with no lock and no…

--

net/mlx5e: TC, offload RX queue mapping in NIC flows

This commit offloads the tc skbedit queue_mapping action for mlx5e NIC
flows. Matching traffic goes to the direct TIR of the selected channel,
the same way ethtool steering rules do it. The action must be last and
cannot follow goto or mirred.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/rx_queue.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/rx_queue.c
> new file mode 100644
> index 0000000000000..869da3cb703a8
> --- /dev/null
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/rx_queue.c
> @@ -0,0 +1,49 @@

[ ... ]

> +static bool
> +tc_act_can_offload_rx_queue(struct mlx5e_tc_act_parse_state *parse_state,
> +			    const struct flow_action_entry *act,
> +			    int act_index,
> +			    struct mlx5_flow_attr *attr)
> +{
> +	struct netlink_ext_ack *extack = parse_state->extack;
> +	struct mlx5e_tc_flow *flow = parse_state->flow;
> +
> +	/* The direct TIR takes over the destination of the rule */
> +	if (!flow_action_is_last_entry(parse_state->flow_action, act) ||
> +	    attr->dest_chain || flow_flag_test(flow, HAIRPIN)) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "RX queue mapping must be the last action and cannot follow goto or mirred");
> +		return false;
> +	}

[Severity: Medium]

Does this guard catch every earlier terminating action whose destination
the direct TIR would override? It only checks attr->dest_chain on the
current attr and the HAIRPIN flag.

A preceding gact ok seems to get through. mlx5e_tc_act_accept has
is_terminating_action set, but tc_act_parse_accept() only does:

	attr->action |= MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
	attr->flags |= MLX5_ATTR_FLAG_ACCEPT;

Nothing on the NIC path reads MLX5_ATTR_FLAG_ACCEPT, and dest_chain and
HAIRPIN are not changed. So a filter like this passes the check:

  tc filter add dev eth0 ingress protocol ip flower skip_sw ... \
      action ok action skbedit queue_mapping 3

verify_attr_actions() accepts it too, because it only rejects FWD_DEST
combined with DROP. In mlx5e_add_offloaded_nic_rule(), the new branch
then wins over the default forward to the VLAN table:

	} else if (nic_attr->rx_tirn) {
		dest[dest_ix].type = MLX5_FLOW_DESTINATION_TYPE_TIR;
		dest[dest_ix].tir_num = nic_attr->rx_tirn;

As a result, hardware sends the flow to direct TIR 3. In tc, the action
list ends at ok, so the queue mapping should never apply.

Can a goto in an earlier split attr get past the check in the same way?
Take goto chain N, then a non-clear ct, then skbedit queue_mapping Q.
tc_act_is_multi_table_act_ct() returns true for the ct, so
parse_tc_actions() splits after it. mlx5e_clone_flow_attr_for_post_act()
then clears the new attr:

	attr2->dest_chain = 0;

When the rx_queue action is checked against attr2, attr->dest_chain is
0, so the rule is accepted. Hardware would then steer to queue Q instead
of processing chain N.

This second case assumes NIC CT offload is available and that goto
followed by ct is accepted on the first attr.

> +
> +	if (act->rx_queue >= flow->priv->channels.params.num_channels) {

[Severity: Low]

Is this read of priv->channels.params.num_channels meant to be lockless?

mlx5e_setup_tc() sets f->unlocked_driver_cb = true, so this path can run
without rtnl:

mlx5e_configure_flower()->parse_tc_actions()->tc_act_can_offload_rx_queue()

At the same time, mlx5e_switch_priv_channels() overwrites the whole
struct under rtnl and priv->state_lock:

	priv->channels = *new_chs;

That leaves a plain write racing a plain read, which KCSAN can report.

The commit message already accepts the functional result. No out of
bounds access is possible either: num_channels never exceeds max_nch,
and direct TIRs exist for every index up to max_nch.

For comparison, the similar ring_cookie check in validate_flow() in
en_fs_ethtool.c runs under rtnl, so it is serialized with set_channels.

Should this read use READ_ONCE(), or should the check take
priv->state_lock?

> +		NL_SET_ERR_MSG_MOD(extack, "RX queue index out of range");
> +		return false;
> +	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109131039.959438-1-acassen%40corp.free.fr

[PATCH net-next 09/10] net/mlx5e: TC, match the tunnel key without a tunnel device

[Alexandre Cassen]

RSS can't spread GTP-U traffic between a few endpoints. The outer headers
barely vary and the TEID is not hashed. A masked tunnel key partitions
this traffic into RX queues. Other tunnels spread better under RSS. The
same match still gives a block of tenants its own queue.

Without a tunnel device nothing decapsulates. enc_key_id reads the tunnel
header on the wire like cxgb4 and hns3 do on a physical port. The plain
ip_proto and dst_port keys select the match register and its capability:

  GRE     ip_proto 0x2f   misc.gre_key     ft_field_support
  VXLAN   dst_port 4789   misc.vxlan_vni   ft_field_support
  GENEVE  dst_port 6081   misc.geneve_vni  flex parser and
                                           ft_field_support
  GTP-U   dst_port 2152   misc3.gtpu_teid  flex parser

These keys stay in the match. Without them a rule on a masked TEID of zero
also catches all other IPv4 traffic on ConnectX-7. The port must be exact
to name a tunnel. Only the default VXLAN port qualifies. The hardware can
stop parsing any other while the rule stays. A keyless GRE packet reads as
key zero like in software.

The parse_tunnel ops can't serve these flows. The VXLAN and GENEVE ones
check eswitch capabilities. The GRE one only matches gretap frames. A new
op keeps each register in the file of its tunnel.

This covers NIC and eswitch flows. A rule on the uplink representor used
to accept enc_key_id without matching on it. It caught all the traffic of
its other keys. It now matches the key or gets refused.

Software only has tunnel metadata after decapsulation. Refuse the offload
without skip_sw to keep both datapaths from diverging. Such a rule stays in
software. There a physical port reads key zero. Refuse a decap action too.
In software it only releases metadata and such a flow has none.

  tc filter add dev <dev> ingress protocol ip flower skip_sw \
      ip_proto udp dst_port 2152 enc_key_id <teid>/<mask>    \
      action skbedit queue_mapping <q> skip_sw

  tc filter add dev <dev> ingress protocol ip flower skip_sw \
      ip_proto 0x2f enc_key_id <key>/<mask>                  \
      action skbedit queue_mapping <q> skip_sw

Tested on ConnectX-7 with firmware 28.48.1000 on NIC flows. Also tested on
the uplink and VF representors in dmfs, smfs and hmfs. This includes
redirect to a VF. GTP-U needs flex parser profile 3 and GENEVE profile 0.
GRE and VXLAN work on both.

Signed-off-by: Alexandre Cassen <acassen@corp.free.fr>
---
 .../net/ethernet/mellanox/mlx5/core/Makefile  |  3 +-
 .../ethernet/mellanox/mlx5/core/en/tc_tun.c   | 88 +++++++++++++++++++
 .../ethernet/mellanox/mlx5/core/en/tc_tun.h   | 11 +++
 .../mellanox/mlx5/core/en/tc_tun_geneve.c     | 38 ++++++++
 .../mellanox/mlx5/core/en/tc_tun_gre.c        | 31 +++++++
 .../mellanox/mlx5/core/en/tc_tun_gtp.c        | 36 ++++++++
 .../mellanox/mlx5/core/en/tc_tun_vxlan.c      | 37 ++++++++
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   |  4 +
 8 files changed, 247 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_gtp.c

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Makefile b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
index f8d1696ab5eb..5d42c681dc7b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Makefile
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
@@ -46,7 +46,8 @@ mlx5_core-$(CONFIG_MLX5_CLS_ACT)     += en_tc.o en/rep/tc.o en/rep/neigh.o \
 					lib/fs_chains.o en/tc_tun.o \
 					esw/indir_table.o en/tc_tun_encap.o \
 					en/tc_tun_vxlan.o en/tc_tun_gre.o en/tc_tun_geneve.o \
-					en/tc_tun_mplsoudp.o diag/en_tc_tracepoint.o \
+					en/tc_tun_mplsoudp.o en/tc_tun_gtp.o \
+					diag/en_tc_tracepoint.o \
 					en/tc/post_act.o en/tc/int_port.o en/tc/meter.o \
 					en/tc/post_meter.o en/tc/act_stats.o
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c
index de74dbfe7b20..921af801d4d4 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c
@@ -7,6 +7,7 @@
 #include <net/gre.h>
 #include <net/geneve.h>
 #include <net/bareudp.h>
+#include <net/gtp.h>
 #include "en/tc_tun.h"
 #include "en/tc_priv.h"
 #include "en_tc.h"
@@ -996,3 +997,90 @@ int mlx5e_tc_tun_parse_udp_ports(struct mlx5e_priv *priv,
 
 	return 0;
 }
+
+/* Without a tunnel device the outer headers name the tunnel */
+static struct mlx5e_tc_tunnel *
+mlx5e_tc_tun_outer_lookup(struct flow_rule *rule)
+{
+	struct flow_match_basic basic;
+	struct flow_match_ports ports;
+
+	if (!flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_BASIC))
+		return NULL;
+
+	flow_rule_match_basic(rule, &basic);
+	if (basic.key->ip_proto == IPPROTO_GRE)
+		return &gre_tunnel;
+
+	if (basic.key->ip_proto != IPPROTO_UDP ||
+	    !flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_PORTS))
+		return NULL;
+
+	flow_rule_match_ports(rule, &ports);
+	/* Only an exact port names a tunnel */
+	if (ports.mask->dst != htons(U16_MAX))
+		return NULL;
+
+	switch (ntohs(ports.key->dst)) {
+	case GTP1U_PORT:
+		return &gtp_tunnel;
+	case IANA_VXLAN_UDP_PORT:
+		return &vxlan_tunnel;
+	case GENEVE_UDP_PORT:
+		return &geneve_tunnel;
+	default:
+		return NULL;
+	}
+}
+
+static bool
+mlx5e_tc_tun_has_decap(struct flow_rule *rule)
+{
+	const struct flow_action_entry *act;
+	int i;
+
+	flow_action_for_each(i, act, &rule->action) {
+		if (act->id == FLOW_ACTION_TUNNEL_DECAP)
+			return true;
+	}
+
+	return false;
+}
+
+/* Without a tunnel device only the hardware can read the tunnel key */
+int
+mlx5e_tc_tun_parse_outer_key(struct mlx5e_priv *priv,
+			     struct mlx5_flow_spec *spec,
+			     struct flow_cls_offload *f,
+			     enum fs_flow_table_type fs_type)
+{
+	struct flow_rule *rule = flow_cls_offload_flow_rule(f);
+	struct netlink_ext_ack *extack = f->common.extack;
+	struct flow_match_enc_keyid enc_keyid;
+	struct mlx5e_tc_tunnel *tunnel;
+
+	if (!f->common.skip_sw) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "enc_key_id without a tunnel device requires skip_sw");
+		return -EOPNOTSUPP;
+	}
+
+	if (mlx5e_tc_tun_has_decap(rule)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "enc_key_id without a tunnel device cannot decap");
+		return -EOPNOTSUPP;
+	}
+
+	tunnel = mlx5e_tc_tun_outer_lookup(rule);
+	if (!tunnel) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "enc_key_id needs ip_proto 0x2f or the exact dst_port of a supported tunnel");
+		return -EOPNOTSUPP;
+	}
+
+	flow_rule_match_enc_keyid(rule, &enc_keyid);
+	return tunnel->parse_outer_key(priv, spec, extack,
+				       be32_to_cpu(enc_keyid.key->keyid),
+				       be32_to_cpu(enc_keyid.mask->keyid),
+				       fs_type);
+}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h
index d7c778bbb2ef..8eccaecb70b2 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h
@@ -52,6 +52,11 @@ struct mlx5e_tc_tunnel {
 			    struct flow_cls_offload *f,
 			    void *headers_c,
 			    void *headers_v);
+	int (*parse_outer_key)(struct mlx5e_priv *priv,
+			       struct mlx5_flow_spec *spec,
+			       struct netlink_ext_ack *extack,
+			       u32 key, u32 mask,
+			       enum fs_flow_table_type fs_type);
 	bool (*encap_info_equal)(struct mlx5e_encap_key *a,
 				 struct mlx5e_encap_key *b);
 	int (*get_remote_ifindex)(struct net_device *mirred_dev);
@@ -61,6 +66,7 @@ extern struct mlx5e_tc_tunnel vxlan_tunnel;
 extern struct mlx5e_tc_tunnel geneve_tunnel;
 extern struct mlx5e_tc_tunnel gre_tunnel;
 extern struct mlx5e_tc_tunnel mplsoudp_tunnel;
+extern struct mlx5e_tc_tunnel gtp_tunnel;
 
 struct mlx5e_tc_tunnel *mlx5e_get_tc_tun(struct net_device *tunnel_dev);
 
@@ -115,6 +121,11 @@ int mlx5e_tc_tun_parse_udp_ports(struct mlx5e_priv *priv,
 				 void *headers_c,
 				 void *headers_v);
 
+int mlx5e_tc_tun_parse_outer_key(struct mlx5e_priv *priv,
+				 struct mlx5_flow_spec *spec,
+				 struct flow_cls_offload *f,
+				 enum fs_flow_table_type fs_type);
+
 bool mlx5e_tc_tun_encap_info_equal_generic(struct mlx5e_encap_key *a,
 					   struct mlx5e_encap_key *b);
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_geneve.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_geneve.c
index ee04fcdca6ff..9a2f7a409a82 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_geneve.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_geneve.c
@@ -4,6 +4,7 @@
 #include <net/geneve.h>
 #include "lib/geneve.h"
 #include "en/tc_tun.h"
+#include "fs_core.h"
 
 #define MLX5E_GENEVE_VER 0
 
@@ -350,6 +351,42 @@ static bool mlx5e_tc_tun_encap_info_equal_geneve(struct mlx5e_encap_key *a,
 						     IP_TUNNEL_GENEVE_OPT_BIT);
 }
 
+static int
+mlx5e_tc_tun_parse_outer_key_geneve(struct mlx5e_priv *priv,
+				    struct mlx5_flow_spec *spec,
+				    struct netlink_ext_ack *extack,
+				    u32 key, u32 mask,
+				    enum fs_flow_table_type fs_type)
+{
+	void *misc_c, *misc_v;
+
+	if (key >= MLX5E_TC_TUN_N_VNI) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Tunnel key does not fit the 24 bit VNI");
+		return -EOPNOTSUPP;
+	}
+
+	if (!mlx5e_tc_tun_can_offload_geneve(priv) ||
+	    !MLX5_CAP_FLOWTABLE_TYPE(priv->mdev,
+				     ft_field_support.outer_geneve_vni,
+				     fs_type)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Matching on GENEVE VNI is not supported");
+		return -EOPNOTSUPP;
+	}
+
+	misc_c = MLX5_ADDR_OF(fte_match_param, spec->match_criteria,
+			      misc_parameters);
+	misc_v = MLX5_ADDR_OF(fte_match_param, spec->match_value,
+			      misc_parameters);
+
+	MLX5_SET(fte_match_set_misc, misc_c, geneve_vni, mask);
+	MLX5_SET(fte_match_set_misc, misc_v, geneve_vni, key);
+	spec->match_criteria_enable |= MLX5_MATCH_MISC_PARAMETERS;
+
+	return 0;
+}
+
 struct mlx5e_tc_tunnel geneve_tunnel = {
 	.tunnel_type          = MLX5E_TC_TUNNEL_TYPE_GENEVE,
 	.match_level          = MLX5_MATCH_L4,
@@ -359,5 +396,6 @@ struct mlx5e_tc_tunnel geneve_tunnel = {
 	.generate_ip_tun_hdr  = mlx5e_gen_ip_tunnel_header_geneve,
 	.parse_udp_ports      = mlx5e_tc_tun_parse_udp_ports_geneve,
 	.parse_tunnel         = mlx5e_tc_tun_parse_geneve,
+	.parse_outer_key      = mlx5e_tc_tun_parse_outer_key_geneve,
 	.encap_info_equal     = mlx5e_tc_tun_encap_info_equal_geneve,
 };
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_gre.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_gre.c
index 579eda89fc76..1a3d329408f4 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_gre.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_gre.c
@@ -3,6 +3,7 @@
 
 #include <net/gre.h>
 #include "en/tc_tun.h"
+#include "fs_core.h"
 
 static bool mlx5e_tc_tun_can_offload_gretap(struct mlx5e_priv *priv)
 {
@@ -89,6 +90,35 @@ static int mlx5e_tc_tun_parse_gretap(struct mlx5e_priv *priv,
 	return 0;
 }
 
+static int
+mlx5e_tc_tun_parse_outer_key_gre(struct mlx5e_priv *priv,
+				 struct mlx5_flow_spec *spec,
+				 struct netlink_ext_ack *extack,
+				 u32 key, u32 mask,
+				 enum fs_flow_table_type fs_type)
+{
+	void *misc_c, *misc_v;
+
+	if (!MLX5_CAP_FLOWTABLE_TYPE(priv->mdev,
+				     ft_field_support.outer_gre_key,
+				     fs_type)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Matching on GRE key is not supported");
+		return -EOPNOTSUPP;
+	}
+
+	misc_c = MLX5_ADDR_OF(fte_match_param, spec->match_criteria,
+			      misc_parameters);
+	misc_v = MLX5_ADDR_OF(fte_match_param, spec->match_value,
+			      misc_parameters);
+
+	MLX5_SET(fte_match_set_misc, misc_c, gre_key.key, mask);
+	MLX5_SET(fte_match_set_misc, misc_v, gre_key.key, key);
+	spec->match_criteria_enable |= MLX5_MATCH_MISC_PARAMETERS;
+
+	return 0;
+}
+
 struct mlx5e_tc_tunnel gre_tunnel = {
 	.tunnel_type          = MLX5E_TC_TUNNEL_TYPE_GRETAP,
 	.match_level          = MLX5_MATCH_L3,
@@ -98,5 +128,6 @@ struct mlx5e_tc_tunnel gre_tunnel = {
 	.generate_ip_tun_hdr  = mlx5e_gen_ip_tunnel_header_gretap,
 	.parse_udp_ports      = NULL,
 	.parse_tunnel         = mlx5e_tc_tun_parse_gretap,
+	.parse_outer_key      = mlx5e_tc_tun_parse_outer_key_gre,
 	.encap_info_equal     = mlx5e_tc_tun_encap_info_equal_generic,
 };
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_gtp.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_gtp.c
new file mode 100644
index 000000000000..19c9bc1a30f5
--- /dev/null
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_gtp.c
@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
+
+#include "en/tc_tun.h"
+
+static int
+mlx5e_tc_tun_parse_outer_key_gtp(struct mlx5e_priv *priv,
+				 struct mlx5_flow_spec *spec,
+				 struct netlink_ext_ack *extack,
+				 u32 key, u32 mask,
+				 enum fs_flow_table_type fs_type)
+{
+	void *misc3_c, *misc3_v;
+
+	if (!(MLX5_CAP_GEN(priv->mdev, flex_parser_protocols) &
+	      MLX5_FLEX_PARSER_GTPU_TEID_ENABLED)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Matching on GTP-U TEID is not supported");
+		return -EOPNOTSUPP;
+	}
+
+	misc3_c = MLX5_ADDR_OF(fte_match_param, spec->match_criteria,
+			       misc_parameters_3);
+	misc3_v = MLX5_ADDR_OF(fte_match_param, spec->match_value,
+			       misc_parameters_3);
+
+	MLX5_SET(fte_match_set_misc3, misc3_c, gtpu_teid, mask);
+	MLX5_SET(fte_match_set_misc3, misc3_v, gtpu_teid, key);
+	spec->match_criteria_enable |= MLX5_MATCH_MISC_PARAMETERS_3;
+
+	return 0;
+}
+
+/* The driver offloads no GTP device so this only matches on the wire */
+struct mlx5e_tc_tunnel gtp_tunnel = {
+	.parse_outer_key = mlx5e_tc_tun_parse_outer_key_gtp,
+};
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
index 33911e72d20d..610e2bbc21f1 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
@@ -5,6 +5,7 @@
 #include <net/vxlan.h>
 #include "lib/vxlan.h"
 #include "en/tc_tun.h"
+#include "fs_core.h"
 
 static bool mlx5e_tc_tun_can_offload_vxlan(struct mlx5e_priv *priv)
 {
@@ -261,6 +262,41 @@ static int mlx5e_tc_tun_get_remote_ifindex(struct net_device *mirred_dev)
 	return ifindex;
 }
 
+static int
+mlx5e_tc_tun_parse_outer_key_vxlan(struct mlx5e_priv *priv,
+				   struct mlx5_flow_spec *spec,
+				   struct netlink_ext_ack *extack,
+				   u32 key, u32 mask,
+				   enum fs_flow_table_type fs_type)
+{
+	void *misc_c, *misc_v;
+
+	if (key >= MLX5E_TC_TUN_N_VNI) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Tunnel key does not fit the 24 bit VNI");
+		return -EOPNOTSUPP;
+	}
+
+	if (!MLX5_CAP_FLOWTABLE_TYPE(priv->mdev,
+				     ft_field_support.outer_vxlan_vni,
+				     fs_type)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Matching on VXLAN VNI is not supported");
+		return -EOPNOTSUPP;
+	}
+
+	misc_c = MLX5_ADDR_OF(fte_match_param, spec->match_criteria,
+			      misc_parameters);
+	misc_v = MLX5_ADDR_OF(fte_match_param, spec->match_value,
+			      misc_parameters);
+
+	MLX5_SET(fte_match_set_misc, misc_c, vxlan_vni, mask);
+	MLX5_SET(fte_match_set_misc, misc_v, vxlan_vni, key);
+	spec->match_criteria_enable |= MLX5_MATCH_MISC_PARAMETERS;
+
+	return 0;
+}
+
 struct mlx5e_tc_tunnel vxlan_tunnel = {
 	.tunnel_type          = MLX5E_TC_TUNNEL_TYPE_VXLAN,
 	.match_level          = MLX5_MATCH_L4,
@@ -270,6 +306,7 @@ struct mlx5e_tc_tunnel vxlan_tunnel = {
 	.generate_ip_tun_hdr  = mlx5e_gen_ip_tunnel_header_vxlan,
 	.parse_udp_ports      = mlx5e_tc_tun_parse_udp_ports_vxlan,
 	.parse_tunnel         = mlx5e_tc_tun_parse_vxlan,
+	.parse_outer_key      = mlx5e_tc_tun_parse_outer_key_vxlan,
 	.encap_info_equal     = mlx5e_tc_tun_encap_info_equal_vxlan,
 	.get_remote_ifindex   = mlx5e_tc_tun_get_remote_ifindex,
 };
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 3823ccc60dd3..6780e54a0cb9 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -2786,6 +2786,10 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
 		err = mlx5e_tc_verify_tunnel_ecn(priv, f, &match_inner_ecn);
 		if (err)
 			return err;
+	} else if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ENC_KEYID)) {
+		err = mlx5e_tc_tun_parse_outer_key(priv, spec, f, fs_type);
+		if (err)
+			return err;
 	}
 
 	err = mlx5e_flower_parse_meta(filter_dev, f);
-- 
2.43.0

Re: [PATCH net-next 09/10] net/mlx5e: TC, match the tunnel key without a tunnel device

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The new non-tunnel-device path in __parse_cls_flower()
  (en_tc.c:2789-2793) offloads any rule that carries ENC_KEYID.
- [Medium] The patch fixes a bug that users can already hit, but the fix
  ships inside a net-next feature patch with no Fixes: tag.
- [Medium] On VF representor (FDB) flows, the new path matches fields
  that sit past the header region the eswitch min-inline contract…

--

net/mlx5e: TC, match the tunnel key without a tunnel device

This adds a parse_outer_key tunnel op. With it, __parse_cls_flower() can
offload enc_key_id on filter devices that are not tunnel devices. The GRE
key, VXLAN VNI, GENEVE VNI or GTP-U TEID is matched on the wire, and the
tunnel is picked from ip_proto and an exact dst_port. These rules must be
skip_sw and cannot carry a decap action.

> This covers NIC and eswitch flows. A rule on the uplink representor used
> to accept enc_key_id without matching on it. It caught all the traffic of
> its other keys. It now matches the key or gets refused.

[Severity: Medium]
This describes a bug that predates this patch. Before this change,
__parse_cls_flower() whitelisted FLOW_DISSECTOR_KEY_ENC_KEYID but only read
it when mlx5e_get_tc_tun(filter_dev) was non-NULL. So a skip_sw rule with
enc_key_id on a non-tunnel device was offloaded without the key.

Could the fix go in its own patch with a Fixes: tag? As it stands, stable
trees can't take it without also taking the new outer key feature,
tc_tun_gtp.c and the parse_outer_key op.

Is the old bug really limited to the uplink representor?
parse_cls_flower() is shared by mlx5e_add_nic_flow() and
__mlx5e_add_fdb_flow(). That suggests the NIC netdev (FS_FT_NIC_RX) and VF
representors accepted enc_key_id the same way without matching it.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c
> index de74dbfe7b20c..921af801d4d4e 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c

[ ... ]

> @@ -996,3 +997,90 @@ int mlx5e_tc_tun_parse_udp_ports(struct mlx5e_priv *priv,

[ ... ]

> +/* Without a tunnel device only the hardware can read the tunnel key */
> +int
> +mlx5e_tc_tun_parse_outer_key(struct mlx5e_priv *priv,
> +			     struct mlx5_flow_spec *spec,
> +			     struct flow_cls_offload *f,
> +			     enum fs_flow_table_type fs_type)
> +{

[ ... ]

> +	tunnel = mlx5e_tc_tun_outer_lookup(rule);
> +	if (!tunnel) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "enc_key_id needs ip_proto 0x2f or the exact dst_port of a supported tunnel");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	flow_rule_match_enc_keyid(rule, &enc_keyid);
> +	return tunnel->parse_outer_key(priv, spec, extack,
> +				       be32_to_cpu(enc_keyid.key->keyid),
> +				       be32_to_cpu(enc_keyid.mask->keyid),
> +				       fs_type);
> +}

[Severity: High]
Are the other tunnel keys in the same rule silently dropped here?

The used_keys whitelist in __parse_cls_flower() still accepts
ENC_IPV4_ADDRS, ENC_IPV6_ADDRS, ENC_PORTS, ENC_IP, ENC_OPTS and ENC_CONTROL
on any filter device. Only two places turn these keys into hardware
matches:

  - mlx5e_get_flow_tunnel_id()
  - mlx5e_tc_tun_parse() and mlx5e_tc_verify_tunnel_ecn()

Both are reached only from the mlx5e_get_tc_tun(filter_dev) branch.

This path programs only the key register and then returns 0. Take this rule
on the uplink representor:

  tc filter add dev <uplink_rep> ingress protocol ip flower skip_sw \
      ip_proto 0x2f enc_key_id 100 enc_dst_ip 10.0.0.1 \
      action mirred egress redirect dev <vf_rep> skip_sw

It looks like the rule would be accepted. Hardware would then redirect
every GRE packet with key 100 to that VF, whatever its outer destination.
enc_opts, enc_dst_port and enc_tos would be ignored in the same way.

Because skip_sw is required, software never shows the wider match. A
documentation patch later in the series says "All the keys describe the
packet on the wire", which makes such rules more likely. That patch does
not change this code.

Should mlx5e_tc_tun_parse_outer_key() refuse rules that carry any other
ENC_* key?

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index 3823ccc60dd39..6780e54a0cb93 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -2786,6 +2786,10 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
>  		err = mlx5e_tc_verify_tunnel_ecn(priv, f, &match_inner_ecn);
>  		if (err)
>  			return err;
> +	} else if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ENC_KEYID)) {
> +		err = mlx5e_tc_tun_parse_outer_key(priv, spec, f, fs_type);
> +		if (err)
> +			return err;
>  	}

[Severity: Medium]
Can this path be hit on VF representors whose eswitch min-inline mode does
not cover the tunnel header?

For non-uplink eswitch flows, parse_cls_flower() only checks the match
level:

	if (rep->vport != MLX5_VPORT_UPLINK &&
	    (esw->offloads.inline_mode != MLX5_INLINE_MODE_NONE &&
	    esw->offloads.inline_mode < non_tunnel_match_level)) {

mlx5e_tc_tun_parse_outer_key() writes misc.gre_key, vxlan_vni, geneve_vni
or misc3.gtpu_teid, but it never raises *outer_match_level. The level stays
at MLX5_MATCH_L3 (from ip_proto) or MLX5_MATCH_L4 (from dst_port).

On the VF TX side, mlx5e_calc_min_inline() does:

	case MLX5_INLINE_MODE_TCP_UDP:
		hlen = eth_get_headlen(skb->dev, skb->data, skb_headlen(skb));
	...
	case MLX5_INLINE_MODE_IP:
		hlen = mlx5e_skb_l3_header_offset(skb);

So inline_mode IP does not inline the GRE key. With inline_mode TCP_UDP,
the inlined bytes end after the UDP header, so the VXLAN, GENEVE or GTP-U
header is left out.

A GRE key rule therefore passes the check with inline_mode IP. A VXLAN,
GENEVE or GTP-U rule passes with inline_mode TCP_UDP.

On min-inline devices, such as ConnectX-4 Lx with vport context inline
mode, could the eswitch miss these bytes in VF-originated traffic? The
skip_sw rule would then never fire, or would match the wrong packets.

Before this patch, tunnel key matching never reached hardware on VF
representors. The VF representor testing in the commit message was on
ConnectX-7, where inline mode is NONE.

Should this path refuse the rule on non-uplink vports when
esw->offloads.inline_mode is not MLX5_INLINE_MODE_NONE?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109131039.959438-1-acassen%40corp.free.fr

[PATCH net-next 10/10] docs: networking: describe tunnel key partitioning in TC queue filtering

[Alexandre Cassen]

Tunnel key partitioning steers blocks of tunnel flows to RX queues with a
mask on enc_key_id. Document this use and what enc_key_id means on a
physical port of mlx5. Also document what happens to such a filter when
its queue goes away.

Signed-off-by: Alexandre Cassen <acassen@corp.free.fr>
---
 Documentation/networking/tc-queue-filters.rst | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/networking/tc-queue-filters.rst b/Documentation/networking/tc-queue-filters.rst
index 6b417092276f..de54d43687c6 100644
--- a/Documentation/networking/tc-queue-filters.rst
+++ b/Documentation/networking/tc-queue-filters.rst
@@ -35,3 +35,25 @@ queues and/or a single queue are supported as below:
    devices, TC filter directing traffic to a queue have higher
    priority over flow director filter assigning a queue. The hash
    filter has lowest priority.
+
+Tunnel key partitioning splits the key space of a tunnel into blocks. One
+filter with a mask on enc_key_id steers a block to its own queue. This
+helps with traffic that RSS cannot spread because its hashed fields barely
+vary, such as GTP-U tunnels between a few endpoints::
+
+  # GTP-U TEIDs whose two top bits are 01 go to queue 1
+  tc filter add dev eth0 ingress protocol ip flower skip_sw \
+      ip_proto udp dst_port 2152 enc_key_id 0x40000000/0xc0000000 \
+      action skbedit queue_mapping 1 skip_sw
+
+Not all drivers can program a partial mask.
+
+On mlx5 devices such a filter runs on a physical port where nothing
+decapsulates the packet. All the keys describe the packet on the wire.
+ip_proto and dst_port tell the hardware which tunnel header holds the key.
+The software datapath has no tunnel metadata on such a port. The filter
+therefore needs skip_sw. Without it the filter stays in software, where
+the port reads key zero.
+
+Reducing the number of channels below the queue of a filter drops the
+traffic of that filter on mlx5 devices until the channel returns.
-- 
2.43.0

Re: [PATCH net-next 10/10] docs: networking: describe tunnel key partitioning in TC queue filtering

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new mlx5 paragraph in Documentation/networking/tc-queue-
  filters.rst (lines 54-56) says: "The filter therefore needs skip_sw.
- [Low] The commit message says the patch documents 'what enc_key_id
  means on a physical port of mlx5'.

--

docs: networking: describe tunnel key partitioning in TC queue filtering

This commit documents tunnel key partitioning in tc-queue-filters.rst, where
a mask on enc_key_id steers blocks of tunnel flows to RX queues. It adds a
GTP-U example and describes what enc_key_id means on an mlx5 physical port.
It also covers what happens to the filter when its target queue goes away.

> diff --git a/Documentation/networking/tc-queue-filters.rst b/Documentation/networking/tc-queue-filters.rst
> index 6b417092276f6..de54d43687c60 100644
> --- a/Documentation/networking/tc-queue-filters.rst
> +++ b/Documentation/networking/tc-queue-filters.rst
> @@ -35,3 +35,25 @@ queues and/or a single queue are supported as below:

[ ... ]

> +  # GTP-U TEIDs whose two top bits are 01 go to queue 1
> +  tc filter add dev eth0 ingress protocol ip flower skip_sw \
> +      ip_proto udp dst_port 2152 enc_key_id 0x40000000/0xc0000000 \
> +      action skbedit queue_mapping 1 skip_sw
> +
> +Not all drivers can program a partial mask.
> +
> +On mlx5 devices such a filter runs on a physical port where nothing
> +decapsulates the packet. All the keys describe the packet on the wire.
> +ip_proto and dst_port tell the hardware which tunnel header holds the key.

[Severity: Low]

The commit message says this documents "what enc_key_id means on a physical
port of mlx5". Should this paragraph also list the limits the driver
enforces? As written, the driver can refuse the GTP-U example above, and
small variations of it, for reasons the document does not mention.

First, the GTP-U example needs firmware flex parser support:

drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_gtp.c:mlx5e_tc_tun_parse_outer_key_gtp() {
    ...
	if (!(MLX5_CAP_GEN(priv->mdev, flex_parser_protocols) &
	      MLX5_FLEX_PARSER_GTPU_TEID_ENABLED)) {
		NL_SET_ERR_MSG_MOD(extack,
				   "Matching on GTP-U TEID is not supported");
		return -EOPNOTSUPP;
    ...
}

The commit "net/mlx5e: TC, match the tunnel key without a tunnel device"
says GTP-U needs flex parser profile 3, while GENEVE needs profile 0. Since
skip_sw is mandatory there is no software fallback. The documented command
therefore fails on a device that runs another profile.

Second, queue mapping is only offloaded in NIC mode. In en/tc/act/act.c,
FLOW_ACTION_RX_QUEUE_MAPPING is registered only in tc_acts_nic, and
mlx5e_tc_act_get() uses tc_acts_fdb for FDB flows. The action is refused on
the uplink representor in switchdev mode, which is also a physical port.

Third, ip_proto and dst_port only select a match register.
mlx5e_tc_tun_outer_lookup() accepts only ip_proto GRE or an exact UDP
dst_port of 2152, 4789 or 6081:

drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c:mlx5e_tc_tun_outer_lookup() {
    ...
	/* Only an exact port names a tunnel */
	if (ports.mask->dst != htons(U16_MAX))
		return NULL;

	switch (ntohs(ports.key->dst)) {
    ...
}

VXLAN on port 8472, or a masked port, is refused with "enc_key_id needs
ip_proto 0x2f or the exact dst_port of a supported tunnel".

Fourth, VXLAN and GENEVE keys are 24 bit VNIs.
mlx5e_tc_tun_parse_outer_key_vxlan() and
mlx5e_tc_tun_parse_outer_key_geneve() refuse a masked key that does not fit
in 24 bits ("Tunnel key does not fit the 24 bit VNI"). The top bits mask
from the example (0x40000000/0xc0000000) cannot be reused for those tunnels.

Fifth, mlx5e_tc_tun_parse_outer_key() also refuses decap actions.

The only caveat the document gives is "Not all drivers can program a
partial mask", which covers none of these limits.

> +The software datapath has no tunnel metadata on such a port. The filter
> +therefore needs skip_sw. Without it the filter stays in software, where
> +the port reads key zero.

[Severity: Low]

Is "Without it the filter stays in software" accurate for the queue_mapping
filters this document describes? Leaving out skip_sw does not seem to
install a software only filter.

If skip_sw is removed only from the filter, tcf_action_init() rejects the
skip_sw action:

net/sched/act_api.c:tcf_action_init() {
    ...
				if ((tc_act_skip_sw(act->tcfa_flags) && !skip_sw) ||
				    (tc_act_skip_hw(act->tcfa_flags) && !skip_hw)) {
					NL_SET_ERR_MSG(extack,
						       "Mismatch between action and filter offload flags");
					err = -EINVAL;
					goto err;
				}
    ...
}

If skip_sw is removed from the action, tcf_skbedit_init() refuses ingress
queue_mapping:

net/sched/act_skbedit.c:tcf_skbedit_init() {
    ...
		if (is_tcf_skbedit_ingress(act_flags) &&
		    !(act_flags & TCA_ACT_FLAGS_SKIP_SW)) {
			NL_SET_ERR_MSG_MOD(extack, "\"queue_mapping\" option on receive side is hardware only, use skip_sw");
			return -EOPNOTSUPP;
		}
    ...
}

The filter level skip_sw is not merged into the flags that the action init
sees, so this happens whatever the filter flags are. Both failures happen
before mlx5e_tc_tun_parse_outer_key() runs.

The "stays in software, reads key zero" behavior only seems to apply to
actions that can run in software, such as drop, mirred or mark. The text
looks like it was carried over from the commit message of "net/mlx5e: TC,
match the tunnel key without a tunnel device", which describes generic
actions.

Could this sentence say that the queue_mapping request is refused instead?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109131039.959438-1-acassen%40corp.free.fr