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