[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

[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

[PATCH nf v2 0/2] netfilter: flowtable: correct and advertise the vlan match

[Julius Bairaktaris]

nf_flow_rule_match() builds a vlan match for an offloaded flow and
never declares it in used_keys, so no driver reads it. Patch 1
restricts the vlan key to the tag that is outermost on the frame.
Patch 2 sets the used_keys bits.

mlx5e is the in-tree driver that reads the vlan key. With the key
undeclared, __parse_cls_flower() installs an untagged-only match for a
flow whose ingress path carries a tag. After patch 2, mlx5 hardware without
outer_second_vid support rejects a double-tagged flow instead of
installing a match that cannot hit, and the flow stays in the software
path as before.

An 802.1ad tag is neither matched nor popped, before and after this
series, and a flow with an 802.1ad tag outside an 802.1Q tag gets no
vlan key after patch 1, as a flow with a single 802.1ad tag does
today. The pop action for the inner 802.1Q tag stays: mtk and airoha
ignore the vlan keys and FLOW_ACTION_VLAN_POP and accept that rule
today, so patch 1 does not reject it. Matching and popping 802.1ad is
a separate change.

Patch 2 is tested with an out-of-tree driver on IPQ8074 (6.18): the
driver reads the ingress vid and offloads the flow. The mlx5e
behaviour is read from the driver and was not run on mlx5 hardware.

Changes in v2:
- Name mlx5e as the consumer instead of mtk. mtk_flow_offload_replace()
  reads the vlan key only in its addr_type == 0 arm, which a flowtable
  rule never takes.
- Add patch 1. The else branch of the second encapsulation block was
  also reached with an 802.1ad tag outside an 802.1Q tag and described
  the inner tag as the outer one.
- v1: https://lore.kernel.org/netfilter-devel/20260906141947.480524-1-julius@bairaktaris.de/

Julius Bairaktaris (2):
  netfilter: flowtable: fill the vlan key from the outermost tag only
  netfilter: flowtable: advertise the vlan match in used_keys

 net/netfilter/nf_flow_table_offload.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

-- 
2.53.0

[PATCH nf v2 1/2] netfilter: flowtable: fill the vlan key from the outermost tag only

[Julius Bairaktaris]

The else branch of the second encapsulation block stores encap[1] in
the vlan key whenever the first block left it unfilled. The first
block skips encap[0] when the switch strips it, and also when it is
not 802.1Q. In the second case, an 802.1ad tag outside an 802.1Q tag,
encap[0] is still the first tag on the frame and the key describes
the inner tag as the outer one.

Store encap[1] in the vlan key only when encap[0] is stripped. The
802.1ad case then gets no vlan key, as a single 802.1ad tag does
today. The action list is unchanged.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
index 801a3dd9ceea..19adc6c7725f 100644
--- a/net/netfilter/nf_flow_table_offload.c
+++ b/net/netfilter/nf_flow_table_offload.c
@@ -137,7 +137,7 @@ 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);
-		} else {
+		} else if (tuple->in_vlan_ingress & BIT(0)) {
 			NF_FLOW_DISSECTOR(match, FLOW_DISSECTOR_KEY_VLAN,
 					  vlan);
 			nf_flow_rule_vlan_match(&key->vlan, &mask->vlan,
-- 
2.53.0

[PATCH nf v2 2/2] 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 leaves their bits clear in used_keys.
flow_rule_match_key() reads used_keys, so no driver sees the vlan
match.

mlx5e consumes flowtable rules through mlx5e_rep_setup_ft_cb() and
__parse_cls_flower(). With the vlan key undeclared and a filter device
that is not a vlan device, the parser sets cvlan_tag in the match
criteria with a zero value, an untagged-only match that the tagged
frames of the flow never hit.

Set the bits where the keys are filled.

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 19adc6c7725f..c50bbb1411d5 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 if (tuple->in_vlan_ingress & BIT(0)) {
 			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