Re: [PATCH net-next] gre: Count GRE packet drops

[Gal Pressman <gal@nvidia.com>]

Addressing Sashiko's comments:
https://sashiko.dev/#/patchset/20260409090945.1542440-1-gal@nvidia.com

On 09/04/2026 12:09, Gal Pressman wrote:
> GRE is silently dropping packets without updating statistics.
> 
> In case of drop, increment rx_dropped counter to provide visibility into
> packet loss. For the case where no GRE protocol handler is registered,
> use rx_nohandler.
> 
> Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
> Reviewed-by: Nimrod Oren <noren@nvidia.com>
> Signed-off-by: Gal Pressman <gal@nvidia.com>
> ---
>  net/ipv4/gre_demux.c | 8 ++++++--
>  net/ipv4/ip_gre.c    | 1 +
>  net/ipv6/ip6_gre.c   | 1 +
>  3 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/gre_demux.c b/net/ipv4/gre_demux.c
> index dafd68f3436a..96fd7dc6d82d 100644
> --- a/net/ipv4/gre_demux.c
> +++ b/net/ipv4/gre_demux.c
> @@ -159,14 +159,18 @@ static int gre_rcv(struct sk_buff *skb)
>  	rcu_read_lock();
>  	proto = rcu_dereference(gre_proto[ver]);
>  	if (!proto || !proto->handler)
> -		goto drop_unlock;
> +		goto drop_nohandler;
>  	ret = proto->handler(skb);
>  	rcu_read_unlock();
>  	return ret;
>  
> -drop_unlock:
> +drop_nohandler:
>  	rcu_read_unlock();
> +	dev_core_stats_rx_nohandler_inc(skb->dev);
> +	kfree_skb(skb);
> +	return NET_RX_DROP;
>  drop:
> +	dev_core_stats_rx_dropped_inc(skb->dev);

---
At this point in gre_rcv(), does skb->dev point to the underlying
ingress physical interface (e.g., eth0) rather than a GRE tunnel device?
If so, does incrementing the L2 physical device counters for an L4 protocol
error case pollute the physical interface metrics? Unconfigured
GRE traffic from the network might cause physical device drop counters to
spike, which could be misdiagnosed as hardware issues.
Would it be better to expose these via protocol-specific MIB counters or by
passing a specific drop reason to kfree_skb_reason()?
---

Yes, it points to the underlying device, and that makes sense IMO.

At this point, the packets are still not associated with any tunnel (it
might not even exist).

>  	kfree_skb(skb);
>  	return NET_RX_DROP;
>  }
> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
> index dafcc0dcd77a..63fc8556b475 100644
> --- a/net/ipv6/ip6_gre.c
> +++ b/net/ipv6/ip6_gre.c
> @@ -593,6 +593,7 @@ static int gre_rcv(struct sk_buff *skb)
>  out:
>  	icmpv6_send(skb, ICMPV6_DEST_UNREACH, ICMPV6_PORT_UNREACH, 0);
>  drop:
> +	dev_core_stats_rx_dropped_inc(skb->dev);
>  	kfree_skb(skb);
>  	return 0;
>  }

---
Does this fully account for silent packet drops during decapsulation?

Looking at erspan_rcv() and ipgre_rcv() which are called earlier in this
function, if they fail internally (for example, if __iptunnel_pull_header()
fails), they free the skb and return PACKET_RCVD:
net/ipv4/ip_gre.c:erspan_rcv() {
    ...
    if (__iptunnel_pull_header(skb, len, htons(ETH_P_TEB), false, false)
< 0)
        goto drop;
    ...
drop:
    kfree_skb(skb);
    return PACKET_RCVD;
}
Since gre_rcv() checks for PACKET_RCVD and returns 0:
    if (erspan_rcv(skb, &tpi, hdr_len) == PACKET_RCVD)
        return 0;
Does this bypass the newly added dev_core_stats_rx_dropped_inc() call under
the drop label entirely, leaving these packets silently dropped?
---

These are also silent drops, but these should be counted towards the
tunnel device, maybe as a separate patch.