Re: [PATCH net-next v2 4/8] ip_gre: add drop reasons to the RX path

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 1 (1)

- [Low] Claim: labelling GRE RX pull failures SKB_DROP_REASON_HDR_TRUNC
  misattributes allocation failures, both for the two new `if…

Pre-existing issues:
- [Low] Claim: the new family-agnostic
  SKB_DROP_REASON_GRE_TUNNEL_NOT_FOUND is wired up only in…

--

ip_gre: add drop reasons to the RX path

This adds SKB_DROP_REASON_GRE_TUNNEL_NOT_FOUND and threads a drop reason
output parameter through erspan_rcv(), __ipgre_rcv() and ipgre_rcv() so
that gre_rcv() can pass a specific reason to kfree_skb_reason().  Header
pull failures are reported as SKB_DROP_REASON_HDR_TRUNC and metadata dst
allocation failures as SKB_DROP_REASON_NOMEM.

A couple of small questions below, both about the reason values rather
than the plumbing.

> diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
> index 6ae7a604722d3..fa8bd552122ff 100644
> --- a/include/net/dropreason-core.h
> +++ b/include/net/dropreason-core.h

[ ... ]

> @@ -637,6 +638,11 @@ enum skb_drop_reason {
>  	SKB_DROP_REASON_GRE_INVALID_HDR,
>  	/** @SKB_DROP_REASON_GRE_CSUM: GRE checksum error */
>  	SKB_DROP_REASON_GRE_CSUM,
> +	/**
> +	 * @SKB_DROP_REASON_GRE_TUNNEL_NOT_FOUND: no GRE tunnel found for the
> +	 * endpoints and the key the packet carries.
> +	 */
> +	SKB_DROP_REASON_GRE_TUNNEL_NOT_FOUND,

[Severity: Low]

This isn't an issue introduced by this patch, but the new reason is
documented in family-neutral wording ("no GRE tunnel found for the
endpoints and the key") while only net/ipv4/ip_gre.c sets it in this
commit.  Until the IPv6 side is converted, a GRE-over-IPv6 packet with no
matching tunnel still takes:

net/ipv6/ip6_gre.c:gre_rcv() -> ip6gre_rcv() -> kfree_skb_reason(skb,
SKB_DROP_REASON_NOT_SPECIFIED)

so ip6gre, ip6gretap and ERSPAN-over-IPv6 lookup failures are still
indistinguishable from unspecified drops.

For the record, this is closed later in the same series: the following
patch, "ip6_gre: add drop reasons to the RX path", converts
__ip6gre_rcv()/ip6gre_rcv() and ip6erspan_rcv() to the same output
parameter scheme and ends both lookup-failure paths with
*reason = SKB_DROP_REASON_GRE_TUNNEL_NOT_FOUND.  Would it be worth
mentioning in this changelog that the IPv6 counterpart follows, so the
intermediate state is not read as a scope gap?

> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 1894c5746a737..4d9bb6d186ae5 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c

[ ... ]

> @@ -356,15 +362,17 @@ static int erspan_rcv(struct sk_buff *skb, struct tnl_ptk_info *tpi,
>  		ip_tunnel_rcv(tunnel, skb, tpi, tun_dst, log_ecn_error);
>  		return PACKET_RCVD;
>  	}
> +	*reason = SKB_DROP_REASON_GRE_TUNNEL_NOT_FOUND;
>  	return PACKET_REJECT;
>  
>  drop:
> -	kfree_skb(skb);
> +	kfree_skb_reason(skb, SKB_DROP_REASON_HDR_TRUNC);
>  	return PACKET_RCVD;
>  }

[Severity: Low]

The only way to reach this drop label in erspan_rcv() is the
__iptunnel_pull_header() failure, and that helper does not only fail on
truncation:

include/net/ip_tunnels.h:iptunnel_pull_offloads() {
	if (skb_is_gso(skb)) {
		int err;

		err = skb_unclone(skb, GFP_ATOMIC);
		if (unlikely(err))
			return err;
	...
}

So for a cloned GSO skb under memory pressure the drop is an allocation
failure but gets reported as SKB_DROP_REASON_HDR_TRUNC.  The commit
message says "The header pull failures reuse SKB_DROP_REASON_HDR_TRUNC",
which covers the pskb_may_pull() cases well, but does it also intend to
cover the skb_unclone() case?

Note the errno cannot be used to tell the two apart, since
__iptunnel_pull_header() returns -ENOMEM for every failure mode, and
there is precedent for the coarse label in the other direction:
vxlan_rcv() tags every __iptunnel_pull_header() failure, truncation
included, as SKB_DROP_REASON_NOMEM.  The two new
*reason = SKB_DROP_REASON_HDR_TRUNC assignments on the pskb_may_pull()
branches above match the HDR_TRUNC kernel-doc exactly, so this question
is only about the drop label.

> @@ -400,22 +408,25 @@ static int __ipgre_rcv(struct sk_buff *skb, const struct tnl_ptk_info *tpi,
>  
>  			tun_id = key32_to_tunnel_id(tpi->key);
>  			tun_dst = ip_tun_rx_dst(skb, flags, tun_id, 0);
> -			if (!tun_dst)
> +			if (!tun_dst) {
> +				*reason = SKB_DROP_REASON_NOMEM;
>  				return PACKET_REJECT;
> +			}
>  		}
>  
>  		ip_tunnel_rcv(tunnel, skb, tpi, tun_dst, log_ecn_error);
>  		return PACKET_RCVD;
>  	}
> +	*reason = SKB_DROP_REASON_GRE_TUNNEL_NOT_FOUND;
>  	return PACKET_NEXT;
>  
>  drop:
> -	kfree_skb(skb);
> +	kfree_skb_reason(skb, SKB_DROP_REASON_HDR_TRUNC);
>  	return PACKET_RCVD;
>  }

The same question applies here: this drop label is reached only from
__iptunnel_pull_header(skb, hdr_len, tpi->proto, raw_proto, false) < 0,
so the skb_unclone(GFP_ATOMIC) failure inside iptunnel_pull_offloads() is
also reported as SKB_DROP_REASON_HDR_TRUNC on the main IPv4 GRE receive
path.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913034937.875068-1-littlesmilingcloud%40gmail.com