Re: [PATCH net-next v2 8/8] ip6_tunnel: add drop reasons to the transmit path

[netdev-bot+sashiko]

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

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

- [High] ipxip6_tnl_xmit() clobbers the drop reason produced by
  ip6_tnl_xmit() on every failure.
- [Low] In the NBMA branch of ip6_tnl_xmit() the IPv4-payload case labels
  a missing rtable as SKB_DROP_REASON_IP_OUTNOROUTES…

--

ip6_tunnel: add drop reasons to the transmit path

This converts the IPv6 tunnel transmit path to report SKB drop reasons,
giving ip6_tnl_xmit() a new output parameter that is threaded through
ipxip6_tnl_xmit(), __gre6_xmit() and the ip6gre_xmit_*() helpers, and
switching the two tx_err paths from kfree_skb() to kfree_skb_reason().

A couple of questions about the reason values below.

> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index 458ce328311b4..d804c67c4be3b 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -1097,6 +1097,7 @@ EXPORT_SYMBOL_GPL(ip6_tnl_xmit_ctl);
>   *   @encap_limit: encapsulation limit
>   *   @pmtu: Path MTU is stored if packet is too big
>   *   @proto: next header value
> + *   @reason: drop reason, only written when the packet is dropped
>   *
>   * Description:
>   *   Build new header and do some sanity checks on the packet before sending

[ ... ]

> @@ -1162,8 +1167,10 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev, __u8 dsfield,
>  		} else if (payload_protocol == htons(ETH_P_IP)) {
>  			const struct rtable *rt = skb_rtable(skb);
>  
> -			if (!rt)
> +			if (!rt) {
> +				*reason = SKB_DROP_REASON_IP_OUTNOROUTES;
>  				goto tx_err_link_failure;
> +			}

[Severity: Low]
Should this be SKB_DROP_REASON_NO_TX_TARGET instead?

No route lookup has happened yet at this point in the NBMA branch of
ip6_tnl_xmit().  skb_rtable() is just dst_rtable(skb_dst(skb)), so !rt is the
same "the skb carries no dst" condition that the sibling IPv6-payload branch a
few lines above labels NO_TX_TARGET:

			if (!skb_dst(skb)) {
				*reason = SKB_DROP_REASON_NO_TX_TARGET;
				goto tx_err_link_failure;
			}

dropreason-core.h documents IP_OUTNOROUTES as "route lookup failed" and
NO_TX_TARGET as "no target found for xmit", and ip_tunnel_xmit() in
net/ipv4/ip_tunnel.c uses NO_TX_TARGET for the analogous NBMA missing-dst
case:

		if (!skb_dst(skb)) {
			DEV_STATS_INC(dev, tx_fifo_errors);
			reason = SKB_DROP_REASON_NO_TX_TARGET;
			goto tx_error;
		}

As written, IP_OUTNOROUTES now covers two different conditions inside
ip6_tnl_xmit(), since it is also used for the genuine ip6_route_output() and
xfrm_lookup() failures further down.

[ ... ]

> @@ -1408,13 +1435,15 @@ ipxip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev,
>  	fl6.flowi6_uid = sock_net_uid(dev_net(dev), NULL);
>  	dsfield = INET_ECN_encapsulate(dsfield, orig_dsfield);
>  
> -	if (iptunnel_handle_offloads(skb, SKB_GSO_IPXIP6))
> +	if (iptunnel_handle_offloads(skb, SKB_GSO_IPXIP6)) {
> +		*reason = SKB_DROP_REASON_NOMEM;
>  		return -1;
> +	}
>  
>  	skb_set_inner_ipproto(skb, protocol);
>  
>  	err = ip6_tnl_xmit(skb, dev, dsfield, &fl6, encap_limit, &mtu,
> -			   protocol);
> +			   protocol, reason);
>  	if (err != 0) {
>  		/* XXX: send ICMP error even if DF is not set. */
>  		if (err == -EMSGSIZE)
> @@ -1429,6 +1458,7 @@ ipxip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev,
>  			default:
>  				break;
>  			}
> +		*reason = SKB_DROP_REASON_PKT_TOO_BIG;
>  		return -1;
>  	}

[Severity: High]
Does this overwrite the reason that ip6_tnl_xmit() just stored?

The if (err == -EMSGSIZE) body is only the ICMP-emitting switch, so the
*reason = SKB_DROP_REASON_PKT_TOO_BIG assignment runs for every non-zero err,
not just for -EMSGSIZE.

ip6_tnl_xmit() sets a distinct reason on each of its failure paths
(NO_TX_TARGET, NEIGH_CREATEFAIL, IP_OUTNOROUTES, DEV_READY, RECURSION_LIMIT,
NOMEM, TNL_ENCAP), and all of them are replaced here before
ip6_tnl_start_xmit() consumes the value:

  ip6_tnl_start_xmit()
    ret = ipxip6_tnl_xmit(skb, dev, ipproto, &reason);
      err = ip6_tnl_xmit(..., reason);   /* sets IP_OUTNOROUTES */
      *reason = SKB_DROP_REASON_PKT_TOO_BIG;
    tx_err:
      kfree_skb_reason(skb, reason);     /* reports PKT_TOO_BIG */

So an ip6ip6/ipip6/mplsip6 tunnel with an unreachable remote, an allocation
failure in skb_cow_head(), a local routing loop (tdev == dev) or an
ip6_tnl_xmit_ctl() refusal all end up reported as PKT_TOO_BIG.

For the one case where the assignment would be correct, -EMSGSIZE,
ip6_tnl_xmit() has already stored PKT_TOO_BIG, so the statement looks
redundant there.

This also disagrees with the kernel-doc added in the same patch,
"@reason: drop reason, only written when the packet is dropped", which makes
the callee the owner of the value, and with the ip6_gre callers
(__gre6_xmit(), ip6gre_xmit_ipv4(), ip6gre_xmit_ipv6(),
ip6erspan_tunnel_xmit()) which propagate the callee's reason untouched.

Would moving the assignment inside the err == -EMSGSIZE block, or dropping
it entirely, be what was intended here?

[ ... ]

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