Re: [PATCH net-next] ipv4: fib: fix route re-dump in inet_dump_fib() on multi-batch dump

[Ido Schimmel]

On Fri, Jun 26, 2026 at 04:56:36PM +0800, Pengfei Zhang wrote:
> inet_dump_fib() saves its progress in cb->args[1] as a positional
> index within the current hash chain.  Between batches, a concurrent
> fib_new_table() can insert a new table at the chain head, shifting
> all existing entries.  On resume the saved index lands on a different
> table, causing already-dumped tables to be re-dumped and the
> originally suspended table to restart from the beginning.
> 
> Fix by storing tb->tb_id in cb->args[1] instead of a positional
> index, mirroring the fix applied to inet6_dump_fib().

Wait for it to be applied, then reference the commit.

> 
> Fixes: 1b43af5480c3 ("[IPV6]: Increase number of possible routing tables to 2^32")

Please drop the fixes tag given this is targeted at net-next.

Also, net-next is currently closed. Submit v2 when it opens:

https://netdev.bots.linux.dev/net-next.html

> Signed-off-by: Pengfei Zhang <zhangfeionline@xxxxxxxxx>
> ---
> Consider a hash slot containing two tables [A(pos=0), B(pos=1)] where
> B is large enough to require multiple batches.  On the first batch, B
> suspends mid-walk and the loop saves:
> 
>   cb->args[1] = e;   /* e=1, position of B in the chain */
> 
> The lock is then released.  At this point a concurrent fib_new_table()
> inserts table C at the chain head via hlist_add_head_rcu(), making the
> chain [C(pos=0), A(pos=1), B(pos=2)].
> 
> On the next batch, inet_dump_fib() resumes with s_e=1 and iterates:
> 
>   s_e = cb->args[1];   /* s_e = 1 */
>   hlist_for_each_entry_rcu(tb, head, tb_hlist) {
>       if (e < s_e)     /* skip C at pos=0 */
>           goto next;
>       /* e=1: tb now points to A, not B */
>       if (dumped)
>           memset(...);  /* resets B's suspended progress */
>       fib_table_dump(tb, ...);   /* re-dumps A from scratch */
>       dumped = 1;
>       /* e=2: tb now points to B */
>       fib_table_dump(tb, ...);   /* re-dumps B from beginning */
>   }
> 
> Routes from A are dumped twice, and the portion of B that was already
> dumped in the first batch is dumped again.
> 
>  net/ipv4/fib_frontend.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
> index 42212970d..65fa245af 100644
> --- a/net/ipv4/fib_frontend.c
> +++ b/net/ipv4/fib_frontend.c
> @@ -1019,10 +1019,11 @@ static int inet_dump_fib(struct sk_buff *skb, struct netlink_callback *cb)
>  		.dump_routes = true,
>  		.dump_exceptions = true,
>  	};
> -	unsigned int e = 0, s_e, h, s_h;
>  	struct hlist_head *head;
>  	int dumped = 0, err = 0;
> +	unsigned int h, s_h;

Move this line below the next line to maintain reverse xmas tree
ordering:

https://docs.kernel.org/process/maintainer-netdev.html#local-variable-ordering-reverse-xmas-tree-rcs

>  	struct fib_table *tb;
> +	u32 s_id;
>  
>  	rcu_read_lock();
>  	if (cb->strict_check) {
> @@ -1054,29 +1055,28 @@ static int inet_dump_fib(struct sk_buff *skb, struct netlink_callback *cb)
>  	}
>  
>  	s_h = cb->args[0];
> -	s_e = cb->args[1];
> +	s_id = cb->args[1];
>  
>  	err = 0;
> -	for (h = s_h; h < FIB_TABLE_HASHSZ; h++, s_e = 0) {
> -		e = 0;
> +	for (h = s_h; h < FIB_TABLE_HASHSZ; h++, s_id = 0) {
>  		head = &net->ipv4.fib_table_hash[h];
>  		hlist_for_each_entry_rcu(tb, head, tb_hlist) {
> -			if (e < s_e)
> -				goto next;
> +			if (s_id && tb->tb_id != s_id)
> +				continue;
> +
> +			s_id = 0;
>  			if (dumped)
>  				memset(&cb->args[2], 0, sizeof(cb->args) -
>  						 2 * sizeof(cb->args[0]));
> +			cb->args[1] = tb->tb_id;
>  			err = fib_table_dump(tb, skb, cb, &filter);
>  			if (err < 0)
>  				goto out;
>  			dumped = 1;
> -next:
> -			e++;
>  		}
>  	}
>  out:
>  
> -	cb->args[1] = e;
>  	cb->args[0] = h;
>  
>  unlock:
> -- 
> 2.34.1
>