Re: [PATCH net v4 1/2] ip6_gre: fix hardware header length for NBMA tunnels

[Ido Schimmel <idosch@nvidia.com>]

On Thu, Aug 13, 2026 at 12:22:34AM +0800, Zhiling Zou wrote:
> ip6gre_tnl_link_config_route() accumulates the lower device's hardware
> header length into dev->hard_header_len whenever header_ops is set. This
> is incorrect for both users of header_ops.
> 
> ip6gretap and ip6erspan have a fixed Ethernet hardware header length.
> For an NBMA ip6gre tunnel, ip6gre_header() creates only the GRE header,
> the optional FOU or GUE header, and the outer IPv6 header. The lower
> device header is headroom needed later, not part of the tunnel device's
> hardware header.
> 
> Keep the lower device header in needed_headroom. Set hard_header_len to
> the tunnel header length only for ARPHRD_IP6GRE devices with header_ops,
> and leave the fixed Ethernet header length unchanged for tap and erspan
> devices.
> 
> Fixes: 832ba596494b ("net: ip6_gre: set dev->hard_header_len when using header_ops")
> Cc: stable@vger.kernel.org
> Reported-by: Vega <vega@nebusec.ai>

It wasn't reported by Vega.

> Suggested-by: Ido Schimmel <idosch@nvidia.com>
> Signed-off-by: Zhiling Zou <zhilinz@nebusec.ai>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

You are expected to reply to AI feedback.

"
Patch authors are expected to proactively look into the AI-generated
reviews and handle such feedback as any other kind of review: either
debate it or address it. In both cases a reply on the mailing list is
expected.
"

https://docs.kernel.org/next/process/maintainer-netdev.html#review-timelines

> ---
>  net/ipv6/ip6_gre.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
> index b843116e9b703..70c1710910203 100644
> --- a/net/ipv6/ip6_gre.c
> +++ b/net/ipv6/ip6_gre.c
> @@ -1137,13 +1137,8 @@ static void ip6gre_tnl_link_config_route(struct ip6_tnl *t, int set_mtu,
>  			return;
>  
>  		if (rt->dst.dev) {
> -			unsigned short dst_len = rt->dst.dev->hard_header_len +
> -						 t_hlen;
> -
> -			if (t->dev->header_ops)
> -				dev->hard_header_len = dst_len;
> -			else
> -				dev->needed_headroom = dst_len;
> +			dev->needed_headroom = rt->dst.dev->hard_header_len +
> +					       t_hlen;

"
Since dev->hard_header_len was already set to t_hlen in ip6gre_calc_hlen()
for NBMA tunnels, doesn't adding t_hlen here double-count the tunnel header
length?
"

No. ip6gre_tnl_link_config_route() is a NOP for NBMA tunnels since they
don't have IP6_TNL_F_CAP_XMIT set.

>  
>  			if (set_mtu) {
>  				int mtu = rt->dst.dev->mtu - t_hlen;
> @@ -1171,8 +1166,8 @@ static int ip6gre_calc_hlen(struct ip6_tnl *tunnel)
>  
>  	t_hlen = tunnel->hlen + sizeof(struct ipv6hdr);
>  
> -	if (tunnel->dev->header_ops)
> -		tunnel->dev->hard_header_len = LL_MAX_HEADER + t_hlen;
> +	if (tunnel->dev->header_ops && tunnel->dev->type == ARPHRD_IP6GRE)
> +		tunnel->dev->hard_header_len = t_hlen;
>  	else
>  		tunnel->dev->needed_headroom = LL_MAX_HEADER + t_hlen;

"
By removing the LL_MAX_HEADER fallback for NBMA ip6gre tunnels, what happens
if rt6_lookup() fails in ip6gre_tnl_link_config_route() (which is expected
for NBMA tunnels with an ANY remote address)?
"

ip6gre_tnl_link_config_route() is not executed for NBMA tunnels.

"
Without the fallback, dev->needed_headroom would remain 0, and the total
allocated headroom would just be t_hlen. When transmitted, the packet will
require t_hlen + physical_dev->hard_header_len bytes, which would miss the
physical device's headroom and force a reallocation and copy via
pskb_expand_head() on every transmitted packet.
"

dev->needed_headroom was 0 even before the patch, but it's irrelevant
since NBMA tunnels seem to be completely broken. See below.

"
This isn't a bug introduced by this patch, but I noticed NBMA IPv6 GRE 
tunnels might be double-encapsulating packets and corrupting headers.

In net/ipv6/ip6_gre.c:__gre6_xmit(), the code relies on the dummy header
pushed by ip6gre_header() to read the destination address:

    if (dev->header_ops && dev->type == ARPHRD_IP6GRE)
        fl6->daddr = ((struct ipv6hdr *)skb->data)->daddr;

However, the dummy header is not pulled before pushing the real outer GRE
and IPv6 headers. Doesn't this leave the dummy headers in the payload,
causing double-encapsulation ([Real IPv6] [Real GRE] [Dummy IPv6]
[Dummy GRE] [Payload])?
"

This does look buggy, but NBMA tunnels are completely broken.
ip6gre_tunnel_xmit() is passing the wrong addresses to
ip6_tnl_xmit_ctl(), so it is always returning 0 and ip6gre_tunnel_xmit()
is dropping the packets. ip6gre_tunnel_xmit() is deriving the addresses
from the tunnel configuration (ANY) instead of from the packet, as
should be done with an NBMA tunnel.

And:

"
This is also a pre-existing issue, but looking at 
net/ipv6/ip6_gre.c:ip6gre_header():

    ipv6h = skb_push(skb, needed);
    ...
    p = (__be16 *)(ipv6h + 1);
    p[0] = ip_tunnel_flags_to_be16(t->parms.o_flags);
    p[1] = htons(type);

This function allocates the full header length via skb_push() and 
initializes the IPv6 header and the first 4 bytes of the GRE header.

Since the remaining bytes of t->hlen (up to 24 bytes for GRE keys, 
checksums, and sequence) are not initialized, and __gre6_xmit() fails 
to pull this dummy header, does this leak uninitialized kernel heap memory 
to the network in every packet?
"

Again, these packets don't even reach __gre6_xmit(), so they don't end
up on the wire.

"
This isn't a bug introduced by this patch, but now that hard_header_len is
set to exactly t_hlen, does it make the disagreement with ip6gre_header()
explicit?
"

Yes, it's returning the wrong value (needs to return 'needed'), but it's
pre-existing and NBMA tunnels wouldn't work even if this is fixed.