* Re: [PATCH net] ipv6: take tb6_lock across the fib6 dump sernum snapshot
2026-10-12 8:38 [PATCH net] ipv6: take tb6_lock across the fib6 dump sernum snapshot Jun Yang
@ 2026-10-13 8:41 ` netdev-bot+sashiko
2026-10-13 12:41 ` Ido Schimmel
1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sashiko @ 2026-10-13 8:41 UTC (permalink / raw)
To: juny24602
Cc: dsahern, idosch, davem, edumazet, kuba, pabeni, horms, kaber,
netdev, junvyyang, stable, corvus
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 1 (2) · 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).
- [High] The patch makes the fn_sernum snapshot and compare in
fib6_dump_table() atomic under tb6_lock.
- [Medium] The patch does not take a reference on the route parked in
w->leaf while the dump is suspended.
- [Low] The commit message says the use-after-free happens at both ends
of the sernum check: "…or passes the compare and only then takes the…
Pre-existing issues:
- [Medium] This was already broken before the patch.
- [Medium] This was already broken before the patch.
--
ipv6: take tb6_lock across the fib6 dump sernum snapshot
This patch takes tb6_lock once at the top of fib6_dump_table(). The
fn_sernum snapshot after the first walk, the compare on continuation and
the walker call now all run in one critical section. The aim is to stop a
concurrent NLM_F_REPLACE from freeing the route parked in w->leaf without
the dump noticing.
> That check is done outside tb6_lock at both ends: the snapshot after the
> first walk is taken after spin_unlock_bh(), and on continuation the
> compare runs before spin_lock_bh(). A writer that slips in between, e.g.
> RTM_NEWROUTE with NLM_F_REPLACE on the route the walker is parked on,
> frees the route via fib6_info_release() (the replace path has no
> FOR_WALKERS fixup, unlike fib6_del_route()) and bumps fn_sernum, yet the
> dump records the new serial next to the stale w->leaf, or passes the
> compare and only then takes the lock. Either way the restart is skipped
> and fib6_walk_continue() hands the freed route to rt6_fill_node():
[Severity: Low]
Is the continuation half of this description accurate?
The first-walk half matches the old code. cb->args[5] was read after
spin_unlock_bh(), so a replace in that window stores the new serial next
to the stale w->leaf.
On the continuation side, inet6_dump_fib() holds rcu_read_lock() for the
whole fib6_dump_table() call, and fib6_info_release() frees through RCU.
A route replaced between a passing compare and spin_lock_bh() cannot be
freed before that call returns.
The else branch also writes cb->args[5] only when it restarts:
int sernum = READ_ONCE(w->root->fn_sernum);
if (cb->args[5] != sernum) {
/* Begin at the root if the tree changed */
cb->args[5] = sernum;
...
} else
w->skip = 0;
If the walk parks again, args[5] still holds the old serial. The next
recvmsg() then sees the mismatch and restarts.
So that window can show a stale route once, but it does not seem able to
hand a freed route to rt6_fill_node(). Could the changelog say that the
use-after-free comes from the post-unlock snapshot in the
cb->args[4] == 0 branch, and that the compare moved only for consistency?
Stable backporters will probably rely on this when they look at older
trees.
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index 9ff761962b45c..70482381165fe 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -592,14 +592,13 @@ static int fib6_dump_table(struct fib6_table *table, struct sk_buff *skb,
> w = (void *)cb->args[2];
> w->root = &table->tb6_root;
>
> + spin_lock_bh(&table->tb6_lock);
> if (cb->args[4] == 0) {
> w->count = 0;
> w->skip = 0;
> w->skip_in_node = 0;
>
> - spin_lock_bh(&table->tb6_lock);
> res = fib6_walk(net, w);
> - spin_unlock_bh(&table->tb6_lock);
> if (res > 0) {
> cb->args[4] = 1;
> cb->args[5] = READ_ONCE(w->root->fn_sernum);
[Severity: High]
The snapshot is now taken under tb6_lock, but the unreferenced w->leaf is
still guarded only by an equality compare on the root fn_sernum. Can the
root fn_sernum go back to the value saved here?
In net/ipv6/route.c, ip6_link_failure() writes a fixed constant while
holding only rcu_read_lock(), not tb6_lock:
fn = rcu_dereference(from->fib6_node);
if (fn && (rt->rt6i_flags & RTF_DEFAULT))
WRITE_ONCE(fn->fn_sernum, -1);
A ::/0 route lives in table->tb6_root, and that is w->root here. That
allows this sequence:
ip6_link_failure() /* root fn_sernum = -1 */
dump parks /* args[5] = -1, w->leaf = R */
RTM_NEWROUTE|NLM_F_REPLACE on R
fib6_add()
fib6_purge_rt()
fib6_info_release() /* frees R via RCU, no fixup */
__fib6_update_sernum_upto_root() /* root fn_sernum = S */
ip6_link_failure() /* root fn_sernum = -1 again */
recvmsg()
fib6_dump_table() /* -1 == -1, no restart */
fib6_walk_continue()
fib6_dump_node()
rt6_dump_route()
rt6_fill_node() /* reads freed R */
That looks like the same KASAN use-after-free shown in the changelog. The
fixup in fib6_del_route() does not help here, because it only matches a
route that is still live:
if (w->state == FWS_C && w->leaf == rt) {
The changelog calls the sernum check "the only thing standing between the
walker and a freed route". Would it be more robust to pin w->leaf while
the dump is suspended, or to add a walker fixup to the replace/purge path
in fib6_add(), instead of relying on fn_sernum equality?
[Severity: Medium]
A related question about the same compare: can wraparound of the serial
also defeat it?
fib6_new_sernum() wraps on purpose:
new = old < INT_MAX ? old + 1 : 1;
and __fib6_update_sernum_upto_root() writes the shared per-netns value
into every node up to the root:
WRITE_ONCE(fn->fn_sernum, sernum);
Suppose a dump parks with args[5] = S and w->leaf = R. R is then replaced
with NLM_F_REPLACE, which frees it through RCU without a walker fixup.
Roughly 2^31 more operations that consume a serial in the same netns then
bring the root back to S. On the next recvmsg() the compare passes under
tb6_lock and w->skip is set to 0. fib6_walk_continue() then passes the
freed R to rt6_dump_route() and rt6_fill_node().
CAP_NET_ADMIN in a netns owned by a user namespace is enough to drive the
counter, but the number of operations makes this expensive. Pinning
w->leaf across the suspension, or a walker fixup in the replace path,
would cover this case too.
[ ... ]
> @@ -616,14 +615,13 @@ static int fib6_dump_table(struct fib6_table *table, struct sk_buff *skb,
> } else
> w->skip = 0;
>
[Severity: Medium]
This isn't a bug introduced by this patch, but the restart branch just
above this resyncs by position:
if (cb->args[5] != sernum) {
/* Begin at the root if the tree changed */
cb->args[5] = sernum;
w->state = FWS_INIT;
w->node = w->root;
w->skip = w->count;
w->skip_in_node = 0;
}
Can this skip or repeat routes when the tree changes between recvmsg()
calls?
Say an RTINFO node before the cursor is emptied, which a plain fib6_del()
does without bumping the root fn_sernum. A route is then added after the
cursor, and __fib6_update_sernum_upto_root() does bump it. The restart
then skips one node too many, and the unsent routes of the partly dumped
node are lost. An add before the cursor sends some nodes twice.
Nothing in the IPv6 FIB dump path sets cb->seq, calls
nl_dump_check_consistent() or sets NLM_F_DUMP_INTR. inet6_dump_addr() in
net/ipv6/addrconf.c does. As a result, user space can get incomplete or
duplicated RTM_GETROUTE output with no consistency signal.
[Severity: Medium]
This is also a pre-existing issue and was not introduced by this patch.
Is w->skip_in_node still valid when this non-restart branch is taken?
When a dump is suspended partway through a route's exceptions,
fib6_dump_node() records:
w->skip_in_node += res;
If that route is then deleted, the walker fixup in fib6_del_route() moves
w->leaf to rt->fib6_next, or sets FWS_U, but does not touch
w->skip_in_node. A plain fib6_del() does not change the root fn_sernum,
so this branch is taken.
rt6_dump_route() then applies the old skip count to a different route.
That route and some of its exceptions are left out of the dump, and
NLM_F_DUMP_INTR is not set.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012083843.60510-1-juny24602%40gmail.com
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net] ipv6: take tb6_lock across the fib6 dump sernum snapshot
2026-10-12 8:38 [PATCH net] ipv6: take tb6_lock across the fib6 dump sernum snapshot Jun Yang
2026-10-13 8:41 ` netdev-bot+sashiko
@ 2026-10-13 12:41 ` Ido Schimmel
1 sibling, 0 replies; 3+ messages in thread
From: Ido Schimmel @ 2026-10-13 12:41 UTC (permalink / raw)
To: Jun Yang
Cc: dsahern, davem, edumazet, kuba, pabeni, horms, kaber, netdev,
Jun Yang, stable, TencentOS Corvus AI
On Mon, Oct 12, 2026 at 04:38:20PM +0800, Jun Yang wrote:
[...]
> the replace path has no FOR_WALKERS fixup, unlike fib6_del_route()
Why not fix that instead with something like [1]?
[1]
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 9ff761962b45..b9268b5569a8 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -89,6 +89,24 @@ static void fib6_walker_unlink(struct net *net, struct fib6_walker *w)
write_unlock_bh(&net->ipv6.fib6_walker_lock);
}
+static void fib6_adjust_walkers(struct net *net, struct fib6_table *table,
+ struct fib6_info *rt)
+{
+ struct fib6_walker *w;
+
+ read_lock(&net->ipv6.fib6_walker_lock);
+ FOR_WALKERS(net, w) {
+ if (w->state == FWS_C && w->leaf == rt) {
+ pr_debug("walker %p adjusted\n", w);
+ w->leaf = rcu_dereference_protected(rt->fib6_next,
+ lockdep_is_held(&table->tb6_lock));
+ if (!w->leaf)
+ w->state = FWS_U;
+ }
+ }
+ read_unlock(&net->ipv6.fib6_walker_lock);
+}
+
static int fib6_new_sernum(struct net *net)
{
int new, old = atomic_read(&net->ipv6.fib6_sernum);
@@ -1324,6 +1342,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
}
nsiblings = iter->fib6_nsiblings;
iter->fib6_node = NULL;
+ fib6_adjust_walkers(info->nl_net, rt->fib6_table, iter);
list_add(&iter->purge_link, purge_list);
if (rcu_access_pointer(fn->rr_ptr) == iter)
fn->rr_ptr = NULL;
@@ -1339,6 +1358,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
if (rt6_qualify_for_ecmp(iter)) {
*ins = iter->fib6_next;
iter->fib6_node = NULL;
+ fib6_adjust_walkers(info->nl_net, rt->fib6_table, iter);
list_add(&iter->purge_link, purge_list);
if (rcu_access_pointer(fn->rr_ptr) == iter)
fn->rr_ptr = NULL;
@@ -1971,7 +1991,6 @@ static void fib6_del_route(struct fib6_table *table, struct fib6_node *fn,
enum rt_del_reason del_reason)
{
struct fib6_info *leaf, *replace_rt = NULL;
- struct fib6_walker *w;
struct fib6_info *rt = rcu_dereference_protected(*rtp,
lockdep_is_held(&table->tb6_lock));
struct net *net = info->nl_net;
@@ -2022,18 +2041,7 @@ static void fib6_del_route(struct fib6_table *table, struct fib6_node *fn,
rt6_multipath_rebalance(next_sibling);
}
- /* Adjust walkers */
- read_lock(&net->ipv6.fib6_walker_lock);
- FOR_WALKERS(net, w) {
- if (w->state == FWS_C && w->leaf == rt) {
- pr_debug("walker %p adjusted by delroute\n", w);
- w->leaf = rcu_dereference_protected(rt->fib6_next,
- lockdep_is_held(&table->tb6_lock));
- if (!w->leaf)
- w->state = FWS_U;
- }
- }
- read_unlock(&net->ipv6.fib6_walker_lock);
+ fib6_adjust_walkers(net, table, rt);
/* If it was last route, call fib6_repair_tree() to:
* 1. For root node, put back null_entry as how the table was created.
^ permalink raw reply related [flat|nested] 3+ messages in thread