On Fri, Aug 25, 2023 at 11:11 AM Florian Westphal <fw@xxxxxxxxx> wrote:
> > - tcph = nft_tcp_header_pointer(pkt, sizeof(buff), buff,
> > - &tcphdr_len);
> > - if (!tcph)
> > - goto err;
> > + tcph = (struct tcphdr *)(pkt->skb->data + nft_thoff(pkt));
> > + opt = (u8 *)tcph;
>
> This modification is not related to the bug?
>
> If you think this is better, then please say that the 'do not use
> nft_tcp_header_pointer' is an unrelated cleanup in the commit message.
>
> But I would prefer to not mix functional and non-functional changes.
> Also, the use of the nft_tcp_header_pointer() helper is the reason why
> this doesn't result in memory corruption.
I think this makes it explicit that
"we are modifying the original packet"
rather than
"we are modifying the packet because above skb_ensure_writable() is enough"
>
> > @@ -325,9 +324,9 @@ static void nft_exthdr_tcp_strip_eval(const struct nft_expr *expr,
> > if (skb_ensure_writable(pkt->skb, nft_thoff(pkt) + tcphdr_len))
>
> Just use the above in nft_exthdr_tcp_set_eval and place it before the loop?
In this case, all TCP headers will be pulled even if they don't have
the target option.
