Re: [PATCH net-next v5 05/14] ip6_tunnel: add drop reasons to the receive path

[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] In ipxip6_rcv() (net/ipv6/ip6_tunnel.c, line 978), every packet
  that ip6_tnl_rcv_ctl() refuses is now dropped with…

--

ip6_tunnel: add drop reasons to the receive path

This patch replaces the plain kfree_skb() calls in __ip6_tnl_rcv() and
ipxip6_rcv() with kfree_skb_reason(). Where possible it reuses the
reasons that ip_tunnel_rcv() and the length helpers already return. It
also gives the early drops in ipxip6_rcv() their own reasons.

> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index d5ff50a2ac017..52f6a38657b9f 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c

[ ... ]

> @@ -952,21 +965,30 @@ static int ipxip6_rcv(struct sk_buff *skb, u8 ipproto,
>  	if (t) {
>  		u8 tproto = READ_ONCE(t->parms.proto);
>  
> -		if (tproto != ipproto && tproto != 0)
> +		if (tproto != ipproto && tproto != 0) {
> +			reason = SKB_DROP_REASON_UNHANDLED_PROTO;
>  			goto drop;
> -		if (!xfrm6_policy_check(NULL, XFRM_POLICY_IN, skb))
> +		}
> +		if (!xfrm6_policy_check(NULL, XFRM_POLICY_IN, skb)) {
> +			reason = SKB_DROP_REASON_XFRM_POLICY;
>  			goto drop;
> +		}
>  		ipv6h = ipv6_hdr(skb);
> -		if (!ip6_tnl_rcv_ctl(t, &ipv6h->daddr, &ipv6h->saddr))
> +		if (!ip6_tnl_rcv_ctl(t, &ipv6h->daddr, &ipv6h->saddr)) {
> +			reason = SKB_DROP_REASON_DEV_READY;
>  			goto drop;
> +		}

[Severity: Low]
Does SKB_DROP_REASON_DEV_READY fit any of the cases where
ip6_tnl_rcv_ctl() refuses a packet?

include/net/dropreason-core.h documents it as:

	 * @SKB_DROP_REASON_DEV_READY: the device is not ready to xmit/recv due to
	 * any of its data structure that is not up/ready/initialized,
	 * e.g., the IFF_UP is not set, or driver specific tun->tfiles[txq]
	 * is not initialized

ip6_tnl_rcv_ctl() does not look at device state. It returns 0 in three
cases:

  - the tunnel has neither IP6_TNL_F_CAP_RCV nor a per-packet capability
    that allows receive for these addresses
  - laddr is not multicast and is not a non-tentative local address
  - raddr is local and IP6_TNL_F_ALLOW_LOCAL_REMOTE is not set

The IFF_UP example cannot apply either. ip6_tnl_lookup() only returns
tunnels that are already up:

ip6_tnl_lookup() {
    ...
		    !(t->dev->flags & IFF_UP))
			continue;
    ...
}

So dropwatch, drop_monitor and BPF users of skb:kfree_skb would report
a misconfigured endpoint, a missing capability, or a spoofed or looped
source as a device readiness problem.

The commit message says DEV_READY "describes only the first" case and
leaves a proper split for a follow-up. The later patches "ip6_tunnel:
make ip6_tnl_xmit() return a drop reason" and "ip6_gre: add drop
reasons to the transmit path" use DEV_READY the same way for
ip6_tnl_xmit_ctl(). By the end of the series, the DEV_READY kernel-doc
is still unchanged, but the series does update the docs for
TUNNEL_INVALID_HDR, TUNNEL_NOT_FOUND and RECURSION_LIMIT.

Would it be better to widen the DEV_READY kernel-doc, or to add a
tunnel address/capability reason for these call sites?

> -		if (iptunnel_pull_header(skb, 0, tpi->proto, false))
> +		reason = iptunnel_pull_header(skb, 0, tpi->proto, false);
> +		if (reason)
>  			goto drop;

[ ... ]

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