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:
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()?
