Re: [PATCH net-next v2 1/3] ipv4: draw rt_genid from a sequence shared by all netns

[netdev-bot+sashiko@kernel.org]

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] The commit says "a cached route never matches the rt_genid of
  the netns its device moves to".
- [Low] The comment added above rt_genid_seq in net/ipv4/route.c, and the
  commit message, say each netns "bumps on NETDEV_UP before any of its…

--

ipv4: draw rt_genid from a sequence shared by all netns

This commit stops incrementing rt_genid separately in each netns. Instead,
every bump takes its value from one global sequence that never returns 0.
The goal is that a route cached through a device can never match the
rt_genid of the netns the device moves into.

> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 595a3b7f17830..25bebbb30bf3c 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -393,6 +393,26 @@ static inline int ip_rt_proc_init(void)
>  }
>  #endif /* CONFIG_PROC_FS */
>  
> +/*
> + * Draw rt_genid bumps from one sequence shared by all netns. A route keeps
> + * the genid of the netns it was created in, and is checked against the netns
> + * its device is in now, so distinct values make that check fail once the
> + * device has moved. Every netns starts at 0, which the sequence never hands
> + * out, and bumps on NETDEV_UP before any of its devices can carry traffic.
> + */

[Severity: Low]
Is the last sentence of this comment accurate? The commit message makes
the same claim:

  "A netns bumps its rt_genid on NETDEV_UP before its devices can carry
  traffic"

In net/core/dev.c, __dev_open() marks the device up and activates it
before any notifier runs:

    netif_set_up(dev, true);
    dev_set_rx_mode(dev);
    dev_activate(dev);

netif_open() only sends NETDEV_UP after __dev_open() has returned:

    rtmsg_ifinfo(RTM_NEWLINK, dev, IFF_UP | IFF_RUNNING, GFP_KERNEL, 0, NULL);
    call_netdevice_notifiers(NETDEV_UP, dev);

So rt_cache_flush() in fib_netdev_event() runs after the device can
already transmit.

The NETDEV_UP bump also doesn't always happen. For a device with no
in_device, for example one whose MTU is too small for IPv4,
fib_netdev_event() returns early:

    in_dev = __in_dev_get_rtnl(dev);
    if (!in_dev)
        return NOTIFY_DONE;

The nonzero genid on cached routes seems to come from the FIB insertion
paths instead, such as fib_table_insert()->rt_cache_flush() and the
NETDEV_UP case of fib_inetaddr_event().

The next patch in the series, "net: remove rcu_barrier() from
__dev_change_net_namespace()", depends on this invariant. Could the
comment and the commit message describe the ordering that actually holds?

> +static atomic_t rt_genid_seq;
> +
> +void rt_genid_bump_ipv4(struct net *net)
> +{
> +	int genid;
> +
> +	do {
> +		genid = atomic_inc_return(&rt_genid_seq);
> +	} while (!genid);
> +	atomic_set(&net->ipv4.rt_genid, genid);
> +}

[Severity: Medium]
The commit message says:

  "so a cached route never matches the rt_genid of the netns its device
  moves to."

Doesn't that only hold modulo 2^32? rt_genid_seq is one 32-bit atomic_t
shared by every netns, and rtable::rt_genid is an int. The loop only
skips 0, so values start repeating after 2^32 - 1 bumps.

Anyone who can create a user namespace and a netns can advance the
shared counter by writing /proc/sys/net/ipv4/route/flush. Each write
makes ipv4_sysctl_rtcache_flush() call rt_cache_flush(), and
sysctl_route_net_init() keeps the flush entry for netns owned by non-init
user namespaces:

    /* Don't export non-whitelisted sysctls to unprivileged users */
    if (net->user_ns != &init_user_ns) {
        if (tbl[0].procname != ipv4_route_flush_procname)
            table_size = 0;
    }

Suppose a route is cached in netns A with genid X, and its device then
moves to netns B. If B's genid equals X before
free_fib_info_rcu()->nexthop_free_rcu() gets to dst_dev_put(), the check
in rt_is_expired() passes:

    res = rth->rt_genid != rt_genid_ipv4(dev_net_rcu(rth->dst.dev));

ipv4_dst_check() then accepts the dst, and the socket in A keeps sending
through a device that B now owns.

This is hard to hit, because B's bump has to land on the unknown X
within a few RCU grace periods. However, the follow-up patch "net: remove
rcu_barrier() from __dev_change_net_namespace()" removes the barrier. It
then relies on genid uniqueness alone to reject every FIB-cached route
through the moved device, not just routes that use nexthop objects.

Would an approach that doesn't depend on a wrapping 32-bit value work
here? For example, the move could invalidate the cached dsts, or call
dst_dev_put() on them, synchronously. Another option would be to check
the netns the route was created in.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019-bug-net-netns-move-rcu-barrier-v2-0-b6eb7780c492%40gmail.com