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

[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 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