Re: [PATCH v1 net 4/4] neighbour: Skip default parms when resumed in neightbl_dump_info().

[netdev-bot+sashiko@kernel.org]

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

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

- [Low] net/core/neighbour.c: neightbl_dump_info() — the patch does two
  independent things but documents only one.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but it is the
  same user-visible symptom class the changelog addresses and remains…

--

neighbour: Skip default parms when resumed in neightbl_dump_info().

This adds a new cb->args[2] flag (default_skip) so that
neightbl_dump_info() does not re-emit the default parms message for a
table when a multi-part dump resumes in the middle of that table's
per-device parms walk.

> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index b90957b630cee..15eedaef65781 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -2587,9 +2587,10 @@ static int neightbl_dump_info(struct sk_buff *skb, struct netlink_callback *cb)
>  {
>  	const struct nlmsghdr *nlh = cb->nlh;
>  	struct net *net = sock_net(skb->sk);
> +	int default_skip = cb->args[2];
> +	int neigh_skip = cb->args[1];
>  	int family, tidx, nidx = 0;
>  	int tbl_skip = cb->args[0];
> -	int neigh_skip = cb->args[1];
>  	struct neigh_table *tbl;
>  
>  	if (cb->strict_check) {
> @@ -2613,12 +2614,13 @@ static int neightbl_dump_info(struct sk_buff *skb, struct netlink_callback *cb)
>  		if (tidx < tbl_skip || (family && tbl->family != family))
>  			continue;
>  
> -		if (neightbl_fill_info(skb, tbl, NETLINK_CB(cb->skb).portid,
> +		if (!default_skip &&
> +		    neightbl_fill_info(skb, tbl, NETLINK_CB(cb->skb).portid,
>  				       nlh->nlmsg_seq, RTM_NEWNEIGHTBL,
>  				       NLM_F_MULTI) < 0)
>  			break;
>  
> -		nidx = 0;
> +		default_skip = 1;

[Severity: Low]
Is the removal of nidx = 0 from this spot, and its reappearance at the
tail of the per-table loop, a second and separate behaviour fix?

Before the patch, nidx was only reset right after the successful
neightbl_fill_info() call, never at the end of a table walk. So when the
skb fills up and neightbl_fill_info() fails for table N, the break path
stores the previous table's parm count:

	/* table 0 walked to completion, nidx == N */
	/* table 1: neightbl_fill_info() fails, break */
	cb->args[0] = 1;
	cb->args[1] = N;	/* table 0's count */

On resume neigh_skip is N while nidx restarts at 0 for table 1, so the
first N per-device parms of table 1 take:

			if (nidx < neigh_skip)
				goto next;

and are never emitted, which is the opposite symptom from the duplicate
described in the changelog.

After the patch nidx = 0 runs at the loop tail, so the break path stores
cb->args[1] = 0 and nothing is skipped. The changelog does not mention
this hunk at all. Could the changelog say that moving nidx = 0 fixes the
stale-nidx-on-break case, or could that hunk be split into its own patch?
As written it reads like cosmetic reordering that could be dropped during
review or a stable backport.

>  
>  		p = list_first_entry(&tbl->parms_list, struct neigh_parms, list);
>  		list_for_each_entry_continue_rcu(p, &tbl->parms_list, list) {
> @@ -2642,12 +2644,15 @@ static int neightbl_dump_info(struct sk_buff *skb, struct netlink_callback *cb)
>  		}
>  
>  		neigh_skip = 0;
> +		nidx = 0;
> +		default_skip = 0;
>  	}
>  out:
>  	rcu_read_unlock();
>  
>  	cb->args[0] = tidx;
>  	cb->args[1] = nidx;
> +	cb->args[2] = default_skip;

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but it is
the same class of user-visible symptom the changelog is fixing, so it may
be worth mentioning.

The parms walk in neightbl_dump_info() resumes by position index only.
rcu_read_lock() is dropped between dump invocations and no reference to
the last dumped struct neigh_parms is kept, so cb->args[1] can refer to a
different object next time around.

Writers mutate the same list under tbl->lock. neigh_parms_alloc() inserts
at the front of the region the dump iterates:

net/core/neighbour.c:neigh_parms_alloc() {
	...
	spin_lock_bh(&tbl->lock);
	list_add_rcu(&p->list, &tbl->parms.list);
	spin_unlock_bh(&tbl->lock);
	...
}

and neigh_parms_release() removes from it:

net/core/neighbour.c:neigh_parms_release() {
	...
	spin_lock_bh(&tbl->lock);
	list_del_rcu(&parms->list);
	parms->dead = 1;
	spin_unlock_bh(&tbl->lock);
	...
}

So if one invocation emits parms 0..4 and stores cb->args[1] = 5, and a
netdev is registered before the dump resumes, every previously dumped
entry shifts to index+1 and the entry formerly at index 4 is emitted a
second time. A netdev unregister shifts indices down instead and an entry
is silently dropped.

Neither cb->seq nor NLM_F_DUMP_INTR is used anywhere in
net/core/neighbour.c, so user space cannot detect this. Should a
generation counter plus NLM_F_DUMP_INTR be added on top of this patch for
the RTM_GETNEIGHTBL dump?

>  
>  	return skb->len;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907215853.3709987-1-kuniyu%40google.com