Re: [PATCH net-next 2/3] ipv6: take fib6_sernum from a sequence shared by all netns

[netdev-bot+sashiko]

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

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

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 patch makes every netns take fib6_sernum (and fn_sernum)
  from one static counter, fib6_sernum_seq.

--

ipv6: take fib6_sernum from a sequence shared by all netns

This patch replaces the per-netns fib6_sernum counter with one static
sequence, fib6_sernum_seq, that all netns draw from, including for the
initial value. The goal is that a nexthop pcpu route cached against one
netns can no longer match the fib6_sernum of another netns after its
device moves.

> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index 0dc6ebee6e568..c66466be64451 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -89,13 +89,21 @@ static void fib6_walker_unlink(struct net *net, struct fib6_walker *w)
>  	write_unlock_bh(&net->ipv6.fib6_walker_lock);
>  }
>
> +/*
> + * Draw every fib6_sernum from one sequence shared by all netns, so that a
> + * route cached against one netns never matches the sernum of another one,
> + * e.g. after its device moved.
> + */
> +static atomic_t fib6_sernum_seq;
> +
>  static int fib6_new_sernum(struct net *net)
>  {
> -	int new, old = atomic_read(&net->ipv6.fib6_sernum);
> +	int new, old = atomic_read(&fib6_sernum_seq);
>
>  	do {
>  		new = old < INT_MAX ? old + 1 : 1;
> -	} while (!atomic_try_cmpxchg(&net->ipv6.fib6_sernum, &old, new));
> +	} while (!atomic_try_cmpxchg(&fib6_sernum_seq, &old, new));
> +	atomic_set(&net->ipv6.fib6_sernum, new);
>
>  	return new;
>  }

[Severity: Medium]
The comment says a cached route "never matches the sernum of another one".
Does the code guarantee that once fib6_sernum_seq wraps?

The shared counter still wraps to 1 at INT_MAX:

    new = old < INT_MAX ? old + 1 : 1;

Nothing checks whether the reissued value is still held by a live netns, a
fib6_node, or a cached dst.

Validation has not changed. ip6_rt_pcpu_alloc() records only the value:

    if (f6i->nh)
        pcpu_rt->sernum = rt_genid_ipv6(dev_net(dev));

rt6_is_valid() then compares it for plain equality against the device's
current netns:

    return rt6->sernum == rt_genid_ipv6(dev_net(rt6->dst.dev));

and ip6_dst_check() returns the dst on a match:

    if (rt->sernum)
        return rt6_is_valid(rt) ? dst : NULL;

After a wrap, couldn't netns B be given the same value S that a socket in
netns A cached? If A's device then moves into B while B holds S, the
original problem comes back in the window before nexthop_free_rcu() runs.

This patch also links netns that used to be independent. Each successful
fib6_add(), fib6_flush_trees(), the link up/down sync paths, and
rt6_insert_exception() (reachable from a received ICMPv6 Packet Too Big)
now advance the shared counter. Any netns can therefore push it through a
full cycle, including one owned by an unprivileged user namespace.

Before this patch, netns N could only reissue an old value after N itself
made about 2^31 draws. Now, once the global counter has wrapped, can a
route add in N draw exactly N's current fib6_sernum, or a value still held
by a stale cookie? In that case the cached dsts that fib6_add() should
invalidate would still pass ip6_dst_check() and keep using the old route.

A hit needs about 2^31 draws plus an exact match, so it is far rarer than
the per-netns collisions this patch removes. The commit message makes the
same absolute claim, though:

    so that a cached route never matches the fib6_sernum of
    another netns.

The later patch in this series, "net: remove rcu_barrier() from
__dev_change_net_namespace()", relies on this as well. It cites that
"rt_genid and fib6_sernum are now unique across netns" as its reason for
dropping the barrier. The end of the series doesn't seem to add a wrap
guard, a wider generation, or a check on the originating netns.

Could the comment and the commit message be reworded so they don't claim
uniqueness? Or, if the rcu_barrier() removal depends on uniqueness, could
the series handle the wrap explicitly?

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