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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Sun, Sep 5, 2021 at 11:53 AM Alexander Duyck
<alexander.duyck@gmail.com> wrote:
>
> On Sun, Sep 5, 2021 at 8:24 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > On Sat, Sep 4, 2021 at 7:47 PM Alexander Duyck
> > <alexander.duyck@gmail.com> wrote:
>
> <snip>
>
> > > 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.