[PATCH v11 nf-next 0/2] conntrack: bridge: add double vlan, pppoe and pppoe-in-q

[PATCH v11 nf-next 0/2] conntrack: bridge: add double vlan, pppoe and pppoe-in-q

[Eric Woudstra]

Conntrack bridge only tracks untagged and 802.1q.

To make the bridge-fastpath experience more similar to the
forward-fastpath experience, add double vlan, pppoe and pppoe-in-q
tagged packets to bridge conntrack and to bridge filter chain.

Changes in v11:

- nft_do_chain_bridge(): Proper readout of encapsulated proto.
- nft_do_chain_bridge(): Use skb_set_network_header() instead of thoff.
- removed test script, it is now in separate patch.

v10 split from patch-set: bridge-fastpath and related improvements v9

Eric Woudstra (2):
  netfilter: bridge: Add conntrack double vlan and pppoe
  netfilter: nft_chain_filter: Add bridge double vlan and pppoe

 net/bridge/netfilter/nf_conntrack_bridge.c | 83 ++++++++++++++++++----
 net/netfilter/nft_chain_filter.c           | 37 ++++++++++
 2 files changed, 108 insertions(+), 12 deletions(-)

-- 
2.47.1

[PATCH v11 nf-next 1/2] netfilter: bridge: Add conntrack double vlan and pppoe

[Eric Woudstra]

This adds the capability to conntrack 802.1ad, QinQ, PPPoE and PPPoE-in-Q
packets that are passing a bridge.

Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
---
 net/bridge/netfilter/nf_conntrack_bridge.c | 83 ++++++++++++++++++----
 1 file changed, 71 insertions(+), 12 deletions(-)

diff --git a/net/bridge/netfilter/nf_conntrack_bridge.c b/net/bridge/netfilter/nf_conntrack_bridge.c
index 816bb0fde718..4b4e3751fb13 100644
--- a/net/bridge/netfilter/nf_conntrack_bridge.c
+++ b/net/bridge/netfilter/nf_conntrack_bridge.c
@@ -242,53 +242,112 @@ static unsigned int nf_ct_bridge_pre(void *priv, struct sk_buff *skb,
 {
 	struct nf_hook_state bridge_state = *state;
 	enum ip_conntrack_info ctinfo;
+	int ret, offset = 0;
 	struct nf_conn *ct;
-	u32 len;
-	int ret;
+	__be16 outer_proto;
+	u32 len, data_len;
 
 	ct = nf_ct_get(skb, &ctinfo);
 	if ((ct && !nf_ct_is_template(ct)) ||
 	    ctinfo == IP_CT_UNTRACKED)
 		return NF_ACCEPT;
 
+	switch (skb->protocol) {
+	case htons(ETH_P_PPP_SES): {
+		struct ppp_hdr {
+			struct pppoe_hdr hdr;
+			__be16 proto;
+		} *ph;
+
+		offset = PPPOE_SES_HLEN;
+		if (!pskb_may_pull(skb, offset))
+			return NF_ACCEPT;
+		outer_proto = skb->protocol;
+		ph = (struct ppp_hdr *)(skb->data);
+		switch (ph->proto) {
+		case htons(PPP_IP):
+			skb->protocol = htons(ETH_P_IP);
+			break;
+		case htons(PPP_IPV6):
+			skb->protocol = htons(ETH_P_IPV6);
+			break;
+		default:
+			nf_ct_set(skb, NULL, IP_CT_UNTRACKED);
+			return NF_ACCEPT;
+		}
+		data_len = ntohs(ph->hdr.length) - 2;
+		skb_pull_rcsum(skb, offset);
+		skb_reset_network_header(skb);
+		break;
+	}
+	case htons(ETH_P_8021Q): {
+		struct vlan_hdr *vhdr;
+
+		offset = VLAN_HLEN;
+		if (!pskb_may_pull(skb, offset))
+			return NF_ACCEPT;
+		outer_proto = skb->protocol;
+		vhdr = (struct vlan_hdr *)(skb->data);
+		skb->protocol = vhdr->h_vlan_encapsulated_proto;
+		data_len = U32_MAX;
+		skb_pull_rcsum(skb, offset);
+		skb_reset_network_header(skb);
+		break;
+	}
+	default:
+		data_len = U32_MAX;
+		break;
+	}
+
+	ret = NF_ACCEPT;
 	switch (skb->protocol) {
 	case htons(ETH_P_IP):
 		if (!pskb_may_pull(skb, sizeof(struct iphdr)))
-			return NF_ACCEPT;
+			goto do_not_track;
 
 		len = skb_ip_totlen(skb);
+		if (data_len < len)
+			len = data_len;
 		if (pskb_trim_rcsum(skb, len))
-			return NF_ACCEPT;
+			goto do_not_track;
 
 		if (nf_ct_br_ip_check(skb))
-			return NF_ACCEPT;
+			goto do_not_track;
 
 		bridge_state.pf = NFPROTO_IPV4;
 		ret = nf_ct_br_defrag4(skb, &bridge_state);
 		break;
 	case htons(ETH_P_IPV6):
 		if (!pskb_may_pull(skb, sizeof(struct ipv6hdr)))
-			return NF_ACCEPT;
+			goto do_not_track;
 
 		len = sizeof(struct ipv6hdr) + ntohs(ipv6_hdr(skb)->payload_len);
+		if (data_len < len)
+			len = data_len;
 		if (pskb_trim_rcsum(skb, len))
-			return NF_ACCEPT;
+			goto do_not_track;
 
 		if (nf_ct_br_ipv6_check(skb))
-			return NF_ACCEPT;
+			goto do_not_track;
 
 		bridge_state.pf = NFPROTO_IPV6;
 		ret = nf_ct_br_defrag6(skb, &bridge_state);
 		break;
 	default:
 		nf_ct_set(skb, NULL, IP_CT_UNTRACKED);
-		return NF_ACCEPT;
+		goto do_not_track;
 	}
 
-	if (ret != NF_ACCEPT)
-		return ret;
+	if (ret == NF_ACCEPT)
+		ret = nf_conntrack_in(skb, &bridge_state);
 
-	return nf_conntrack_in(skb, &bridge_state);
+do_not_track:
+	if (offset) {
+		skb_push_rcsum(skb, offset);
+		skb_reset_network_header(skb);
+		skb->protocol = outer_proto;
+	}
+	return ret;
 }
 
 static unsigned int nf_ct_bridge_in(void *priv, struct sk_buff *skb,
-- 
2.47.1

Re: [PATCH v11 nf-next 1/2] netfilter: bridge: Add conntrack double vlan and pppoe

[Florian Westphal]

Eric Woudstra <ericwouds@gmail.com> wrote:
> This adds the capability to conntrack 802.1ad, QinQ, PPPoE and PPPoE-in-Q
> packets that are passing a bridge.

Conntrack is l2 agnostic, so this either requires distinct
ip addresses in the vlans/pppoe tunneled traffic or users
need to configure connection tracking zones manually to
ensure there are no collisions or traffic merges (i.e.,
packet x from PPPoE won't be merged with frag from a vlan).

Actually reading  nf_ct_br_defrag4/6 it seems existing
code already has this bug :/

I currently don't see a fix for this problem.
Can't add L2 addresses to conntrack since those aren't
unique accross vlans/tunnels and they can change anyway
even mid-stream, we can't add ifindexes into the mix
as we'd miss all reply traffic, can't use the vlan tag
since it can be vlan-in-vlan etc.

So likely, we have to live with this.

Maybe refuse to track (i.e. ACCEPT) vlan/8021ad qinq, etc.
traffic if the skb has no template with a zone attached to it?

This would at least push 'address collisions' into the
'incorrect ruleset configuration' domain.

Re: [PATCH v11 nf-next 1/2] netfilter: bridge: Add conntrack double vlan and pppoe

[Pablo Neira Ayuso]

On Tue, Apr 08, 2025 at 06:39:31PM +0200, Florian Westphal wrote:
> Eric Woudstra <ericwouds@gmail.com> wrote:
> > This adds the capability to conntrack 802.1ad, QinQ, PPPoE and PPPoE-in-Q
> > packets that are passing a bridge.
> 
> Conntrack is l2 agnostic, so this either requires distinct
> ip addresses in the vlans/pppoe tunneled traffic or users
> need to configure connection tracking zones manually to
> ensure there are no collisions or traffic merges (i.e.,
> packet x from PPPoE won't be merged with frag from a vlan).

There are conntrack zones.

Re: [PATCH v11 nf-next 1/2] netfilter: bridge: Add conntrack double vlan and pppoe

[Eric Woudstra]

On 4/8/25 6:39 PM, Florian Westphal wrote:
> Eric Woudstra <ericwouds@gmail.com> wrote:
>> This adds the capability to conntrack 802.1ad, QinQ, PPPoE and PPPoE-in-Q
>> packets that are passing a bridge.
> 
> Conntrack is l2 agnostic, so this either requires distinct
> ip addresses in the vlans/pppoe tunneled traffic or users
> need to configure connection tracking zones manually to
> ensure there are no collisions or traffic merges (i.e.,
> packet x from PPPoE won't be merged with frag from a vlan).
> 
> Actually reading  nf_ct_br_defrag4/6 it seems existing
> code already has this bug :/
> 
> I currently don't see a fix for this problem.
> Can't add L2 addresses to conntrack since those aren't
> unique accross vlans/tunnels and they can change anyway
> even mid-stream, we can't add ifindexes into the mix
> as we'd miss all reply traffic, can't use the vlan tag
> since it can be vlan-in-vlan etc.
> 
> So likely, we have to live with this.
> 
> Maybe refuse to track (i.e. ACCEPT) vlan/8021ad qinq, etc.
> traffic if the skb has no template with a zone attached to it?
> 
> This would at least push 'address collisions' into the
> 'incorrect ruleset configuration' domain.

Thanks for the input. I will look in to it and see if I can also add it
to the test script.

The thing is, single vlan (802.1Q) can be conntracked without setting up
a zone. I've only added Q-in-Q, AD and PPPoE-in-Q. Since single Q (L2)
can be conntracked, I thought the same will apply to other L2 tags.

So would single Q also need this restriction added in your opinion?

Re: [PATCH v11 nf-next 1/2] netfilter: bridge: Add conntrack double vlan and pppoe

[Florian Westphal]

Eric Woudstra <ericwouds@gmail.com> wrote:
> The thing is, single vlan (802.1Q) can be conntracked without setting up
> a zone. I've only added Q-in-Q, AD and PPPoE-in-Q. Since single Q (L2)
> can be conntracked, I thought the same will apply to other L2 tags.
> 
> So would single Q also need this restriction added in your opinion?

I think its too risky to add it now for single-Q case.

Re: [PATCH v11 nf-next 1/2] netfilter: bridge: Add conntrack double vlan and pppoe

[Eric Woudstra]

On 4/8/25 8:48 PM, Florian Westphal wrote:
> Eric Woudstra <ericwouds@gmail.com> wrote:
>> The thing is, single vlan (802.1Q) can be conntracked without setting up
>> a zone. I've only added Q-in-Q, AD and PPPoE-in-Q. Since single Q (L2)

I forgot to mention only PPPoE here.

>> can be conntracked, I thought the same will apply to other L2 tags.
>>
>> So would single Q also need this restriction added in your opinion?
> 
> I think its too risky to add it now for single-Q case.

Indeed, this would be a regression. I will look into only adding the
restriction to the newly added tags. However, it is inconsistent, which
is the point I was trying making.

[PATCH v11 nf-next 2/2] netfilter: nft_chain_filter: Add bridge double vlan and pppoe

[Eric Woudstra]

This adds the capability to evaluate 802.1ad, QinQ, PPPoE and PPPoE-in-Q
packets in the bridge filter chain.

Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
---
 net/netfilter/nft_chain_filter.c | 37 ++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/net/netfilter/nft_chain_filter.c b/net/netfilter/nft_chain_filter.c
index 19a553550c76..fe0b12f748dc 100644
--- a/net/netfilter/nft_chain_filter.c
+++ b/net/netfilter/nft_chain_filter.c
@@ -233,10 +233,47 @@ nft_do_chain_bridge(void *priv,
 		    const struct nf_hook_state *state)
 {
 	struct nft_pktinfo pkt;
+	__be16 proto;
 
 	nft_set_pktinfo(&pkt, skb, state);
 
 	switch (eth_hdr(skb)->h_proto) {
+	case htons(ETH_P_PPP_SES): {
+		struct ppp_hdr {
+			struct pppoe_hdr hdr;
+			__be16 proto;
+		} *ph = (struct ppp_hdr *)(skb->data);
+
+		skb_set_network_header(skb, PPPOE_SES_HLEN);
+		switch (ph->proto) {
+		case htons(PPP_IP):
+			proto = htons(ETH_P_IP);
+			skb->protocol = proto;
+			break;
+		case htons(PPP_IPV6):
+			proto = htons(ETH_P_IPV6);
+			skb->protocol = proto;
+			break;
+		default:
+			proto = 0;
+			break;
+		}
+		break;
+	}
+	case htons(ETH_P_8021Q): {
+		struct vlan_hdr *vhdr = (struct vlan_hdr *)(skb->data);
+
+		skb_set_network_header(skb, VLAN_HLEN);
+		proto = vhdr->h_vlan_encapsulated_proto;
+		skb->protocol = proto;
+		break;
+	}
+	default:
+		proto = eth_hdr(skb)->h_proto;
+		break;
+	}
+
+	switch (proto) {
 	case htons(ETH_P_IP):
 		nft_set_pktinfo_ipv4_validate(&pkt);
 		break;
-- 
2.47.1