Re: [PATCH v4 net-next 3/8] vxlan: vnifilter: signal interrupted RTM_GETTUNNEL dumps

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

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

- [Medium] The generation counter added by this patch is not bumped on a
  mutating error path, so an RTM_GETTUNNEL dump that straddles the…

--

vxlan: vnifilter: signal interrupted RTM_GETTUNNEL dumps

This adds a per-netns generation counter to struct vxlan_net, bumped when
the set of vxlan devices in the netns changes or when a vni_list is
modified, and feeds it to nl_dump_check_consistent() from
vxlan_vnifilter_dump_dev() so user space sees NLM_F_DUMP_INTR on an
interrupted RTM_GETTUNNEL dump.

No Link: tags in the commit header.

[ ... ]

[Severity: Medium]

Should the counter also be bumped when vxlan_vni_update_group() fails
after it has already overwritten vninode->remote_ip?

In vxlan_vni_update_group() the new group is stored into the dump-visible
list node before the fallible multicast steps:

	if (group)
		memcpy(&vninode->remote_ip, group, sizeof(vninode->remote_ip));

	if (vxlan->dev->flags & IFF_UP) {
		if (vxlan_addr_multicast(&old_remote_ip) &&
		    !vxlan_group_used(vn, vxlan, vninode->vni, ...)) {
			ret = vxlan_igmp_leave(vxlan, &old_remote_ip, 0);
			if (ret)
				goto out;
		}

		if (vxlan_addr_multicast(&vninode->remote_ip)) {
			ret = vxlan_igmp_join(vxlan, &vninode->remote_ip, 0);
			if (ret == -EADDRINUSE)
				ret = 0;
			if (ret)
				goto out;
		}
	}

	*changed = true;

	return 0;
out:
	return ret;

The memcpy is not undone on either goto out, and *changed stays false, so
vxlan_vni_update() takes the "if (ret) return ret;" path above and never
reaches the new vxlan_vnifilter_seq_inc() call even though the entry's
dumped content has already changed.

Is that failure reachable in practice?  vxlan_igmp_join() ends in
ip_mc_join_group():

net/ipv4/igmp.c:__ip_mc_join_group() {
	...
	if (count >= READ_ONCE(net->ipv4.sysctl_igmp_max_memberships))
		goto done; /* err = -ENOBUFS */
	...
}

which returns -ENOBUFS once the default net.ipv4.igmp_max_memberships
limit of 20 is exceeded on the vxlan socket.

The dump reads v->remote_ip both for the VXLAN_VNIFILTER_ENTRY_GROUP and
GROUP6 attributes and for the range coalescing decision in
vxlan_vnifilter_dump_dev():

		if (!dump_stats && vnirange(vend, v) == 1 &&
		    vxlan_addr_equal(&v->remote_ip, &vend->remote_ip)) {

If such a failed update lands between two dump skbs, does
vxlan_vnifilter_base_seq() return the same value, so
nl_dump_check_consistent() leaves NLM_F_DUMP_INTR clear and user space
assembles a dump mixing pre- and post-mutation group content with no
indication the snapshot was interrupted?

The entry ordinals still add up in this case (idx += vnirange(vbegin,
vend) + 1 over a coalesced run equals its node count), so this does not
duplicate or skip entries, only skews the reported group addresses.  The
same error path also skips the RTM_NEWTUNNEL notification, so no listener
learns of the change that was applied.

Would it make sense to bump the counter right where remote_ip is
committed, or to restore the old remote_ip on the goto out paths?

The condition is still present at the end of the series, in
vxlan_vni_update_group() and vxlan_vni_update().

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