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

On 6/28/25 3:27 PM, Eric Woudstra wrote:

On 6/22/25 10:16 PM, Florian Westphal wrote:

quoted

Eric Woudstra [off-list ref] wrote:

quoted

-	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(skb, offset);

nf_conntrack_in() can free the skb, or steal it.

But aside from this, I'm not sure this is a good idea to begin with,
it feels like we start to reimplement br_netfilter.c .

Perhaps it would be better to not push/pull but instead rename

unsigned int
nf_conntrack_in(struct sk_buff *skb, const struct nf_hook_state *state)

 to

unsigned int 
nf_conntrack_inner(struct sk_buff *skb, const struct nf_hook_state *state,
		   unsigned int nhoff)

and add

unsigned int 
nf_conntrack_in(struct sk_buff *skb, const struct nf_hook_state *state)
{
	return nf_conntrack_inner(skb, state, skb_network_offset(skb));
}

Or, alternatively, add
struct nf_ct_pktoffs {
	u16 nhoff;
	u16 thoff;
};

then populate that from nf_ct_bridge_pre(), then pass that to
nf_conntrack_inner() (all names are suggestions, if you find something
better thats fine).

Its going to be more complicated than this, but my point is that e.g.
nf_ct_get_tuple() already gets the l4 offset, so why not pass l3
offset too?

So I've tried nf_conntrack_inner(). The thing is:

quoted

 	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;

This part all use ip_hdr(skb) and ipv6_hdr(skb). I could add offset to
skb->network_header temporarily for this part of the code. Do you think
that is okay?

Adding offset to skb->network_header during the call to
nf_conntrack_in() does not work, but, as you mentioned, adding the
offset through the nf_conntrack_inner() function, that does work. Except
for 1 piece of code, I found so far:

A small correction, Adding offset to skb->network_header during to call
to nf_conntrack_in() also works. Then skb->network_header can be
restored after this call and nf_conntrack_inner() is not needed.

nf_checksum() reports an error when it is called from
nf_conntrack_tcp_packet(). It also uses ip_hdr(skb) and ipv6_hdr(skb).
Strangely, It only gives the error when dealing with a pppoe packet or
pppoe-in-q packet. There is no error when q-in-q (double q) or 802.1ad
are involved.

Do you have any suggestion how you want to handle this failure in
nf_checksum()?