Re: [PATCH net-next 03/10] nfp: flower: 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] 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