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

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

[Cong Wang]

GRE tunnel has its own header_ops, ipgre_header_ops, and sets it
conditionally. When it is set, it assumes the outer IP header is
already created before ipgre_xmit().

This is not true when we send packets through a raw packet socket,
where L2 headers are supposed to be constructed by user. Packet
socket calls dev_validate_header() to validate the header. But
GRE tunnel does not set dev->hard_header_len, so that check can
be simply bypassed, therefore uninit memory could be passed down
to ipgre_xmit(). Similar for dev->needed_headroom.

dev->hard_header_len is supposed to be the length of the header
created by dev->header_ops->create(), so it should be used whenever
header_ops is set, and dev->needed_headroom should be used when it
is not set.

Reported-and-tested-by: syzbot+4a2c52677a8a1aa283cb@syzkaller.appspotmail.com
Cc: Xie He <xie.he.0141@gmail.com>
Cc: William Tu <u9012063@gmail.com>
Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
Note, there are some other suspicious use of dev->hard_header_len in
the same file, but let's leave them to a separate patch if really
needed.

 net/ipv4/ip_gre.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 4e31f23e4117..82fee0010353 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -626,8 +626,7 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
 
 	if (dev->header_ops) {
 		/* Need space for new headers */
-		if (skb_cow_head(skb, dev->needed_headroom -
-				      (tunnel->hlen + sizeof(struct iphdr))))
+		if (skb_cow_head(skb, dev->hard_header_len))
 			goto free_skb;
 
 		tnl_params = (const struct iphdr *)skb->data;
@@ -748,7 +747,11 @@ static void ipgre_link_update(struct net_device *dev, bool set_mtu)
 	len = tunnel->tun_hlen - len;
 	tunnel->hlen = tunnel->hlen + len;
 
-	dev->needed_headroom = dev->needed_headroom + len;
+	if (dev->header_ops)
+		dev->hard_header_len += len;
+	else
+		dev->needed_headroom += len;
+
 	if (set_mtu)
 		dev->mtu = max_t(int, dev->mtu - len, 68);
 
@@ -944,6 +947,7 @@ static void __gre_tunnel_init(struct net_device *dev)
 	tunnel->parms.iph.protocol = IPPROTO_GRE;
 
 	tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen;
+	dev->needed_headroom = tunnel->hlen + sizeof(tunnel->parms.iph);
 
 	dev->features		|= GRE_FEATURES;
 	dev->hw_features	|= GRE_FEATURES;
@@ -987,10 +991,14 @@ static int ipgre_tunnel_init(struct net_device *dev)
 				return -EINVAL;
 			dev->flags = IFF_BROADCAST;
 			dev->header_ops = &ipgre_header_ops;
+			dev->hard_header_len = tunnel->hlen + sizeof(*iph);
+			dev->needed_headroom = 0;
 		}
 #endif
 	} else if (!tunnel->collect_md) {
 		dev->header_ops = &ipgre_header_ops;
+		dev->hard_header_len = tunnel->hlen + sizeof(*iph);
+		dev->needed_headroom = 0;
 	}
 
 	return ip_tunnel_init(dev);
-- 
2.28.0

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

[Xie He]

On Sun, Oct 11, 2020 at 12:11 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> GRE tunnel has its own header_ops, ipgre_header_ops, and sets it
> conditionally. When it is set, it assumes the outer IP header is
> already created before ipgre_xmit().
>
> This is not true when we send packets through a raw packet socket,
> where L2 headers are supposed to be constructed by user. Packet
> socket calls dev_validate_header() to validate the header. But
> GRE tunnel does not set dev->hard_header_len, so that check can
> be simply bypassed, therefore uninit memory could be passed down
> to ipgre_xmit(). Similar for dev->needed_headroom.
>
> dev->hard_header_len is supposed to be the length of the header
> created by dev->header_ops->create(), so it should be used whenever
> header_ops is set, and dev->needed_headroom should be used when it
> is not set.

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.

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

[Willem de Bruijn]

On Sun, Oct 11, 2020 at 4:42 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Sun, Oct 11, 2020 at 12:11 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > GRE tunnel has its own header_ops, ipgre_header_ops, and sets it
> > conditionally. When it is set, it assumes the outer IP header is
> > already created before ipgre_xmit().
> >
> > This is not true when we send packets through a raw packet socket,
> > where L2 headers are supposed to be constructed by user. Packet
> > socket calls dev_validate_header() to validate the header. But
> > GRE tunnel does not set dev->hard_header_len, so that check can
> > be simply bypassed, therefore uninit memory could be passed down
> > to ipgre_xmit(). Similar for dev->needed_headroom.
> >
> > dev->hard_header_len is supposed to be the length of the header
> > created by dev->header_ops->create(), so it should be used whenever
> > header_ops is set, and dev->needed_headroom should be used when it
> > is not set.
>
> 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.

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

[Xie He]

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.

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

[Willem de Bruijn]

On Sun, Oct 11, 2020 at 3:11 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> GRE tunnel has its own header_ops, ipgre_header_ops, and sets it
> conditionally. When it is set, it assumes the outer IP header is
> already created before ipgre_xmit().
>
> This is not true when we send packets through a raw packet socket,
> where L2 headers are supposed to be constructed by user. Packet
> socket calls dev_validate_header() to validate the header. But
> GRE tunnel does not set dev->hard_header_len, so that check can
> be simply bypassed, therefore uninit memory could be passed down
> to ipgre_xmit(). Similar for dev->needed_headroom.
>
> dev->hard_header_len is supposed to be the length of the header
> created by dev->header_ops->create(), so it should be used whenever
> header_ops is set, and dev->needed_headroom should be used when it
> is not set.
>
> Reported-and-tested-by: syzbot+4a2c52677a8a1aa283cb@syzkaller.appspotmail.com
> Cc: Xie He <xie.he.0141@gmail.com>
> Cc: William Tu <u9012063@gmail.com>
> Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
> Note, there are some other suspicious use of dev->hard_header_len in
> the same file, but let's leave them to a separate patch if really
> needed.

There is agreement that hard_header_len should be the length of link
layer headers visible to the upper layers, needed_headroom the
additional room required for headers that are not exposed, i.e., those
pushed inside ndo_start_xmit.

The link layer header length also has to agree with the interface
hardware type (ARPHRD_..).

Tunnel devices have not always been consistent in this, but today
"bare" ip tunnel devices without additional headers (ipip, sit, ..) do
match this and advertise 0 byte hard_header_len. Bareudp, vxlan and
geneve also conform to this. Known exception that probably needs to be
addressed is sit, which still advertises LL_MAX_HEADER and so has
exposed quite a few syzkaller issues. Side note, it is not entirely
clear to me what sets ARPHRD_TUNNEL et al apart from ARPHRD_NONE and
why they are needed.

GRE devices advertise ARPHRD_IPGRE and GRETAP advertise ARPHRD_ETHER.
The second makes sense, as it appears as an Ethernet device. The first
should match "bare" ip tunnel devices, if following the above logic.
Indeed, this is what commit e271c7b4420d ("gre: do not keep the GRE
header around in collect medata mode") implements. It changes
dev->type to ARPHRD_NONE in collect_md mode.

Some of the inconsistency comes from the various modes of the GRE
driver. Which brings us to ipgre_header_ops. It is set only in two
special cases.

Commit 6a5f44d7a048 ("[IPV4] ip_gre: sendto/recvfrom NBMA address")
added ipgre_header_ops.parse to be able to receive the inner ip source
address with PF_PACKET recvfrom. And apparently relies on
ipgre_header_ops.create to be able to set an address, which implies
SOCK_DGRAM.

The other special case, CONFIG_NET_IPGRE_BROADCAST, predates git. Its
implementation starts with the beautiful comment "/* Nice toy.
Unfortunately, useless in real life :-)". From the rest of that
detailed comment, it is not clear to me why it would need to expose
the headers. The example does not use packet sockets.

A packet socket cannot know devices details such as which configurable
mode a device may be in. And different modes conflict with the basic
rule that for a given well defined link layer type, i.e., dev->type,
header length can be expected to be consistent. In an ideal world
these exceptions would not exist, therefore.

Unfortunately, this is legacy behavior that will have to continue to
be supported.

I agree that then swapping dev->needed_headroom for
dev->hard_header_len at init is then a good approach.

Underlying functions like ip_tunnel_xmit can modify needed_headroom at
runtime, not sure how that affects runtime changes in
ipgre_link_update:

"
        max_headroom = LL_RESERVED_SPACE(rt->dst.dev) + sizeof(struct iphdr)
                        + rt->dst.header_len + ip_encap_hlen(&tunnel->encap);
        if (max_headroom > dev->needed_headroom)
                dev->needed_headroom = max_headroom;
"

>  net/ipv4/ip_gre.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 4e31f23e4117..82fee0010353 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -626,8 +626,7 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
>
>         if (dev->header_ops) {
>                 /* Need space for new headers */
> -               if (skb_cow_head(skb, dev->needed_headroom -
> -                                     (tunnel->hlen + sizeof(struct iphdr))))
> +               if (skb_cow_head(skb, dev->hard_header_len))
>                         goto free_skb;
>
>                 tnl_params = (const struct iphdr *)skb->data;
> @@ -748,7 +747,11 @@ static void ipgre_link_update(struct net_device *dev, bool set_mtu)
>         len = tunnel->tun_hlen - len;
>         tunnel->hlen = tunnel->hlen + len;
>
> -       dev->needed_headroom = dev->needed_headroom + len;
> +       if (dev->header_ops)
> +               dev->hard_header_len += len;
> +       else
> +               dev->needed_headroom += len;
> +
>         if (set_mtu)
>                 dev->mtu = max_t(int, dev->mtu - len, 68);
>
> @@ -944,6 +947,7 @@ static void __gre_tunnel_init(struct net_device *dev)
>         tunnel->parms.iph.protocol = IPPROTO_GRE;
>
>         tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen;
> +       dev->needed_headroom = tunnel->hlen + sizeof(tunnel->parms.iph);
>
>         dev->features           |= GRE_FEATURES;
>         dev->hw_features        |= GRE_FEATURES;
> @@ -987,10 +991,14 @@ static int ipgre_tunnel_init(struct net_device *dev)
>                                 return -EINVAL;
>                         dev->flags = IFF_BROADCAST;
>                         dev->header_ops = &ipgre_header_ops;
> +                       dev->hard_header_len = tunnel->hlen + sizeof(*iph);
> +                       dev->needed_headroom = 0;

here and below, perhaps more descriptive of what is being done, something like:

/* If device has header_ops.create, then headers are part of hard_header_len */
swap(dev->needed_headroom, dev->hard_header_len)

>                 }
>  #endif
>         } else if (!tunnel->collect_md) {
>                 dev->header_ops = &ipgre_header_ops;
> +               dev->hard_header_len = tunnel->hlen + sizeof(*iph);
> +               dev->needed_headroom = 0;
>         }
>
>         return ip_tunnel_init(dev);
> --
> 2.28.0
>

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

[Willem de Bruijn]

On Sun, Oct 11, 2020 at 5:00 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Sun, Oct 11, 2020 at 3:11 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > GRE tunnel has its own header_ops, ipgre_header_ops, and sets it
> > conditionally. When it is set, it assumes the outer IP header is
> > already created before ipgre_xmit().
> >
> > This is not true when we send packets through a raw packet socket,
> > where L2 headers are supposed to be constructed by user. Packet
> > socket calls dev_validate_header() to validate the header. But
> > GRE tunnel does not set dev->hard_header_len, so that check can
> > be simply bypassed, therefore uninit memory could be passed down
> > to ipgre_xmit(). Similar for dev->needed_headroom.
> >
> > dev->hard_header_len is supposed to be the length of the header
> > created by dev->header_ops->create(), so it should be used whenever
> > header_ops is set, and dev->needed_headroom should be used when it
> > is not set.
> >
> > Reported-and-tested-by: syzbot+4a2c52677a8a1aa283cb@syzkaller.appspotmail.com
> > Cc: Xie He <xie.he.0141@gmail.com>
> > Cc: William Tu <u9012063@gmail.com>
> > Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>


> I agree that then swapping dev->needed_headroom for
> dev->hard_header_len at init is then a good approach.
>
> Underlying functions like ip_tunnel_xmit can modify needed_headroom at
> runtime, not sure how that affects runtime changes in
> ipgre_link_update:

Never mind, the patch only selects between needed_headroom or
hard_header_link at ipgre_tunnel_init.

> > @@ -987,10 +991,14 @@ static int ipgre_tunnel_init(struct net_device *dev)
> >                                 return -EINVAL;
> >                         dev->flags = IFF_BROADCAST;
> >                         dev->header_ops = &ipgre_header_ops;
> > +                       dev->hard_header_len = tunnel->hlen + sizeof(*iph);
> > +                       dev->needed_headroom = 0;
>
> here and below, perhaps more descriptive of what is being done, something like:
>
> /* If device has header_ops.create, then headers are part of hard_header_len */
> swap(dev->needed_headroom, dev->hard_header_len)

Arguably nitpicking. Otherwise, patch looks great to me, thanks, so

Acked-by: Willem de Bruijn <willemb@google.com>

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

[Xie He]

On Sun, Oct 11, 2020 at 2:01 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> There is agreement that hard_header_len should be the length of link
> layer headers visible to the upper layers, needed_headroom the
> additional room required for headers that are not exposed, i.e., those
> pushed inside ndo_start_xmit.
>
> The link layer header length also has to agree with the interface
> hardware type (ARPHRD_..).
>
> Tunnel devices have not always been consistent in this, but today
> "bare" ip tunnel devices without additional headers (ipip, sit, ..) do
> match this and advertise 0 byte hard_header_len. Bareudp, vxlan and
> geneve also conform to this. Known exception that probably needs to be
> addressed is sit, which still advertises LL_MAX_HEADER and so has
> exposed quite a few syzkaller issues. Side note, it is not entirely
> clear to me what sets ARPHRD_TUNNEL et al apart from ARPHRD_NONE and
> why they are needed.
>
> GRE devices advertise ARPHRD_IPGRE and GRETAP advertise ARPHRD_ETHER.
> The second makes sense, as it appears as an Ethernet device. The first
> should match "bare" ip tunnel devices, if following the above logic.
> Indeed, this is what commit e271c7b4420d ("gre: do not keep the GRE
> header around in collect medata mode") implements. It changes
> dev->type to ARPHRD_NONE in collect_md mode.
>
> Some of the inconsistency comes from the various modes of the GRE
> driver. Which brings us to ipgre_header_ops. It is set only in two
> special cases.
>
> Commit 6a5f44d7a048 ("[IPV4] ip_gre: sendto/recvfrom NBMA address")
> added ipgre_header_ops.parse to be able to receive the inner ip source
> address with PF_PACKET recvfrom. And apparently relies on
> ipgre_header_ops.create to be able to set an address, which implies
> SOCK_DGRAM.
>
> The other special case, CONFIG_NET_IPGRE_BROADCAST, predates git. Its
> implementation starts with the beautiful comment "/* Nice toy.
> Unfortunately, useless in real life :-)". From the rest of that
> detailed comment, it is not clear to me why it would need to expose
> the headers. The example does not use packet sockets.
>
> A packet socket cannot know devices details such as which configurable
> mode a device may be in. And different modes conflict with the basic
> rule that for a given well defined link layer type, i.e., dev->type,
> header length can be expected to be consistent. In an ideal world
> these exceptions would not exist, therefore.

Nice explanation of the situation. I agree with you.

Thanks!

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

[Xie He]

On Sun, Oct 11, 2020 at 2:01 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> There is agreement that hard_header_len should be the length of link
> layer headers visible to the upper layers, needed_headroom the
> additional room required for headers that are not exposed, i.e., those
> pushed inside ndo_start_xmit.
>
> The link layer header length also has to agree with the interface
> hardware type (ARPHRD_..).
>
> Tunnel devices have not always been consistent in this, but today
> "bare" ip tunnel devices without additional headers (ipip, sit, ..) do
> match this and advertise 0 byte hard_header_len. Bareudp, vxlan and
> geneve also conform to this. Known exception that probably needs to be
> addressed is sit, which still advertises LL_MAX_HEADER and so has
> exposed quite a few syzkaller issues. Side note, it is not entirely
> clear to me what sets ARPHRD_TUNNEL et al apart from ARPHRD_NONE and
> why they are needed.
>
> GRE devices advertise ARPHRD_IPGRE and GRETAP advertise ARPHRD_ETHER.
> The second makes sense, as it appears as an Ethernet device. The first
> should match "bare" ip tunnel devices, if following the above logic.
> Indeed, this is what commit e271c7b4420d ("gre: do not keep the GRE
> header around in collect medata mode") implements. It changes
> dev->type to ARPHRD_NONE in collect_md mode.
>
> Some of the inconsistency comes from the various modes of the GRE
> driver. Which brings us to ipgre_header_ops. It is set only in two
> special cases.
>
> Commit 6a5f44d7a048 ("[IPV4] ip_gre: sendto/recvfrom NBMA address")
> added ipgre_header_ops.parse to be able to receive the inner ip source
> address with PF_PACKET recvfrom. And apparently relies on
> ipgre_header_ops.create to be able to set an address, which implies
> SOCK_DGRAM.
>
> The other special case, CONFIG_NET_IPGRE_BROADCAST, predates git. Its
> implementation starts with the beautiful comment "/* Nice toy.
> Unfortunately, useless in real life :-)". From the rest of that
> detailed comment, it is not clear to me why it would need to expose
> the headers. The example does not use packet sockets.
>
> A packet socket cannot know devices details such as which configurable
> mode a device may be in. And different modes conflict with the basic
> rule that for a given well defined link layer type, i.e., dev->type,
> header length can be expected to be consistent. In an ideal world
> these exceptions would not exist, therefore.
>
> Unfortunately, this is legacy behavior that will have to continue to
> be supported.

Thanks for your explanation. So header_ops for GRE devices is only
used in 2 special situations. In normal situations, header_ops is not
used for GRE devices. And we consider not using header_ops should be
the ideal arrangement for GRE devices.

Can we create a new dev->type (like ARPHRD_IPGRE_SPECIAL) for GRE
devices that use header_ops? I guess changing dev->type will not
affect the interface to the user space? This way we can solve the
problem of the same dev->type having different hard_header_len values.

Also, for the second special situation, if there's no obvious reason
to use header_ops, maybe we can consider removing header_ops for this
situation.

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

[Willem de Bruijn]

On Wed, Oct 14, 2020 at 4:52 AM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Sun, Oct 11, 2020 at 2:01 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > There is agreement that hard_header_len should be the length of link
> > layer headers visible to the upper layers, needed_headroom the
> > additional room required for headers that are not exposed, i.e., those
> > pushed inside ndo_start_xmit.
> >
> > The link layer header length also has to agree with the interface
> > hardware type (ARPHRD_..).
> >
> > Tunnel devices have not always been consistent in this, but today
> > "bare" ip tunnel devices without additional headers (ipip, sit, ..) do
> > match this and advertise 0 byte hard_header_len. Bareudp, vxlan and
> > geneve also conform to this. Known exception that probably needs to be
> > addressed is sit, which still advertises LL_MAX_HEADER and so has
> > exposed quite a few syzkaller issues. Side note, it is not entirely
> > clear to me what sets ARPHRD_TUNNEL et al apart from ARPHRD_NONE and
> > why they are needed.
> >
> > GRE devices advertise ARPHRD_IPGRE and GRETAP advertise ARPHRD_ETHER.
> > The second makes sense, as it appears as an Ethernet device. The first
> > should match "bare" ip tunnel devices, if following the above logic.
> > Indeed, this is what commit e271c7b4420d ("gre: do not keep the GRE
> > header around in collect medata mode") implements. It changes
> > dev->type to ARPHRD_NONE in collect_md mode.
> >
> > Some of the inconsistency comes from the various modes of the GRE
> > driver. Which brings us to ipgre_header_ops. It is set only in two
> > special cases.
> >
> > Commit 6a5f44d7a048 ("[IPV4] ip_gre: sendto/recvfrom NBMA address")
> > added ipgre_header_ops.parse to be able to receive the inner ip source
> > address with PF_PACKET recvfrom. And apparently relies on
> > ipgre_header_ops.create to be able to set an address, which implies
> > SOCK_DGRAM.
> >
> > The other special case, CONFIG_NET_IPGRE_BROADCAST, predates git. Its
> > implementation starts with the beautiful comment "/* Nice toy.
> > Unfortunately, useless in real life :-)". From the rest of that
> > detailed comment, it is not clear to me why it would need to expose
> > the headers. The example does not use packet sockets.
> >
> > A packet socket cannot know devices details such as which configurable
> > mode a device may be in. And different modes conflict with the basic
> > rule that for a given well defined link layer type, i.e., dev->type,
> > header length can be expected to be consistent. In an ideal world
> > these exceptions would not exist, therefore.
> >
> > Unfortunately, this is legacy behavior that will have to continue to
> > be supported.
>
> Thanks for your explanation. So header_ops for GRE devices is only
> used in 2 special situations. In normal situations, header_ops is not
> used for GRE devices. And we consider not using header_ops should be
> the ideal arrangement for GRE devices.
>
> Can we create a new dev->type (like ARPHRD_IPGRE_SPECIAL) for GRE
> devices that use header_ops? I guess changing dev->type will not
> affect the interface to the user space? This way we can solve the
> problem of the same dev->type having different hard_header_len values.

But does that address any real issue?

If anything, it would make sense to keep ARHPHRD_IPGRE for tunnels
that expect headers and switch to ARPHRD_NONE for those that do not.
As the collect_md commit I mentioned above does.

> Also, for the second special situation, if there's no obvious reason
> to use header_ops, maybe we can consider removing header_ops for this
> situation.

Unfortunately, there's no knowing if some application is using this
broadcast mode *with* a process using packet sockets.

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

[Xie He]

On Wed, Oct 14, 2020 at 8:12 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Wed, Oct 14, 2020 at 4:52 AM Xie He <xie.he.0141@gmail.com> wrote:
> >
> > On Sun, Oct 11, 2020 at 2:01 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > There is agreement that hard_header_len should be the length of link
> > > layer headers visible to the upper layers, needed_headroom the
> > > additional room required for headers that are not exposed, i.e., those
> > > pushed inside ndo_start_xmit.
> > >
> > > The link layer header length also has to agree with the interface
> > > hardware type (ARPHRD_..).
> > >
> > > Tunnel devices have not always been consistent in this, but today
> > > "bare" ip tunnel devices without additional headers (ipip, sit, ..) do
> > > match this and advertise 0 byte hard_header_len. Bareudp, vxlan and
> > > geneve also conform to this. Known exception that probably needs to be
> > > addressed is sit, which still advertises LL_MAX_HEADER and so has
> > > exposed quite a few syzkaller issues. Side note, it is not entirely
> > > clear to me what sets ARPHRD_TUNNEL et al apart from ARPHRD_NONE and
> > > why they are needed.
> > >
> > > GRE devices advertise ARPHRD_IPGRE and GRETAP advertise ARPHRD_ETHER.
> > > The second makes sense, as it appears as an Ethernet device. The first
> > > should match "bare" ip tunnel devices, if following the above logic.
> > > Indeed, this is what commit e271c7b4420d ("gre: do not keep the GRE
> > > header around in collect medata mode") implements. It changes
> > > dev->type to ARPHRD_NONE in collect_md mode.
> > >
> > > Some of the inconsistency comes from the various modes of the GRE
> > > driver. Which brings us to ipgre_header_ops. It is set only in two
> > > special cases.
> > >
> > > Commit 6a5f44d7a048 ("[IPV4] ip_gre: sendto/recvfrom NBMA address")
> > > added ipgre_header_ops.parse to be able to receive the inner ip source
> > > address with PF_PACKET recvfrom. And apparently relies on
> > > ipgre_header_ops.create to be able to set an address, which implies
> > > SOCK_DGRAM.
> > >
> > > The other special case, CONFIG_NET_IPGRE_BROADCAST, predates git. Its
> > > implementation starts with the beautiful comment "/* Nice toy.
> > > Unfortunately, useless in real life :-)". From the rest of that
> > > detailed comment, it is not clear to me why it would need to expose
> > > the headers. The example does not use packet sockets.
> > >
> > > A packet socket cannot know devices details such as which configurable
> > > mode a device may be in. And different modes conflict with the basic
> > > rule that for a given well defined link layer type, i.e., dev->type,
> > > header length can be expected to be consistent. In an ideal world
> > > these exceptions would not exist, therefore.
> > >
> > > Unfortunately, this is legacy behavior that will have to continue to
> > > be supported.
> >
> > Thanks for your explanation. So header_ops for GRE devices is only
> > used in 2 special situations. In normal situations, header_ops is not
> > used for GRE devices. And we consider not using header_ops should be
> > the ideal arrangement for GRE devices.
> >
> > Can we create a new dev->type (like ARPHRD_IPGRE_SPECIAL) for GRE
> > devices that use header_ops? I guess changing dev->type will not
> > affect the interface to the user space? This way we can solve the
> > problem of the same dev->type having different hard_header_len values.
>
> But does that address any real issue?

It doesn't address any issue visible when using. Just to solve the
problem of the same dev->type having different hard_header_len values
which you mentioned. Making this change will not affect the user in
any way. So I think it is valuable to make this change.

> If anything, it would make sense to keep ARHPHRD_IPGRE for tunnels
> that expect headers and switch to ARPHRD_NONE for those that do not.
> As the collect_md commit I mentioned above does.

I thought we agreed that ideally GRE devices would not have
header_ops. Currently GRE devices (in normal situations) indeed do not
use header_ops (and use ARHPHRD_IPGRE as dev->type). I think we should
keep this behavior.

To solve the problem of the same dev->type having different
hard_header_len values which you mentioned. I think we should create a
new dev->type (ARPHRD_IPGRE_SPECIAL) for GRE devices that use
header_ops.

Also, for collect_md, I think we should use ARHPHRD_IPGRE. I see no
reason to use ARPHRD_NONE.

> > Also, for the second special situation, if there's no obvious reason
> > to use header_ops, maybe we can consider removing header_ops for this
> > situation.
>
> Unfortunately, there's no knowing if some application is using this
> broadcast mode *with* a process using packet sockets.

We can't always keep the interface to the user space unchanged when
fixing problems. When we fix drivers by adding hard_header_len or
removing hard_header_len, we ARE changing the interface. I did these
fixes a lot. I also changed skb->protocol when sending skbs for some
drivers, which in fact was also changing the interface. It is not
possible to keep the interface strictly unchanged, otherwise a lot of
problems will be impossible to fix.

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

[Willem de Bruijn]

On Wed, Oct 14, 2020 at 3:48 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Wed, Oct 14, 2020 at 8:12 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > On Wed, Oct 14, 2020 at 4:52 AM Xie He <xie.he.0141@gmail.com> wrote:
> > >
> > > On Sun, Oct 11, 2020 at 2:01 PM Willem de Bruijn
> > > <willemdebruijn.kernel@gmail.com> wrote:
> > > >
> > > > There is agreement that hard_header_len should be the length of link
> > > > layer headers visible to the upper layers, needed_headroom the
> > > > additional room required for headers that are not exposed, i.e., those
> > > > pushed inside ndo_start_xmit.
> > > >
> > > > The link layer header length also has to agree with the interface
> > > > hardware type (ARPHRD_..).
> > > >
> > > > Tunnel devices have not always been consistent in this, but today
> > > > "bare" ip tunnel devices without additional headers (ipip, sit, ..) do
> > > > match this and advertise 0 byte hard_header_len. Bareudp, vxlan and
> > > > geneve also conform to this. Known exception that probably needs to be
> > > > addressed is sit, which still advertises LL_MAX_HEADER and so has
> > > > exposed quite a few syzkaller issues. Side note, it is not entirely
> > > > clear to me what sets ARPHRD_TUNNEL et al apart from ARPHRD_NONE and
> > > > why they are needed.
> > > >
> > > > GRE devices advertise ARPHRD_IPGRE and GRETAP advertise ARPHRD_ETHER.
> > > > The second makes sense, as it appears as an Ethernet device. The first
> > > > should match "bare" ip tunnel devices, if following the above logic.
> > > > Indeed, this is what commit e271c7b4420d ("gre: do not keep the GRE
> > > > header around in collect medata mode") implements. It changes
> > > > dev->type to ARPHRD_NONE in collect_md mode.
> > > >
> > > > Some of the inconsistency comes from the various modes of the GRE
> > > > driver. Which brings us to ipgre_header_ops. It is set only in two
> > > > special cases.
> > > >
> > > > Commit 6a5f44d7a048 ("[IPV4] ip_gre: sendto/recvfrom NBMA address")
> > > > added ipgre_header_ops.parse to be able to receive the inner ip source
> > > > address with PF_PACKET recvfrom. And apparently relies on
> > > > ipgre_header_ops.create to be able to set an address, which implies
> > > > SOCK_DGRAM.
> > > >
> > > > The other special case, CONFIG_NET_IPGRE_BROADCAST, predates git. Its
> > > > implementation starts with the beautiful comment "/* Nice toy.
> > > > Unfortunately, useless in real life :-)". From the rest of that
> > > > detailed comment, it is not clear to me why it would need to expose
> > > > the headers. The example does not use packet sockets.
> > > >
> > > > A packet socket cannot know devices details such as which configurable
> > > > mode a device may be in. And different modes conflict with the basic
> > > > rule that for a given well defined link layer type, i.e., dev->type,
> > > > header length can be expected to be consistent. In an ideal world
> > > > these exceptions would not exist, therefore.
> > > >
> > > > Unfortunately, this is legacy behavior that will have to continue to
> > > > be supported.
> > >
> > > Thanks for your explanation. So header_ops for GRE devices is only
> > > used in 2 special situations. In normal situations, header_ops is not
> > > used for GRE devices. And we consider not using header_ops should be
> > > the ideal arrangement for GRE devices.
> > >
> > > Can we create a new dev->type (like ARPHRD_IPGRE_SPECIAL) for GRE
> > > devices that use header_ops? I guess changing dev->type will not
> > > affect the interface to the user space? This way we can solve the
> > > problem of the same dev->type having different hard_header_len values.
> >
> > But does that address any real issue?
>
> It doesn't address any issue visible when using. Just to solve the
> problem of the same dev->type having different hard_header_len values
> which you mentioned. Making this change will not affect the user in
> any way. So I think it is valuable to make this change.
>
> > If anything, it would make sense to keep ARHPHRD_IPGRE for tunnels
> > that expect headers and switch to ARPHRD_NONE for those that do not.
> > As the collect_md commit I mentioned above does.
>
> I thought we agreed that ideally GRE devices would not have
> header_ops. Currently GRE devices (in normal situations) indeed do not
> use header_ops (and use ARHPHRD_IPGRE as dev->type). I think we should
> keep this behavior.
>
> To solve the problem of the same dev->type having different
> hard_header_len values which you mentioned. I think we should create a
> new dev->type (ARPHRD_IPGRE_SPECIAL) for GRE devices that use
> header_ops.
>
> Also, for collect_md, I think we should use ARHPHRD_IPGRE. I see no
> reason to use ARPHRD_NONE.

What does ARPHRD_IPGRE define beyond ARPHRD_NONE? And same for
ARPHRD_TUNNEL variants. If they are indistinguishable, they are the
same and might as well have the same label.

> > > Also, for the second special situation, if there's no obvious reason
> > > to use header_ops, maybe we can consider removing header_ops for this
> > > situation.
> >
> > Unfortunately, there's no knowing if some application is using this
> > broadcast mode *with* a process using packet sockets.
>
> We can't always keep the interface to the user space unchanged when
> fixing problems. When we fix drivers by adding hard_header_len or
> removing hard_header_len, we ARE changing the interface. I did these
> fixes a lot. I also changed skb->protocol when sending skbs for some
> drivers, which in fact was also changing the interface. It is not
> possible to keep the interface strictly unchanged, otherwise a lot of
> problems will be impossible to fix.

I understand that for bug fixes this is sometimes unavoidable. I don't
think code cleanup is reason enough, though.

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

[Xie He]

On Wed, Oct 14, 2020 at 1:19 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Wed, Oct 14, 2020 at 3:48 PM Xie He <xie.he.0141@gmail.com> wrote:
> >
> > I thought we agreed that ideally GRE devices would not have
> > header_ops. Currently GRE devices (in normal situations) indeed do not
> > use header_ops (and use ARHPHRD_IPGRE as dev->type). I think we should
> > keep this behavior.
> >
> > To solve the problem of the same dev->type having different
> > hard_header_len values which you mentioned. I think we should create a
> > new dev->type (ARPHRD_IPGRE_SPECIAL) for GRE devices that use
> > header_ops.
> >
> > Also, for collect_md, I think we should use ARHPHRD_IPGRE. I see no
> > reason to use ARPHRD_NONE.
>
> What does ARPHRD_IPGRE define beyond ARPHRD_NONE? And same for
> ARPHRD_TUNNEL variants. If they are indistinguishable, they are the
> same and might as well have the same label.

It is indeed reasonable to keep devices indistinguishable to each
other having the same dev->type label. But I see a lot of devices in
the kernel without header_ops having different dev->type labels. For
example, ARPHRD_SLIP should be the same as ARPHRD_RAWIP. One feature
distinguishing these devices might be their dev->mtu.

GRE devices may have their special dev->mtu determined by the maximum
IP packet size and the GRE header length.

For ARPHRD_TUNNEL, it may also have its own dev->mtu. I also see it
has header_ops->parse_protocol (but it doesn't have
header_ops->create).

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

[Xie He]

On Wed, Oct 14, 2020 at 6:38 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Wed, Oct 14, 2020 at 1:19 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > On Wed, Oct 14, 2020 at 3:48 PM Xie He <xie.he.0141@gmail.com> wrote:
> > >
> > > I thought we agreed that ideally GRE devices would not have
> > > header_ops. Currently GRE devices (in normal situations) indeed do not
> > > use header_ops (and use ARHPHRD_IPGRE as dev->type). I think we should
> > > keep this behavior.
> > >
> > > To solve the problem of the same dev->type having different
> > > hard_header_len values which you mentioned. I think we should create a
> > > new dev->type (ARPHRD_IPGRE_SPECIAL) for GRE devices that use
> > > header_ops.
> > >
> > > Also, for collect_md, I think we should use ARHPHRD_IPGRE. I see no
> > > reason to use ARPHRD_NONE.
> >
> > What does ARPHRD_IPGRE define beyond ARPHRD_NONE? And same for
> > ARPHRD_TUNNEL variants. If they are indistinguishable, they are the
> > same and might as well have the same label.
>
> It is indeed reasonable to keep devices indistinguishable to each
> other having the same dev->type label. But I see a lot of devices in
> the kernel without header_ops having different dev->type labels. For
> example, ARPHRD_SLIP should be the same as ARPHRD_RAWIP. One feature
> distinguishing these devices might be their dev->mtu.
>
> GRE devices may have their special dev->mtu determined by the maximum
> IP packet size and the GRE header length.
>
> For ARPHRD_TUNNEL, it may also have its own dev->mtu. I also see it
> has header_ops->parse_protocol (but it doesn't have
> header_ops->create).

Actually I think dev->type can be seen from user space. For example,
when you type "ip link", it will display the link type for you. So I
think it is useful to keep different dev->type labels without merging
them even if they appear to have no difference.

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

[Willem de Bruijn]

On Wed, Oct 14, 2020 at 10:25 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Wed, Oct 14, 2020 at 6:38 PM Xie He <xie.he.0141@gmail.com> wrote:
> >
> > On Wed, Oct 14, 2020 at 1:19 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > On Wed, Oct 14, 2020 at 3:48 PM Xie He <xie.he.0141@gmail.com> wrote:
> > > >
> > > > I thought we agreed that ideally GRE devices would not have
> > > > header_ops. Currently GRE devices (in normal situations) indeed do not
> > > > use header_ops (and use ARHPHRD_IPGRE as dev->type). I think we should
> > > > keep this behavior.
> > > >
> > > > To solve the problem of the same dev->type having different
> > > > hard_header_len values which you mentioned. I think we should create a
> > > > new dev->type (ARPHRD_IPGRE_SPECIAL) for GRE devices that use
> > > > header_ops.
> > > >
> > > > Also, for collect_md, I think we should use ARHPHRD_IPGRE. I see no
> > > > reason to use ARPHRD_NONE.
> > >
> > > What does ARPHRD_IPGRE define beyond ARPHRD_NONE? And same for
> > > ARPHRD_TUNNEL variants. If they are indistinguishable, they are the
> > > same and might as well have the same label.
> >
> > It is indeed reasonable to keep devices indistinguishable to each
> > other having the same dev->type label. But I see a lot of devices in
> > the kernel without header_ops having different dev->type labels. For
> > example, ARPHRD_SLIP should be the same as ARPHRD_RAWIP. One feature
> > distinguishing these devices might be their dev->mtu.
> >
> > GRE devices may have their special dev->mtu determined by the maximum
> > IP packet size and the GRE header length.
> >
> > For ARPHRD_TUNNEL, it may also have its own dev->mtu. I also see it
> > has header_ops->parse_protocol (but it doesn't have
> > header_ops->create).
>
> Actually I think dev->type can be seen from user space. For example,
> when you type "ip link", it will display the link type for you. So I
> think it is useful to keep different dev->type labels without merging
> them even if they appear to have no difference.

Ah, indeed. These constants are matched in iproute2 in lib/ll_types.c
to string representations.

Good catch. Yes, then they have to stay as is.

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

[Xie He]

On Thu, Oct 15, 2020 at 6:42 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Wed, Oct 14, 2020 at 10:25 PM Xie He <xie.he.0141@gmail.com> wrote:
> >
> > Actually I think dev->type can be seen from user space. For example,
> > when you type "ip link", it will display the link type for you. So I
> > think it is useful to keep different dev->type labels without merging
> > them even if they appear to have no difference.
>
> Ah, indeed. These constants are matched in iproute2 in lib/ll_types.c
> to string representations.
>
> Good catch. Yes, then they have to stay as is.

So in this case it may be better to keep dev->type as ARHPHRD_IPGRE
for GRE devices with or without header_ops. This way we can avoid the
need to update iproute2.

We can still consider changing GRE devices in collect_md mode from
ARPHRD_NONE to ARHPHRD_IPGRE. The original author who changed it to
ARPHRD_NONE might assume ARHPHRD_IPGRE is for GRE devices with
header_ops, so he felt he needed to distinguish GRE devices without
header_ops by dev->type. However, ARHPHRD_IPGRE is actually already
used for GRE devices with header_ops AND without header_ops. So it
doesn't hurt to use ARHPHRD_IPGRE for GRE devices in collect_md mode,
too. This would also make iproute2 to correctly display GRE devices in
collect_md mode as GRE devices.

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

[Willem de Bruijn]

On Thu, Oct 15, 2020 at 3:19 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Thu, Oct 15, 2020 at 6:42 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > On Wed, Oct 14, 2020 at 10:25 PM Xie He <xie.he.0141@gmail.com> wrote:
> > >
> > > Actually I think dev->type can be seen from user space. For example,
> > > when you type "ip link", it will display the link type for you. So I
> > > think it is useful to keep different dev->type labels without merging
> > > them even if they appear to have no difference.
> >
> > Ah, indeed. These constants are matched in iproute2 in lib/ll_types.c
> > to string representations.
> >
> > Good catch. Yes, then they have to stay as is.
>
> So in this case it may be better to keep dev->type as ARHPHRD_IPGRE
> for GRE devices with or without header_ops. This way we can avoid the
> need to update iproute2.
>
> We can still consider changing GRE devices in collect_md mode from
> ARPHRD_NONE to ARHPHRD_IPGRE. The original author who changed it to
> ARPHRD_NONE might assume ARHPHRD_IPGRE is for GRE devices with
> header_ops, so he felt he needed to distinguish GRE devices without
> header_ops by dev->type. However, ARHPHRD_IPGRE is actually already
> used for GRE devices with header_ops AND without header_ops. So it
> doesn't hurt to use ARHPHRD_IPGRE for GRE devices in collect_md mode,
> too. This would also make iproute2 to correctly display GRE devices in
> collect_md mode as GRE devices.

That's true. Whenever you make user visible changes there is some
chance of breakage. I'm not sure it's worth the risk here. The present
state is perhaps not ideal, but not broken.

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

[Xie He]

On Sun, Oct 11, 2020 at 12:11 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> @@ -626,8 +626,7 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
>
>         if (dev->header_ops) {
>                 /* Need space for new headers */
> -               if (skb_cow_head(skb, dev->needed_headroom -
> -                                     (tunnel->hlen + sizeof(struct iphdr))))
> +               if (skb_cow_head(skb, dev->hard_header_len))
>                         goto free_skb;
>
>                 tnl_params = (const struct iphdr *)skb->data;

As I understand, the skb_cow functions are for ensuring enough header
space before skb->data. (Right?) However, at this stage our skb->data
is already at the outer IP header, I think we don't need to request
additional header space before the outer IP header.

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

[Cong Wang]

On Sun, Oct 11, 2020 at 3:46 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Sun, Oct 11, 2020 at 12:11 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > @@ -626,8 +626,7 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
> >
> >         if (dev->header_ops) {
> >                 /* Need space for new headers */
> > -               if (skb_cow_head(skb, dev->needed_headroom -
> > -                                     (tunnel->hlen + sizeof(struct iphdr))))
> > +               if (skb_cow_head(skb, dev->hard_header_len))
> >                         goto free_skb;
> >
> >                 tnl_params = (const struct iphdr *)skb->data;
>
> As I understand, the skb_cow functions are for ensuring enough header
> space before skb->data. (Right?) However, at this stage our skb->data
> is already at the outer IP header, I think we don't need to request
> additional header space before the outer IP header.

Good point, I thought skb_headroom() == dev->hard_header_len,
but skb->data already points to the tunnel header like you said, so
we should pass 0 to skb_cow_head() here.

Thanks.