Re: [PATCH v6 net-next 1/8] vxlan: update default fdb entries when the lower device changes

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Regression in vxlan_vni_update_group()
  (drivers/net/vxlan/vxlan_vnifilter.c).
- [Medium] Retrying a failed per-VNI group change now reports success
  without joining the multicast group.
- [Medium] vxlan: FDB state corruption due to extack API misuse

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the per-VNI
  IGMP leave paths drop a membership that other VNIs on the same device,…
- [High] This problem wasn't introduced by this patch, but a per-VNI IPv6
  multicast group can cause a NULL pointer dereference when the device…
- [Medium] This problem wasn't introduced by this patch, but the unwind
  in vxlan_multicast_join_vnigrp() can remove multicast memberships…

--

vxlan: update default fdb entries when the lower device changes

With this patch, vxlan_changelink() refreshes the default fdb entries,
including the per-VNI ones on VNIFILTER devices, when only the lower
device changes. It does this by passing the old and new ifindex down to
vxlan_update_default_fdb_entry(). It also sets oldrip in
vxlan_vni_update_group() whenever an existing VNI is updated, and moves
per-VNI multicast memberships during changelink for VNIFILTER devices.

[ ... ]

[Severity: Medium]
This isn't a bug introduced by this patch, but this change makes
vxlan_multicast_join() reachable from vxlan_changelink() for VNIFILTER
devices whose default remote is not multicast. Can the unwind in
vxlan_multicast_join_vnigrp() drop memberships that belong to other
devices sharing the same socket?

drivers/net/vxlan/vxlan_multicast.c:vxlan_multicast_join_vnigrp() {
    ...
		ret = vxlan_igmp_join(vxlan, &v->remote_ip, 0);
		if (ret == -EADDRINUSE)
			ret = 0;
		if (ret)
			goto out;
		vgood = v;
	}
out:
	if (ret) {
		list_for_each_entry_safe(v, tmp, &vg->vni_list, vlist) {
			...
			vxlan_igmp_leave(vxlan, &v->remote_ip, 0);
			if (v == vgood)
				break;
    ...
}

A VNI whose join returned -EADDRINUSE got nothing new, because another
device already holds that membership on the shared socket. It is still
recorded as vgood, though.

If a later join fails (for example -ENOBUFS once igmp_max_memberships is
reached), the unwind leaves those groups without checking
vxlan_group_used(). If vgood is still NULL, it leaves every group.
Another device on the same socket would then stop receiving that group.

vxlan_open()->vxlan_multicast_join() already reaches this unwind, so
the problem predates this patch.

[Severity: Medium]
With this change a NULL extack alters what
vxlan_update_default_fdb_entry() does. When vxlan_fdb_update() fails
to add the new rdst, the error is no longer returned right away.
Instead the code falls through to __vxlan_fdb_delete() and removes the
old rdst anyway.

The rollback needs exactly that behavior. The unwind in
vxlan_vnilist_update_group() and in vxlan_changelink() has to remove
the new rdst even if re-adding the old one fails. Elsewhere in netlink
code, though, extack is optional, and NL_SET_ERR_MSG() and friends
accept NULL. A forward update that reaches this helper without an
extack would silently get rollback semantics.

For example, suppose the forward loop in vxlan_vnilist_update_group()
ran with a NULL extack and vxlan_fdb_update() failed with -ENOMEM for
one VNI. That VNI's old all-zeros rdst would be deleted and nothing
would replace it. The err_unwind loop starts at the entry after hpos
in list order, so it never revisits the VNI that failed. That VNI
would be left with no default fdb entry, and the fdb state would no
longer match default_dst or the VNI configuration.

Today vxlan_changelink() and vxlan_vni_add() only reach this helper
from rtnetlink with a non-NULL extack, so the forward case is not
reachable yet. Would it be cleaner to pass the rollback intent as a
separate bool argument? Then the add-failure handling would not depend
on whether the caller has an error message buffer.

[ ... ]

[ ... ]

[Severity: Medium]
Since oldrip is now set whenever !create, can the existing equality
check just below return early and drop the requested change?

	if (!create && oldrip && newrip && vxlan_addr_equal(oldrip, newrip))
		return 0;

This return runs before the group is stored in vninode->remote_ip and
before *changed is set, so no RTM_NEWTUNNEL notification is sent. Before
this patch, oldrip was only set when newrip was NULL, so this return could
not be reached.

Here are two cases where it seems to matter.

VNI 10 inherits the device group D (vninode->remote_ip is zero), and
"bridge vni add dev vxlan0 vni 10 group D" is run. newrip is group (D)
and oldrip is &dst->remote_ip (D). The function returns 0 and the VNI is
never pinned to D.

VNI 10 has an explicit group G equal to D and is re-added without a group
to clear it. vxlan_process_vni_filter() passes a zeroed, non-NULL group,
so newrip is &dst->remote_ip (D) and oldrip is &old_remote_ip (G). The
explicit group is never cleared.

In both cases success is returned, but the stored state does not match
the request.

vxlan_vnilist_update_group() in this patch uses
vxlan_addr_any(&vent->remote_ip) to decide whether a VNI follows a device
group change. So a later "ip link set vxlan0 type vxlan group D2" would
move the VNI in the first case, and fail to move it in the second, against
its configuration. "bridge vni show" would also report the wrong group
through vxlan_fill_vni_filter_entry().

Should the early return also check whether vninode->remote_ip itself is
changing?

[Severity: Medium]
A related question about the same early return: can retrying a failed
group change now succeed without joining the multicast group?

When an existing VNI moves from group A to B, vxlan_vni_update_group()
stores B before it joins:

	if (group)
		memcpy(&vninode->remote_ip, group, sizeof(vninode->remote_ip));
	...
		if (vxlan_addr_multicast(&vninode->remote_ip)) {
			ret = vxlan_igmp_join(vxlan, &vninode->remote_ip, 0);
			...
			if (ret)
				goto out;

If vxlan_igmp_join() fails, for example with -ENOBUFS once
net.ipv4.igmp_max_memberships is reached, the error goes back to user
space but B stays stored.

Suppose the limit is then raised and "bridge vni add ... group B" is run
again. oldrip is &old_remote_ip (B) and newrip is group (B), so the
equality check returns 0 before the leave/join block runs. The VNI stays
configured for B with no membership, and the command reports success.

Before this patch, oldrip was NULL here, so the retry still reached the
IGMP block.

 
 	if (!newrip && !oldrip)
 		return 0;

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but can this leave drop a
membership that another VNI on the same device, or the device default
group, still needs?

vxlan_group_used() returns false immediately when the socket refcnt is 1.
Otherwise it skips the device itself:

drivers/net/vxlan/vxlan_multicast.c:vxlan_group_used() {
    ...
	if (family == AF_INET && sock4 && refcount_read(&sock4->refcnt) == 1)
		return false;
    ...
	list_for_each_entry(vxlan, &vn->vxlan_list, next) {
		if (!netif_running(vxlan->dev) || vxlan == dev)
			continue;
    ...
}

Its vni argument is never used, so sibling VNIs and default_dst on the
same device are never counted.

For example, take an UP VNIFILTER device where VNI 10 and VNI 20 both use
239.1.1.1. vxlan_multicast_join_vnigrp() joins once and treats the second
-EADDRINUSE as success. "bridge vni del dev vxlan0 vni 10" then goes
through vxlan_vni_del()->vxlan_vni_delete_group()->vxlan_igmp_leave() and
removes the socket's only membership. VNI 20 then stops receiving BUM
traffic for that group.

In the same way, moving a VNI whose explicit group equals
default_dst.remote_ip to another group makes this block leave the default
group that vxlan_multicast_join() joined for the device.

The patch adds a default group skip and -EADDRNOTAVAIL tolerance to
vxlan_multicast_leave_vnigrp() for shared groups. The single-VNI update and
delete paths still behave as before. Should they follow the same rules?

[Severity: High]
This is a pre-existing issue, but can the per-VNI join and leave calls
here dereference a NULL vn6_sock?

vxlan_sock_add() tolerates -EAFNOSUPPORT when it creates the IPv6 socket:

		ret = __vxlan_sock_add(vxlan, true);
		if (ret < 0 && ret != -EAFNOSUPPORT)
			ipv4 = false;

With CONFIG_IPV6=y and ipv6.disable=1, a collect metadata VNIFILTER
device can therefore be up with vn6_sock == NULL.

vxlan_process_vni_filter() accepts VXLAN_VNIFILTER_ENTRY_GROUP6 without
checking for an IPv6 socket. vxlan_igmp_join() then does:

		struct vxlan_sock *sock6 = rtnl_dereference(vxlan->vn6_sock);

		sk = sock6->sk;

The call chain is RTM_NEWTUNNEL -> vxlan_vnifilter_process() ->
vxlan_process_vni_filter() -> vxlan_vni_add() -> vxlan_vni_update_group()
-> vxlan_igmp_join(). For example, "bridge vni add dev vxlan0 vni 10 group
ff05::1" on an UP device.

vxlan_open() reaches the same helper through vxlan_multicast_join_vnigrp().
With this patch, vxlan_changelink() also reaches it for VNIFILTER devices.

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