Re: [Patch net v2] ip_gre: set dev->hard_header_len and dev->needed_headroom properly

From: Xie He <xie.he.0141@gmail.com>
To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>,
	Linux Kernel Network Developers <netdev@vger.kernel.org>,
	syzbot <syzbot+4a2c52677a8a1aa283cb@syzkaller.appspotmail.com>,
	William Tu <u9012063@gmail.com>
Subject: Re: [Patch net v2] ip_gre: set dev->hard_header_len and dev->needed_headroom properly
Date: Sun, 11 Oct 2020 14:50:00 -0700	[thread overview]
Message-ID: <CAJht_EOfNv-zHTxzsstPT2-DqaCSb-wrATffoscDPQS3wPkU3w@mail.gmail.com> (raw)
In-Reply-To: <CA+FuTSe7QxHUJfyh3Wr3nb+dG=mrE5MCETCdSiVu9ZCXnFqAag@mail.gmail.com>

On Sun, Oct 11, 2020 at 2:07 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Sun, Oct 11, 2020 at 4:42 PM Xie He <xie.he.0141@gmail.com> wrote:
> >
> > Hi, thanks for attempting to fix this tunnel. Are we still considering
> > removing header_ops->create?
> >
> > As said in my email sent previously today, I want to remove
> > header_ops->create because 1) this keeps the un-exposed headers of GRE
> > devices consistent with those of GRETAP devices, and 2) I think the
> > GRE header (and the headers before the GRE header) is not actually the
> > L2 header of the tunnel (the Wikipedia page for "Generic Routing
> > Encapsulation" doesn't consider this protocol to be at L2 either).
> >
> > I'm not sure if you still agree to remove header_ops->create. Do you
> > still agree but think it'd be better to do that in a separate patch?
> >
> > Removing header_ops->create would simplify the fixing of the issue you
> > are trying to fix, too, because that way we would no longer need to
> > use header_ops or hard_header_len. Also, I'm worried that changing
> > hard_header_len (or needed_headroom) in ipgre_link_update would have
> > racing issues. If we remove header_ops, we no longer need to use
> > hard_header_len and we can just set needed_headroom to the maximum
> > value, so that we no longer need to update them in ipgre_link_update.
>
> Our messages crossed.
>
> It seems there are legacy expectations that sendto/recvfrom packet
> sockets allow writing/reading the outer IP address, as of commit
> 6a5f44d7a048 ("[IPV4] ip_gre: sendto/recvfrom NBMA address"). That is
> the express purpose of that commit.
>
> The behavior is inconsistent with other tunnels, as you also point
> out, and probably only rarely used if at all. I would love to get rid
> of it, but given that we cannot be certain that it is unused, I'm afraid
> that we have to continue to support this special case.

OK. At least we agree that in an ideal world header_ops for GRE
devices should be removed.

Thanks, Willem.