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

[netdev-bot+sashiko@kernel.org]

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