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

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] vxlan_vnifilter_dump_dev() resumes a paginated RTM_GETTUNNEL dump
  inside one device by a positional ordinal (cb->args[1], applied via…

Pre-existing issues:
- [Low] vxlan_vnifilter_dump() still resumes the device walk by ordinal
  position (cb->args[0], 'if (idx < s_idx) goto skip;'), while the new…

--

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() from
list_for_each_entry_safe() to list_for_each_entry_rcu(), and clears
cb->args[1] on the early return paths.  It also records the current
device ifindex in cb->args[2] so the per-device VNI offset is reset when
the device being dumped changes.

A couple of questions about the resume bookkeeping below.

[Severity: Low]
This isn't a bug introduced by this patch, but the two halves of the
resume state now use different encodings.  vxlan_vnifilter_dump_dev()
keys per-device progress on dev->ifindex in cb->args[2], while the
caller vxlan_vnifilter_dump() still resumes the device walk by ordinal:

	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 positioned before the partially dumped device is
unregistered between dump callbacks, the ordinals shift and the
in-progress device is now at idx < s_idx, so it is skipped before
vxlan_vnifilter_dump_dev() is ever reached and the new cb->args[2]
check cannot re-locate it.  The remainder of that device's VNIs is
then never emitted (and if it was the last device, the callback emits
an empty skb and the dump ends with NLMSG_DONE).

Would it make sense to key the device walk on ifindex as well, so the
resume point survives device removal?

For the record, the following patch in this series, "vxlan: vnifilter:
signal interrupted RTM_GETTUNNEL dumps", bumps a per-netns generation
counter on NETDEV_REGISTER/NETDEV_UNREGISTER of vxlan devices and
publishes it through cb->seq plus nl_dump_check_consistent(), and
netlink_dump_done() applies the same check to NLMSG_DONE, so after that
patch the truncation is at least reported to user space via
NLM_F_DUMP_INTR.

[Severity: Low]
The new cb->args[2] check validates which device the saved offset
belongs to, but the offset itself is still a position in vg->vni_list,
and writers insert or remove entries anywhere in that sorted list:

drivers/net/vxlan/vxlan_vnifilter.c:__vxlan_vni_add_list() {
	...
	list_add_rcu(&v->vlist, hpos);
	vg->num_vnis++;
	...
}

drivers/net/vxlan/vxlan_vnifilter.c:__vxlan_vni_del_list() {
	...
	list_del_rcu(&v->vlist);
	vg->num_vnis--;
	...
}

Since RTNL is dropped between dump skbs, can a "bridge vni add/del" on
the device being dumped shift that cursor, so the next callback skips or
repeats VNI entries?

[Severity: Low]
At this commit no generation counter is recorded in cb->seq and
nl_dump_check_consistent() is not called here, so a skipped or duplicated
VNI is invisible to user space, i.e. NLM_F_DUMP_INTR is never set on the
affected messages.  The sibling dump in the same driver does maintain
such a counter:

drivers/net/vxlan/vxlan_mdb.c:vxlan_mdb_dump_dev() {
	...
	cb->seq = vxlan->mdb_seq;
	nl_dump_check_consistent(cb, nlh);
	...
}

Should this dump do the same?  Note the immediately following patch in
this series, "vxlan: vnifilter: signal interrupted RTM_GETTUNNEL dumps",
adds a per-netns atomic counter (vxlan_net.vnifilter_seq) incremented in
__vxlan_vni_add_list(), __vxlan_vni_del_list() and
vxlan_vni_update_group(), sets cb->seq in vxlan_vnifilter_dump() and
calls nl_dump_check_consistent(cb, nlh) here, which addresses this.  Is
there a reason to keep the two changes in separate patches, given the
positional resume is being reworked in this one?

 
 	nlmsg_end(skb, nlh);

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