Re: [PATCH net-next v2 1/3] ipv4: Don't raise a cached pmtu from another path's Packet Too Big

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

Critical: 0 · High: 0 · Medium: 2 · Low: 4

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] The fix does not do "the same" as the early return in
  __ip_rt_update_pmtu(), as the commit message claims.
- [Medium] The new early exit in update_or_create_fnhe() ignores the
  `lock` argument, so it can drop an MTU-lock transition.
- [Low] Missing Fixes: tag.
- [Low] The commit message presents the raise as the normal result of a
  wider path's Fragmentation Needed ("update_or_create_fnhe() overwrites…
- [Low] No selftest exercises the new no-raise check in
  update_or_create_fnhe().
- [Low] The new comment in update_or_create_fnhe() (route.c:679-682)
  explains the check as stopping "a Packet Too Big from one path" from…

--

ipv4: Don't raise a cached pmtu from another path's Packet Too Big

This commit adds a check to update_or_create_fnhe(). If an existing
exception is unexpired, has the same genid and has a lower cached pmtu
than the new value, the check leaves it alone. The goal is to stop the
multipath loop in __ip_rt_update_pmtu(), added by 7d3f3b4367f3, from
raising a narrower path's learned pmtu with a value from a wider path.

Since commit 7d3f3b4367f3 ("net: ipv4: Cache pmtu for all packet paths
if multipath enabled") __ip_rt_update_pmtu() updates the exception of
every path of a multipath route, but update_or_create_fnhe() overwrites
whatever pmtu a path already has. A Packet Too Big from a wider path
thus raises the pmtu a narrower path has learned, and flows on that path
blackhole until it gets its own Packet Too Big again.

[Severity: Low]
The message names 7d3f3b4367f3 as the origin of this behaviour. Should
there be a Fixes tag for it?

Fixes: 7d3f3b4367f3 ("net: ipv4: Cache pmtu for all packet paths if multipath enabled")

That commit is in released kernels. Without the tag, stable and backport
tooling may not connect this change to the trees that carry the multipath
loop in __ip_rt_update_pmtu().

The early return in __ip_rt_update_pmtu() already keeps a pmtu from going
up on the path the Packet Too Big was looked up on. Do the same for the
other paths: leave an unexpired exception with a lower pmtu alone.

[Severity: Low]
Can the sequence in the first paragraph happen in normal operation?

When the narrower path's ICMP (say 1300) is processed, the multipath loop
in __ip_rt_update_pmtu() writes 1300, with the same expiry, into the
exception of every path:

    for (nhsel = 0; nhsel < fib_info_num_path(res.fi); nhsel++) {
        nhc = fib_info_nhc(res.fi, nhsel);
        ...
        update_or_create_fnhe(nhc, fl4->daddr, 0, mtu, lock,
                              jiffies + net->ipv4.ip_rt_mtu_expires);
    }

fill_route_from_fnhe() then updates each bound dst. The wider path's dst
also reports ipv4_mtu() == 1300, so a later, larger ICMP for it is dropped
by the existing early return:

    old_mtu = ipv4_mtu(dst);
    if (old_mtu < mtu)
        return;

So the raise only seems possible once the paths' exceptions have
diverged. That could happen through:

- fnhe_remove_oldest() evicting the exception on one nexthop
- a GFP_ATOMIC allocation failure
- a per-device MTU change via fib_nhc_update_mtu() and fnhe_update_pmtu()
- a nexthop shared between routes in different tables

Could the commit message say which of these scenarios is being fixed?

The message also says the change is for "the other paths", but the check
is in the shared helper. It applies to the single-path call as well:

    nhc = FIB_RES_NHC(res);
    update_or_create_fnhe(nhc, fl4->daddr, 0, mtu, lock,
                          jiffies + net->ipv4.ip_rt_mtu_expires);

It also applies to builds without CONFIG_IP_ROUTE_MULTIPATH. So
non-multipath behaviour changes whenever the dst passed in isn't bound to
the exception, for example when ip_rt_update_pmtu() clears flowi4_oif for
bridged encapsulations. Should the message mention this?

[Severity: Low]
This isn't a bug, but the comment describes the check as one path versus
another on a multipath route. update_or_create_fnhe() has no idea which
path an update came from. The check applies to every caller that passes
a non-zero pmtu, including the single-path call in __ip_rt_update_pmtu()
and the nexthop the ICMP was actually looked up on. Could the comment
describe the real scope?

Also, "Packet Too Big" is the ICMPv6 message name and doesn't appear
anywhere else in net/ipv4/. On IPv4, updates reach this code from ICMP
Fragmentation Needed and from non-ICMP callers such as tunnel PMTU
handling. The subject and commit message use the same term.

[Severity: Medium]
Does this really do the same as the early return in
__ip_rt_update_pmtu(), as the commit message says?

The early return compares the new mtu with ipv4_mtu() of the looked-up
dst, which includes the device MTU. This check only fires when a sibling
nexthop already has an exception with a non-zero, same-genid, unexpired
fnhe_pmtu.

Some siblings still get the other path's value, with no comparison
against their own device MTU:

- a sibling with no exception (the else branch below)
- a redirect-only exception (fnhe_pmtu == 0)
- an expired exception

fill_route_from_fnhe() copies that value into rt_pmtu, and
ip_dst_mtu_maybe_forward() uses it before falling back to dev->mtu:

    mtu = rt->rt_pmtu;
    if (mtu && time_before(jiffies, READ_ONCE(rt->dst.expires)))
        goto out;

For example, take an ECMP route with path A on a 9000 MTU device, path B
on a 1400 MTU device, and no exception on B. A Fragmentation Needed of
1500 for a flow on A passes A's early return (9000 >= 1500), and the loop
creates an exception on B with pmtu 1500.

Local flows hashed to B then size packets for 1500. That comes from TCP
MSS via dst_mtu() and from the skb->len > mtu check in
__ip_finish_output(). These packets are larger than B's device MTU, so
they are dropped, and no ICMP comes back to correct it. This lasts until
ip_rt_mtu_expires and happens again each time A learns a new PMTU.

The sibling-path raise itself came in with 7d3f3b4367f3. Since this patch
sets out to stop it, can one path's PMTU update still raise another
path's effective MTU and blackhole it this way?

[Severity: Medium]
Can this early exit drop an MTU lock transition?

The check compares only the pmtu values, and it skips the
fnhe_mtu_locked = lock assignment below. In __ip_rt_update_pmtu(), an mtu
below ip_rt_min_pmtu is clamped up using the looked-up dst's MTU, and
that value is sent to every nexthop with lock set:

    if (mtu < net->ipv4.ip_rt_min_pmtu) {
        lock = true;
        mtu = min(old_mtu, net->ipv4.ip_rt_min_pmtu);
    }

The looked-up dst can belong to a different path than the affected flow.
__build_flow_key() passes 0, 0 for the ports to flowi4_init_output(), so
with fib_multipath_hash_policy=1 the lookup can pick path X while the
flow uses path B.

B can have an unexpired, unlocked exception with fnhe_pmtu below
ip_rt_min_pmtu. fnhe_update_pmtu() leaves it that way after B's device
MTU drops:

    if (fnhe->fnhe_mtu_locked) {
        if (new <= fnhe->fnhe_pmtu) {
            fnhe->fnhe_pmtu = new;
            fnhe->fnhe_mtu_locked = false;
        }
    } else if (new < fnhe->fnhe_pmtu || orig == fnhe->fnhe_pmtu) {
        fnhe->fnhe_pmtu = new;
    }

Raising the min_pmtu sysctl after a pmtu was learned has the same effect.

Say min_pmtu=1000, B is unlocked at 600, and X's effective MTU is 1500. A
Fragmentation Needed of 300 for B's flow gives mtu = min(1500, 1000) =
1000 with lock = true. For B, 1000 > 600 and the exception is unexpired,
so the code jumps to out_unlock.

B stays at 600 with fnhe_mtu_locked false. DF stays set while the next
hop only passes 300, so B's flows blackhole. X is now locked, so later
ICMPs that resolve to X hit this:

    if (ip_mtu_locked(dst))
        return;

B isn't corrected until its exception expires. Before this patch, B was
overwritten with 1000 and locked, so DF was cleared and the traffic was
fragmented and delivered.

Should the check only skip when !lock || fnhe->fnhe_mtu_locked? Or could
it set the lock without raising fnhe_pmtu?

[Severity: Low]
Does any selftest reach this check?

pmtu_ipv4_mp_exceptions in tools/testing/selftests/net/pmtu.sh uses 1500
on both legs, so pmtu > fnhe->fnhe_pmtu is never true.

pmtu_ipv4_mp_asym_exceptions comes later in the series ("selftests: net:
pmtu: Add IPv6 and asymmetric multipath tests"). It sends an 1800 byte
ping via R1, which creates both exceptions at 1400 through the else
branch. It then sends a 1350 byte ping via R2, which lowers both to 1300.
Neither step reaches the check.

Both IPv4 tests give the same result with or without this patch. They
would still pass if the check were removed or inverted. The selftest
commit message only says that the IPv6 tests fail without the preceding
fix.

Could a test case be added where a narrower path's unexpired exception
is offered a larger value from another path and has to keep its lower
pmtu?

[ ... ]

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