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

[netdev-bot+sashiko@kernel.org]

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