Re: [PATCH net-next 3/9] vxlan: vnifilter: use list_for_each_entry_rcu() in vxlan_vnifilter_dump_dev()

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

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

- [Medium] Commit message misstates the locking context that justifies
  change 1.
- [Medium] The fix is incomplete: the exact symptom the commit message
  claims to close — a non-zero cb->args[1] being applied to a device it…

Pre-existing issues:
- [Low] Claimed unsynchronized read of multi-word state (vg->num_vnis and
  vxlan_vni_node::remote_ip) by the RCU-only dump path while RTNL-only…

--

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

This converts the vg->vni_list traversal in vxlan_vnifilter_dump_dev() from
list_for_each_entry_safe() to list_for_each_entry_rcu(), and clears
cb->args[1] on the two early returns (no VXLAN_F_VNIFILTER, and no VNIs in
the group) so a paginated dump does not carry a stale index to the next
device.

A couple of questions about the changelog and about the completeness of the
cb->args[1] fix.

    vxlan_vnifilter_dump_dev() runs under rcu_read_lock() without RTNL when
    dumping VNI filter entries via RTM_GETTUNNEL.

    1) Currently it traverses vg->vni_list using list_for_each_entry_safe(),
       which performs raw pointer accesses without RCU dereference barriers.
       Since concurrent RTNL writers modify vg->vni_list using list_add_rcu()
       and list_del_rcu(), use list_for_each_entry_rcu() instead.

[Severity: Medium]
Is the "without RTNL" premise accurate for this dump at this revision?

The PF_BRIDGE/RTM_GETTUNNEL handler is registered with a zero flags field,
i.e. without RTNL_FLAG_DUMP_UNLOCKED:

drivers/net/vxlan/vxlan_vnifilter.c:
static const struct rtnl_msg_handler vxlan_vnifilter_rtnl_msg_handlers[] = {
	{THIS_MODULE, PF_BRIDGE, RTM_GETTUNNEL, NULL, vxlan_vnifilter_dump, 0},

so rtnetlink_dump_start() installs rtnl_dumpit() as the dump callback:

net/core/rtnetlink.c:rtnl_dumpit() {
	const bool needs_lock = !(cb->flags & RTNL_FLAG_DUMP_UNLOCKED);
	...
	if (needs_lock)
		rtnl_lock();
	err = dumpit(skb, cb);
	...
}

All writers of vg->vni_list (__vxlan_vni_add_list()/__vxlan_vni_del_list(),
vxlan_vs_add_vnigrp()/vxlan_vs_del_vnigrp(), vxlan_vnigroup_uninit()) run
under RTNL with rtnl_dereference()/ASSERT_RTNL(). With RTNL held around
vxlan_vnifilter_dump() the traversal cannot observe a concurrent
list_add_rcu()/list_del_rcu(), which would make part 1 an annotation
improvement rather than a fix for a reachable race. I also checked the end
of the series and the handler flags are still 0 there.

Would it make sense to describe part 1 as an annotation/lockdep improvement
(and preparation for an RCU-only dump), and keep the Fixes: tag tied to the
cb->args[1] problem in part 2?

[ ... ]

[Severity: Medium]
Do these two clears cover the symptom described in the changelog?

vxlan_vnifilter_dump() resumes the multi-device walk by ordinal position,
and both RTNL and RCU are dropped between netlink_dump() invocations:

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 (err == -EMSGSIZE)
				break;
skip:
			idx++;
		}
	}
	cb->args[0] = idx;
	rcu_read_unlock();

If a vxlan device positioned before the paused one is unregistered in that
gap, the device now sitting at idx == s_idx is a different device. That
device has VXLAN_F_VNIFILTER set and a non-empty group, so neither new
cb->args[1] = 0 path is taken, and vxlan_vnifilter_dump_dev() consumes the
stale index:

	int idx = 0, s_idx = cb->args[1];
	...
	list_for_each_entry_rcu(v, &vg->vni_list, vlist) {
		if (idx < s_idx) {
			idx++;
			continue;
		}

which skips that device's first N VNIs — the exact case the changelog says
is being fixed. A device inserted at the head produces the mirror case, a
device re-dumped from a bogus VNI offset. The same positional resume inside
one device's vni_list can skip or repeat entries when VNIs are added or
removed via __vxlan_vni_add_list()/__vxlan_vni_del_list() between
invocations.

Would resetting cb->args[1] in vxlan_vnifilter_dump() whenever it moves on
to a device other than the paused one (or resuming by ifindex and VNI value
instead of by position) close the remaining cases? And since the iteration
can still skip or repeat objects, should the dump record a generation
counter in cb->seq so NLM_F_DUMP_INTR is reported to user space?

 
 	tmsg = nlmsg_data(cb->nlh);
 	dump_stats = !!(tmsg->flags & TUNNEL_MSG_FLAG_STATS);

[ ... ]

[Severity: Low]
This isn't a bug introduced by this patch, but if the dump is later made
RTNL-less, are the per-node fields read in this loop safe for an RCU-only
reader?

The loop and vxlan_fill_vni_filter_entry() read the multi-word
vxlan_vni_node::remote_ip union, and the early return above reads
vg->num_vnis, while the writers update them in place under RTNL only:

drivers/net/vxlan/vxlan_vnifilter.c:vxlan_vni_update_group() {
	if (group)
		memcpy(&vninode->remote_ip, group, sizeof(vninode->remote_ip));
}

and vg->num_vnis++/-- in __vxlan_vni_add_list()/__vxlan_vni_del_list().

Today this is serialized because rtnl_dumpit() holds RTNL for this handler,
so there is no torn read. If the plan is to drop RTNL from this dump, would
these fields need their own publish/read discipline as well?

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