Re: [PATCH net-next v2 8/8] ip6_tunnel: add drop reasons to the transmit path
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-14 04:37:23
Also in:
lkml
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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.
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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