Re: [PATCH v4 net-next 2/8] vxlan: vnifilter: use list_for_each_entry_rcu() in vxlan_vnifilter_dump_dev()

[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 · Low: 1

- [Low] Incomplete dump consistency in the paginated RTM_GETTUNNEL dump:
  both resume cursors remain plain ordinals (cb->args[0] = netdev…

--

vxlan: vnifilter: use list_for_each_entry_rcu() in vxlan_vnifilter_dump_dev()

This switches the vg->vni_list walk in vxlan_vnifilter_dump_dev() to
list_for_each_entry_rcu(), since writers modify the list with
list_add_rcu()/list_del_rcu().  It also clears cb->args[1] on the early
return paths and tracks the device ifindex in cb->args[2] so a stale VNI
offset is not carried over to the next device of a paginated dump.

> diff --git a/drivers/net/vxlan/vxlan_vnifilter.c b/drivers/net/vxlan/vxlan_vnifilter.c
> index e433e66f6d4d3..4404b912a7427 100644
> --- a/drivers/net/vxlan/vxlan_vnifilter.c
> +++ b/drivers/net/vxlan/vxlan_vnifilter.c
> @@ -333,22 +333,34 @@ static int vxlan_vnifilter_dump_dev(const struct net_device *dev,
>  				    struct sk_buff *skb,
>  				    struct netlink_callback *cb)
>  {
> -	struct vxlan_vni_node *tmp, *v, *vbegin = NULL, *vend = NULL;
> +	struct vxlan_vni_node *v, *vbegin = NULL, *vend = NULL;
>  	struct vxlan_dev *vxlan = netdev_priv(dev);
>  	struct tunnel_msg *new_tmsg, *tmsg;
> -	int idx = 0, s_idx = cb->args[1];
>  	struct vxlan_vni_group *vg;
>  	struct nlmsghdr *nlh;
> +	int idx = 0, s_idx;
>  	bool dump_stats;
>  	int err = 0;
>  
> -	if (!(vxlan->cfg.flags & VXLAN_F_VNIFILTER))
> +	if (cb->args[2] != dev->ifindex) {
> +		cb->args[1] = 0;
> +		cb->args[2] = dev->ifindex;
> +	}
> +	s_idx = cb->args[1];
> +
> +	if (!(vxlan->cfg.flags & VXLAN_F_VNIFILTER)) {
> +		cb->args[1] = 0;
> +		cb->args[2] = 0;
>  		return -EINVAL;
> +	}
>  
>  	/* RCU needed because of the vni locking rules (rcu || rtnl) */
>  	vg = rcu_dereference(vxlan->vnigrp);
> -	if (!vg || !vg->num_vnis)
> +	if (!vg || !vg->num_vnis) {
> +		cb->args[1] = 0;
> +		cb->args[2] = 0;
>  		return 0;
> +	}

[Severity: Low]
The new cb->args[2] check only runs after vxlan_vnifilter_dump() has already
selected a device using the unchanged ordinal cursor cb->args[0]:

drivers/net/vxlan/vxlan_vnifilter.c:vxlan_vnifilter_dump() {
	...
	for_each_netdev_rcu(net, dev) {
		if (!netif_is_vxlan(dev))
			continue;
		if (idx < s_idx)
			goto skip;
		err = vxlan_vnifilter_dump_dev(dev, skb, cb);
	...
}

If a vxlan device is unregistered between two dump skbs, the ordinal of the
partially dumped device drops below cb->args[0] and it is skipped before
vxlan_vnifilter_dump_dev() gets a chance to compare ifindexes.

Can the remaining VNIs of that device then be dropped from the dump result
even with the new ifindex tracking in place?

Also, is the extra clearing of cb->args[1]/cb->args[2] in these two early
return paths needed?  The unconditional check at the top of
vxlan_vnifilter_dump_dev() already resets cb->args[1] for any device whose
ifindex differs from the saved one, and vxlan_vnifilter_dump() breaks out of
for_each_netdev_rcu() on -EMSGSIZE without visiting further devices.

>  
>  	tmsg = nlmsg_data(cb->nlh);
>  	dump_stats = !!(tmsg->flags & TUNNEL_MSG_FLAG_STATS);
> @@ -362,7 +374,7 @@ static int vxlan_vnifilter_dump_dev(const struct net_device *dev,
>  	new_tmsg->family = PF_BRIDGE;
>  	new_tmsg->ifindex = dev->ifindex;
>  
> -	list_for_each_entry_safe(v, tmp, &vg->vni_list, vlist) {
> +	list_for_each_entry_rcu(v, &vg->vni_list, vlist) {
>  		if (idx < s_idx) {
>  			idx++;
>  			continue;

[Severity: Low]
Resume inside the device is still a plain position into vg->vni_list, and
RTNL/RCU are dropped between dump skbs.  If a VNI located before s_idx is
removed between two calls, the entries after it shift down by one, so is one
VNI skipped from the dump?  Conversely, a VNI added before s_idx would make
one entry appear twice.

At this commit there is no generation counter, no cb->seq and no
nl_dump_check_consistent(), so userspace gets no NLM_F_DUMP_INTR indication
that the dump was interrupted.

The next patch in the series, "vxlan: vnifilter: signal interrupted
RTM_GETTUNNEL dumps", adds vxlan_vnifilter_base_seq()/cb->seq plus
nl_dump_check_consistent() in vxlan_vnifilter_dump_dev(), which covers this
case.  Would it be worth noting in this commit message that skipped or
duplicated VNIs remain possible until that patch, since this one already
claims to address shifting ordinal indexes?

> @@ -394,6 +406,7 @@ static int vxlan_vnifilter_dump_dev(const struct net_device *dev,
>  	}
>  
>  	cb->args[1] = err ? idx : 0;
> +	cb->args[2] = err ? dev->ifindex : 0;
>  
>  	nlmsg_end(skb, nlh);
>

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