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

[Eric Woudstra <ericwouds@gmail.com>]

On 6/28/25 3:27 PM, Eric Woudstra wrote:
> 
> 
> On 6/22/25 10:16 PM, Florian Westphal wrote:
>> Eric Woudstra <ericwouds@gmail.com> wrote:
>>> -	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:
> 
>>  	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()?
>