Re: [PATCH net] ip_gre: validate csum_start only if CHECKSUM_PARTIAL

On Sun, Sep 5, 2021 at 11:53 AM Alexander Duyck
[off-list ref] wrote:

On Sun, Sep 5, 2021 at 8:24 AM Willem de Bruijn
[off-list ref] wrote:

quoted

On Sat, Sep 4, 2021 at 7:47 PM Alexander Duyck
[off-list ref] wrote:

<snip>

quoted

quoted

You can do it since you have essentially already written half the code.. :)

Sent, but only the ipv4 patch.

I actually do not see an equivalent skb_pull path in ip6_gre.c. Will
take a closer look later, but don't have time for that now.

https://patchwork.kernel.org/project/netdevbpf/patch/20210905152109.1805619-1-willemdebruijn.kernel@gmail.com/

So does that mean that commit 9cf448c200ba ("ip6_gre: add validation
for csum_start") is adding overhead that doesn't really address
anything, and is introducing false positives? If so, should we just
revert it?

I agree.

I didn't immediately understand how there can be an ip6gre_header
analogous to ipgre_header (for headers_ops.create), but no skb_pull
in ip6gre_tunnel_xmit equivalent to the one in ipgre_xmit.

One difference between the two of them is the return value, which
returns an offset: ip6gre_header returns t->hlen. ipgre_header returns
t->hlen + sizeof(*iph). This extra offset was introduced in commit
77a482bdb2e6 ("ip_gre: fix ipgre_header to return correct offset") as
a fix to previous commit c544193214 ("GRE: Refactor GRE tunneling
code."). Before the latter commit the IPv4 header was included in
t->hlen, similar to IPv6. After that commit it is no longer. The latter
patch is also the one that introduces the skb_pull in ipgre_xmit.

I can't say that I fully understand how this works for IPv6.

But I think that is sufficient to understand that this skb_pull issue
does not affect the ip6_gre path and we best just revert that commit.