Re: [PATCH net-next v5 13/14] ip6_gre: add drop reasons to the transmit path

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] On a collect_md ip6gre/ip6gretap device, ip6gre_xmit_ipv6()
  (net/ipv6/ip6_gre.c:839-840) still compares the inner IPv6 source…
- [Low] ip6gre_tunnel_xmit() (net/ipv6/ip6_gre.c:901-904) and
  ip6erspan_tunnel_xmit() (lines 967-970) label every refusal by…

--

ip6_gre: add drop reasons to the transmit path

ip6gre_tunnel_xmit() and ip6erspan_tunnel_xmit() now free dropped packets
with kfree_skb_reason() instead of a plain kfree_skb(). The handlers' own
drops get specific reasons. For a collect_md ip6gre device, the metadata
check now runs right after skb_tunnel_info_txcheck(), so a packet without
usable IPv6 metadata gets SKB_DROP_REASON_TUNNEL_TXINFO.

[Severity: Low]
Is SKB_DROP_REASON_DEV_READY the right label for every refusal by
ip6_tnl_xmit_ctl()? The same mapping is added in ip6erspan_tunnel_xmit().

One of the refusal paths in ip6_tnl_xmit_ctl() is an explicit loop
detection:

net/ipv6/ip6_tunnel.c:ip6_tnl_xmit_ctl() {
    ...
		else if (!(p->flags & IP6_TNL_F_ALLOW_LOCAL_REMOTE) &&
			 !ipv6_addr_is_multicast(raddr) &&
			 unlikely(ipv6_chk_addr_and_flags(net, raddr, ldev,
							  true, 0, IFA_F_TENTATIVE)))
			pr_warn_ratelimited("%s xmit: Routing loop! Remote address found on this node!\n",
					    p->name);
    ...
}

The kernel-doc for DEV_READY in include/net/dropreason-core.h describes a
device whose data structures are not up/ready/initialized. In the same
transmit path, ip6_tnl_xmit() uses SKB_DROP_REASON_RECURSION_LIMIT for its
own loop case:

net/ipv6/ip6_tunnel.c:ip6_tnl_xmit() {
    ...
		net_warn_ratelimited("%s: Local routing loop detected!\n",
				     t->parms.name);
		reason = SKB_DROP_REASON_RECURSION_LIMIT;
    ...
}

So a loop through the local node could show up as either reason,
depending on which check catches it.

This happens when the device's non-multicast remote address is configured
on the local node and IP6_TNL_F_ALLOW_LOCAL_REMOTE is not set. The commit
message does acknowledge this choice.

ip6_tnl_xmit_ctl() only returns a boolean, so callers can't tell its
refusal causes apart. Could ip6_tnl_xmit_ctl() return a drop reason
instead, so the loop case is reported like the one in ip6_tnl_xmit()?

[Severity: Medium]
This early check only covers packets without valid IPv6 metadata. What
happens to an inner IPv6 packet from :: that does carry valid IPv6 TX
metadata on a collect_md ip6gre or ip6gretap device?

On such a device, raddr is :: because ip6gre_tap_validate() accepts an
omitted IFLA_GRE_REMOTE. ip6gre_xmit_ipv6() still compares the inner
source address with raddr, even in collect_md mode:

net/ipv6/ip6_gre.c:ip6gre_xmit_ipv6() {
    ...
	if (ipv6_addr_equal(&t->parms.raddr, &ipv6h->saddr))
		return SKB_DROP_REASON_RECURSION_LIMIT;
    ...
}

As a result, every inner IPv6 packet sourced from :: is dropped, even if
key->u.ipv6.dst holds a real endpoint. One example is the DAD Neighbour
Solicitation of a VM behind an OVS collect_md ip6gretap port.

With this patch, that drop goes to kfree_skb_reason() as
SKB_DROP_REASON_RECURSION_LIMIT. dropreason-core.h documents that reason
as a "Dead loop on virtual device", but no loop exists here. Drop
monitoring tools would then report a routing loop for ordinary DAD or
MLD traffic.

The commit message notes this case:

    A packet from :: that carries IPv6 metadata still meets the check in
    ip6gre_xmit_ipv6().

ip6erspan_tunnel_xmit() in the same file only runs the raddr/saddr test
in native mode. ip6_tnl_start_xmit() in net/ipv6/ip6_tunnel.c applies
ip6_tnl_addr_conflict() to collect_md ip6tnl devices as well, and it now
reports the same RECURSION_LIMIT reason.

The drop itself predates this series. The check came from c12b395a4664,
and 6712abc168eb left it in place for collect_md. The RECURSION_LIMIT
label is new, though, and the last patch in the series only touches
vxlan, so it is still there at the end of the series.

Would it make sense to guard the test in ip6gre_xmit_ipv6() (and in
ip6_tnl_start_xmit()) with !t->parms.collect_md, as
ip6erspan_tunnel_xmit() does?

[ ... ]

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