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

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

[Julius Bairaktaris]

nf_flow_rule_match() fills the vlan and cvlan keys and sets their
dissector offsets, but does not set their bits in used_keys. That bit
is how a driver learns that a rule has the key: flow_rule_match_key()
reads used_keys, and a key the rule does not declare cannot be read.
So no driver sees the vlan match, although the rule carries the vlan
pop action built from the same encapsulation. 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.

Set the bits where the keys are filled, like the other keys in this
function.

Fixes: 3e1b0c168f6c ("netfilter: flowtable: add vlan match offload support")
Assisted-by: Claude:claude-fable-5-1
Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>
---
 net/netfilter/nf_flow_table_offload.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
index 801a3dd9ceea..9ef89127bf25 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);
 		}
 	}
 
-- 
2.53.0

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

[Julius Bairaktaris]

Sent twice by mistake; both copies are identical, please ignore this one.

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

[netdev-bot+sashiko]

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

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

[Julius Bairaktaris]

nf_flow_rule_match() fills the vlan and cvlan keys and sets their
dissector offsets, but does not set their bits in used_keys. That bit
is how a driver learns that a rule has the key: flow_rule_match_key()
reads used_keys, and a key the rule does not declare cannot be read.
So no driver sees the vlan match, although the rule carries the vlan
pop action built from the same encapsulation. 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.

Set the bits where the keys are filled, like the other keys in this
function.

Fixes: 3e1b0c168f6c ("netfilter: flowtable: add vlan match offload support")
Assisted-by: Claude:claude-fable-5-1
Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>
---
 net/netfilter/nf_flow_table_offload.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
index 801a3dd9ceea..9ef89127bf25 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);
 		}
 	}
 
-- 
2.53.0

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 0

- [Medium] Commit message justification names a driver that cannot
  consume the newly advertised key.
- [Medium] The `else` branch of the second encapsulation block now
  declares an outer VLAN match that describes the wrong tag when the…

--

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 ORs BIT_ULL(FLOW_DISSECTOR_KEY_VLAN) and
BIT_ULL(FLOW_DISSECTOR_KEY_CVLAN) into used_keys where the keys are filled.

> netfilter: flowtable: advertise the vlan match in used_keys
>
> 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: Medium]
Is the mtk part of this justification accurate?  In
mtk_flow_offload_replace() the FLOW_DISSECTOR_KEY_VLAN read sits inside the
addr_type == 0 arm:

drivers/net/ethernet/mediatek/mtk_ppe_offload.c:mtk_flow_offload_replace() {
	switch (addr_type) {
	case 0:
		offload_type = MTK_PPE_PKT_TYPE_BRIDGE;
		...
		if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_VLAN)) {
			...
			data.vlan_in = match.key->vlan_id;
		...
	case FLOW_DISSECTOR_KEY_IPV4_ADDRS:
	...
}

But nf_flow_rule_match() always sets key->control.addr_type to
FLOW_DISSECTOR_KEY_IPV4_ADDRS or FLOW_DISSECTOR_KEY_IPV6_ADDRS and returns
-EOPNOTSUPP for anything else, so flowtable rules always take the IPv4/IPv6
arms, which do not look at the vlan key at all.  mtk also never reads
FLOW_DISSECTOR_KEY_CVLAN and has no used_keys whitelist, so its behaviour
looks unchanged by this patch, before and after.

The driver whose behaviour does change appears to be mlx5e, through
mlx5e_rep_setup_ft_cb() -> mlx5e_rep_setup_tc_cls_flower() ->
__parse_cls_flower().  The other TC_SETUP_FT consumers (airoha_ppe.c,
mtk_wed.c, mt76, netdevsim) contain no FLOW_DISSECTOR_KEY_VLAN read either,
and nfp does not implement TC_SETUP_FT at all.

Could the changelog name the driver that actually consumes the new bits,
and drop or reword the mtk claim?

> 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 now advertise a vlan key that describes the wrong tag?

The branch is taken whenever vlan_encap is false, which includes the case
where encap[0] is simply not 802.1Q, because the first block tests:

net/netfilter/nf_flow_table_offload.c:nf_flow_rule_match() {
	if (tuple->encap_num > 0 && !(tuple->in_vlan_ingress & BIT(0)) &&
	    tuple->encap[0].proto == htons(ETH_P_8021Q)) {
	...
}

encap[0] is the outermost tag on the wire, since flow_offload_fill_route()
reverses the path-walk order:

net/netfilter/nf_flow_table_core.c:flow_offload_fill_route() {
	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;
	...
}

and encap[].proto is copied verbatim from the configured VLAN protocol,
which can be ETH_P_8021AD:

net/8021q/vlan_dev.c:vlan_dev_fill_forward_path() {
	path->encap.proto = vlan->vlan_proto;
	...
}

net/bridge/br_vlan.c:br_vlan_fill_forward_path_pvid() {
	path->bridge.vlan_proto = br->vlan_proto;
	...
}

So with an 802.1ad outer tag and an 802.1Q tag beneath it, the first block
is skipped, vlan_encap stays false, and the else branch stores the inner
tag (encap[1].id, tpid 0x8100) into key->vlan, the outer vlan slot.  Before
this patch that key was inert; with the bit set it becomes an authoritative
match, while the action side pops only the 802.1Q tag:

net/netfilter/nf_flow_table_offload.c:nf_flow_rule_route_common() {
		if (tuple->encap[i].proto == htons(ETH_P_8021Q)) {
			entry = flow_action_entry_next(flow_rule);
			if (!entry)
				return -1;
			entry->id = FLOW_ACTION_VLAN_POP;
	...
}

Would it be better to gate the else branch on tuple->in_vlan_ingress &
BIT(0), or to return -EOPNOTSUPP when the outer encapsulation is not
802.1Q, so the declared match and the emitted actions agree on which tag is
outermost?

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