Re: [PATCH net-next v2 2/3] ipv6: Cache pmtu for all paths of a multipath route

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

Critical: 0 · High: 2 · Medium: 1 · Low: 1 (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).

- [High] The in-place update branch of rt6_nh_update_pmtu() can raise
  another path's effective PMTU above that path's link MTU.
- [High] The fix does not close the blackhole it describes.
- [Medium] A passed-in RTF_CACHE dst is no longer updated when the daddr
  used by the walk is not that dst's exception key.
- [Low] No Fixes: tag, even though the commit message presents the change
  as a fix for a user-visible blackhole.

Pre-existing issues:
- [Low] Reported concern: rt6_nh_update_pmtu() reads and then writes an
  exception's RTAX_MTU under only rcu_read_lock().

--

ipv6: Cache pmtu for all paths of a multipath route

This patch changes __ip6_rt_update_pmtu() so that a Packet Too Big pmtu is
cached on every path of a multipath route. It walks every fib6_nh of a
nexthop object, or the fib6_siblings of a legacy multipath route. A path
that already has an exception for the destination is updated in place by
the new rt6_nh_update_pmtu() helper; a path without one gets a new
exception.

Link: https://lore.kernel.org/netdev/ad29493c-9e48-4a94-b327-e47af3a9d137@kernel.org/ (local)

The flow then keeps sending too big packets on its path, every Packet
Too Big lands on the same wrong path, and the flow blackholes.

[Severity: Low]
This isn't a bug, but the change is described as fixing a user-visible
blackhole. Should it carry a Fixes: tag?

The IPv4 counterpart, commit 7d3f3b4367f3 ("net: ipv4: Cache pmtu for all
packet paths if multipath enabled"), went through net-next without one.
So treating this as a behaviour extension rather than a stable fix may be
intentional.

[ ... ]

[Severity: High]
Can this raise a path's effective pmtu above that path's own link MTU?

A redirect exception normally has a raw RTAX_MTU of 0, because
rt6_do_redirect()->ip6_rt_cache_alloc() inherits the route's metrics.
Raw 0 does not mean unlimited, though. ip6_dst_mtu_maybe_forward() falls
back to the device mtu6:

include/net/ip6_route.h:ip6_dst_mtu_maybe_forward() {
    ...
		mtu = dst_metric_raw(dst, RTAX_MTU);
		if (mtu)
			goto out;
    ...
		mtu = READ_ONCE(idev->cnf.mtu6);
    ...
}

The only upper bound on arg->mtu is this check in __ip6_rt_update_pmtu():

	if (mtu >= dst6_mtu(dst))
		return;

That check is made against the dst that was looked up, not against the
path being written here.

For example, path A is on a 9000 MTU device and path B is on a 1500 MTU
device, and B has a redirect exception for daddr. A Packet Too Big of 8000
that resolves to A passes the check, and the walk then sets RTAX_MTU to
8000 on B's exception. ip6_mtu() for B now returns 8000, so frames larger
than B's device MTU are dropped until ip6_rt_mtu_expires.

The create branch is protected by rt6_insert_exception():

	if (dst_metric_raw(&nrt->dst, RTAX_MTU) >= fib6_mtu(res)) {
		err = -EINVAL;
		goto out;
	}

The in-place branch has no equivalent check. This also seems to
contradict "its pmtu never goes up" in the commit message and in the
comment above.

Does this branch also need a bound such as arg->mtu < fib6_mtu(res)?

[Severity: Low]
This is a pre-existing issue, but the read of RTAX_MTU and the
rt6_do_update_pmtu() call here run under rcu_read_lock() only.

Two concurrent Packet Too Big messages (say 1300 and 1400) could leave
1400 behind. The update could also race with rt6_exceptions_update_pmtu(),
which holds rt6_exception_lock.

The old __ip6_rt_update_pmtu() already did the same unlocked
compare-then-set on RTF_CACHE dsts. A value left too high would also be
lowered by the next Packet Too Big, so the impact looks limited to one
extra round trip.

[ ... ]

[Severity: High]
Does this early return still allow the blackhole the commit message
describes?

Here dst is whatever ip6_update_pmtu() found with a lookup that has no
protocol or ports. It can be path B while the flow is on path A. B may
have a lower MTU than the Packet Too Big value. That happens with a
smaller device MTU, a route mtu metric, or lwtunnel headroom. It also
happens when B has an exception that A lacks, for example because:

- A's GFP_ATOMIC insertion failed
- rt6_exception_remove_oldest() evicted A's entry
- a later redirect replaced A's exception
- A was added to the group afterwards

In any of these cases the function returns before the walk, and A never
gets an exception.

The flow-less hash depends only on saddr, daddr, flowlabel and proto 0.
Every later Packet Too Big for that flow therefore lands on B as well.

For example, A is Ethernet at 1500 and B is PPPoE at 1492. A UDP or tunnel
flow on A gets a Packet Too Big of 1496. The lookup returns B, 1496 >= 1492
is true, and nothing is cached on A.

This check predates the patch and mirrors the IPv4 old_mtu < mtu check.
However, the commit message says this patch fixes the case where "every
Packet Too Big lands on the same wrong path, and the flow blackholes".

Could the check be moved into rt6_nh_update_pmtu(), bounded against each
path's MTU, so the update is skipped only when no path would be lowered?

 
-	if (!rt6_cache_allowed_for_pmtu(rt6)) {
-		rt6_do_update_pmtu(rt6, mtu);
-		/* update rt6_ex->stamp for cache */
-		if (rt6->rt6i_flags & RTF_CACHE)
-			rt6_update_exception_stamp_rt(rt6);
-	} else if (daddr) {

[ ... ]

[Severity: Medium]
What happens to a passed-in RTF_CACHE dst whose exception key is not
daddr?

Before this patch, any RTF_CACHE dst was lowered directly. Now, when
daddr, rt6->from and fib6_node are all set, the dst itself is updated only
if rt6_find_cached_rt(res, daddr, saddr) happens to return it.

Take a TCP socket with a routing header (IPV6_RTHDR or SRH).
inet6_csk_route_socket() looks up the first hop after fl6_update_dst(), so
dst can be a first-hop RTF_CACHE exception, for example one created by a
redirect. The call chain is then:

tcp_v6_mtu_reduced()
  inet6_csk_update_pmtu()
    dst->ops->update_pmtu(dst, sk, NULL, mtu, true)
      __ip6_rt_update_pmtu()    /* daddr = &sk->sk_v6_daddr */

The walk looks up the final destination and finds nothing. It then
inserts an unrelated exception, keyed by that destination, on the
first-hop route, and leaves the first-hop exception's MTU unchanged.

The second inet6_csk_route_socket() call in inet6_csk_update_pmtu()
returns the same exception with the old MTU. The icsk_pmtu_cookie >
dst_mtu(dst) test is then false, so the MSS is never reduced.

The same gap applies to the FLOWI_FLAG_KNOWN_NH clones from
ip6_pol_route(). rt6_uncached_list_add() puts them on the uncached list,
and they are never in an exception bucket.

The commit message says "that also covers a dst that is itself a cached
exception, which used to be updated alone". Is that only true when daddr
matches the exception's key?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110095903.84813-1-deliran%40verdict.gg