Re: [PATCH net-next v5 02/14] ip_tunnel: make __iptunnel_pull_header() return a drop reason

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] At this commit, vxlan_rcv() in drivers/net/vxlan/vxlan_core.c
  still ignores the drop reason that __iptunnel_pull_header() now…

--

ip_tunnel: make __iptunnel_pull_header() return a drop reason

__iptunnel_pull_header() and iptunnel_pull_header() used to return -ENOMEM.
They now return an enum skb_drop_reason: the reason from
pskb_may_pull_reason() when a pull fails, SKB_DROP_REASON_NOMEM when
skb_unclone() fails, and SKB_NOT_DROPPED_YET on success. The three callers
in ip_gre and ip6_gre that tested the result with "< 0" now test for a
non-zero value.

> diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
> index bab42b9e277f7..6c2855adff289 100644
> --- a/net/ipv4/ip_tunnel_core.c
> +++ b/net/ipv4/ip_tunnel_core.c

[ ... ]

>  	if (!raw_proto && inner_proto == htons(ETH_P_TEB)) {
>  		struct ethhdr *eh;
>
> -		if (unlikely(!pskb_may_pull(skb, ETH_HLEN)))
> -			return -ENOMEM;
> +		reason = pskb_may_pull_reason(skb, ETH_HLEN);
> +		if (unlikely(reason))
> +			return reason;
                        ^^^^

[Severity: Low]
After this change, a truncated inner Ethernet header returns
SKB_DROP_REASON_PKT_TOO_SMALL here. Should vxlan_rcv() be updated in this
patch as well? At this commit it still throws away the returned reason:

drivers/net/vxlan/vxlan_core.c:vxlan_rcv() {
    ...
	if (__iptunnel_pull_header(skb, VXLAN_HLEN, protocol, raw_proto,
				   !net_eq(vxlan->net, dev_net(vxlan->dev)))) {
		reason = SKB_DROP_REASON_NOMEM;
		goto drop;
	}
    ...
}

This means a non-GPE VXLAN packet (protocol ETH_P_TEB, raw_proto false)
with an inner frame shorter than ETH_HLEN is still reported as NOMEM. The
commit message uses exactly this case as its example.

Only the drop reason seen through skb:kfree_skb and drop_monitor is
affected. The packet is dropped either way.

The next patch in the series, "vxlan: report the drop reason of
__iptunnel_pull_header()", changes this to
reason = __iptunnel_pull_header(...), so the problem only exists partway
through the series.

Would it make sense to fold that change into this patch? Alternatively,
the commit message could say that vxlan is converted separately.

>
>  		eh = (struct ethhdr *)skb->data;
>  		if (likely(eth_proto_is_802_3(eh->h_proto)))

[ ... ]

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