Re: [PATCH nf] netfilter: flowtable: advertise the vlan match in used_keys

[netdev-bot+sashiko@kernel.org]

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

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

- [High] The commit message's only concrete impact claim is false, and
  the bug class it claims to close stays reachable for the driver it…
- [Medium] In the second encap block of nf_flow_rule_match()
  (net/netfilter/nf_flow_table_offload.c:133-152), the `else` branch now…

--

netfilter: flowtable: advertise the vlan match in used_keys

nf_flow_rule_match() fills the vlan and cvlan dissector keys and sets
their offsets, but never sets the matching bits in
match->dissector.used_keys.  This patch sets those bits where the keys
are filled, so that flow_rule_match_key() based drivers can see the
match.

Setting the bits for keys that are actually filled looks right to me.
A couple of questions about the changelog and about the second
encapsulation branch.

The changelog says:

  mtk_ppe_offload.c reads the ingress vlan through flow_rule_match_key()
  and so never gets it, and a driver that checks used_keys against the
  keys it supports installs an entry that ignores the tag.

[Severity: High]
Does the mediatek part of that hold after this change?

mtk_flow_offload_replace() reads FLOW_DISSECTOR_KEY_VLAN only inside
case 0: of its addr_type switch:

	switch (addr_type) {
	case 0:
		offload_type = MTK_PPE_PKT_TYPE_BRIDGE;
		...
		if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_VLAN)) {
			struct flow_match_vlan match;

			flow_rule_match_vlan(rule, &match);
			...
			data.vlan_in = match.key->vlan_id;
		}
		break;
	case FLOW_DISSECTOR_KEY_IPV4_ADDRS:
		offload_type = MTK_PPE_PKT_TYPE_IPV4_HNAPT;
		break;

nf_flow_rule_match() always assigns a non-zero addr_type, and bails out
otherwise:

	switch (tuple->l3proto) {
	case AF_INET:
		key->control.addr_type = FLOW_DISSECTOR_KEY_IPV4_ADDRS;
		...
	case AF_INET6:
		key->control.addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
		...
	default:
		return -EOPNOTSUPP;
	}

so can a flowtable rule ever reach that case 0: read?  And the value it
would store only reaches the hardware for bridge entries:

	if (offload_type == MTK_PPE_PKT_TYPE_BRIDGE)
		foe.bridge.vlan = data.vlan_in;

The driver has no FLOW_DISSECTOR_KEY_CVLAN handling and no used_keys
whitelist, and its FLOW_ACTION_VLAN_POP case is an empty break, so it
neither consumes nor refuses the newly advertised keys and still installs
an HNAPT/IPv6 route entry that does not describe the ingress tag.

Could the mtk_ppe_offload.c sentence be dropped or reworded?  The driver
that does gate on used_keys and reads the vlan/cvlan keys is mlx5e in
__parse_cls_flower(), which the changelog does not name.

> diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
> index 801a3dd9ceea3..9ef89127bf252 100644
> --- a/net/netfilter/nf_flow_table_offload.c
> +++ b/net/netfilter/nf_flow_table_offload.c
> @@ -126,6 +126,7 @@ static int nf_flow_rule_match(struct nf_flow_match *match,
>  		nf_flow_rule_vlan_match(&key->vlan, &mask->vlan,
>  					tuple->encap[0].id,
>  					tuple->encap[0].proto);
> +		match->dissector.used_keys |= BIT_ULL(FLOW_DISSECTOR_KEY_VLAN);
>  		vlan_encap = true;
>  	}
>  
> @@ -137,12 +138,16 @@ static int nf_flow_rule_match(struct nf_flow_match *match,
>  			nf_flow_rule_vlan_match(&key->cvlan, &mask->cvlan,
>  						tuple->encap[1].id,
>  						tuple->encap[1].proto);
> +			match->dissector.used_keys |=
> +				BIT_ULL(FLOW_DISSECTOR_KEY_CVLAN);
>  		} else {
>  			NF_FLOW_DISSECTOR(match, FLOW_DISSECTOR_KEY_VLAN,
>  					  vlan);
>  			nf_flow_rule_vlan_match(&key->vlan, &mask->vlan,
>  						tuple->encap[1].id,
>  						tuple->encap[1].proto);
> +			match->dissector.used_keys |=
> +				BIT_ULL(FLOW_DISSECTOR_KEY_VLAN);

[Severity: Medium]
Can this else branch publish the inner tag as the outer vlan match?

The branch is reached both when the outer tag was already stripped before
the hook (in_vlan_ingress & BIT(0)), where treating encap[1] as the outer
tag is right, and when encap[0] is still present in the frame but is not
ETH_P_8021Q, because of the test in the first block:

	if (tuple->encap_num > 0 && !(tuple->in_vlan_ingress & BIT(0)) &&
	    tuple->encap[0].proto == htons(ETH_P_8021Q)) {

With a stacked 802.1ad plus 802.1Q configuration:

	ip link add link eth0 name eth0.100 type vlan proto 802.1ad id 100
	ip link add link eth0.100 name eth0.100.200 type vlan proto 802.1Q id 200

vlan_dev_fill_forward_path() copies the protocol verbatim:

	path->encap.id = vlan->vlan_id;
	path->encap.proto = vlan->vlan_proto;

and flow_offload_fill_dir() reverses the path order, so encap[0] is the
outermost tag:

	for (i = route->tuple[dir].in.num_encaps - 1; i >= 0; i--) {
		flow_tuple->encap[j].id = route->tuple[dir].in.encap[i].id;
		flow_tuple->encap[j].proto = route->tuple[dir].in.encap[i].proto;

That gives encap[0].proto == ETH_P_8021AD (vid 100) and encap[1].proto ==
ETH_P_8021Q (vid 200).  The first block is skipped, vlan_encap stays
false, and this else branch fills key->vlan with tpid 0x8100 / vid 200
and now declares it in used_keys, so a driver reads an outer vlan match
of 0x8100/200 for a frame whose outermost tag is 0x88a8/100.

nf_flow_rule_route_common() emits a pop only for ETH_P_8021Q encaps:

	if (tuple->encap[i].proto == htons(ETH_P_8021Q)) {
		entry = flow_action_entry_next(flow_rule);
		...
		entry->id = FLOW_ACTION_VLAN_POP;

so the 802.1ad tag is neither matched nor popped.  Before this patch the
mismatched value could not be read by any driver; should this branch be
restricted to the in_vlan_ingress case, or should a non-8021Q encap[0]
make the rule unoffloadable?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906142018.480577-1-julius%40bairaktaris.de