Re: [PATCH v5 net-next 7/8] vxlan: remove default_dst and use vxlan_config and lowerdev

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] The patch is advertised as a pure redundancy-removal refactor
  ("remove default_dst and use vxlan_config and lowerdev"), but it…

Pre-existing issues:
- [High] On a running VXLAN device with VXLAN_F_VNIFILTER, a changelink
  that swaps the lower device (or changes the remote) makes…
- [Medium] This problem wasn't introduced by this patch, but the patch
  touches the affected function: in vxlan_vni_update_group()…

--

vxlan: remove default_dst and use vxlan_config and lowerdev

This drops vxlan->default_dst and replaces it with a plain
struct net_device *lowerdev in struct vxlan_dev, moving the remaining
readers of remote_ip, remote_vni and remote_ifindex over to vxlan->cfg.
vxlan_config_apply() gains a lowerdev_changed argument, and
vxlan_config_validate() gains a new rejection when IFLA_VXLAN_LINK is
cleared while per-VNI multicast groups exist.

A few questions below about how the behaviour changes are packaged, plus
two older issues in code this patch touches.

(no Link: tags in the commit header)

[ ... ]

[Severity: Low]

The subject presents this as removing a redundant copy of the default
destination, but two of the changes here look like fixes for existing
user triggerable defects, and there is no Fixes: tag for either.  Would
it make sense to split them out?

Before this patch vxlan_changelink() forced lowerdev to NULL when it was
unchanged:

	if (dst->remote_dev == lowerdev)
		lowerdev = NULL;

and vxlan_config_apply() only derives the headroom inside if (lowerdev):

	needed_headroom = lowerdev->hard_header_len;
	needed_headroom += lowerdev->needed_headroom;

So any changelink that kept the same lower device, for example
ip link set vxlan0 type vxlan ttl 64, recomputed dev->needed_headroom as
ETH_HLEN + vxlan_headroom(flags) and dropped the lower device's
contribution.  Passing lowerdev unconditionally repairs that, and the
new lowerdev_changed argument keeps the previous mtu semantics.

The second one: vxlan_config_apply() used to commit
dst->remote_ifindex = new_cfg->remote_ifindex only when lowerdev was
non-NULL, while vxlan->cfg received remote_ifindex == 0 anyway, so
clearing IFLA_VXLAN_LINK left default_dst.remote_ifindex stale.
vxlan_fill_info(), vxlan_change_mtu(), vxlan_get_link_ksettings() and
vxlan_handle_lowerdev_unregister() all read that stale value.  This
patch removes the divergence, again with no Fixes: tag.

Finally, the new vxlan_vnifilter_has_multicast() check in
vxlan_config_validate() makes a changelink with IFLA_VXLAN_LINK=0 return
-EINVAL where it was previously accepted, and IFLA_VXLAN_LINK reporting
after such a request changes as well.

Could the two fixes be carried as separate patches with Fixes: tags so
they can be picked up for stable, and the new -EINVAL be called out as a
user visible change?

[ ... ]

[ ... ]

[Severity: High]

This is a pre-existing issue and not introduced by this patch, but since
the error handling around vxlan_multicast_leave() is being touched here,
can a failing leave leave a running device with no memberships at all?

With VXLAN_F_VNIFILTER, vxlan_multicast_leave() calls
vxlan_multicast_leave_vnigrp(), which walks every VNI without
deduplicating:

	list_for_each_entry_safe(v, tmp, &vg->vni_list, vlist) {
		if (vxlan_addr_multicast(&v->remote_ip) &&
		    !vxlan_group_used(vn, vxlan, v->vni, &v->remote_ip, 0)) {
			ret = vxlan_igmp_leave(vxlan, &v->remote_ip, 0);
			if (ret)
				last_err = ret;
		}
	}

The join side collapses duplicates, so only one membership exists:

	bridge vni add dev vx0 vni 100 group 239.1.1.1
	bridge vni add dev vx0 vni 200 group 239.1.1.1

vxlan_multicast_join_vnigrp() maps the second join's -EADDRINUSE to
success, and it also skips any per-VNI group equal to cfg->remote_ip,
which the leave path does not.

vxlan_group_used() cannot see the duplicate inside the same device:

	if (family == AF_INET && sock4 && refcount_read(&sock4->refcnt) == 1)
		return false;
	...
		if (!netif_running(vxlan->dev) || vxlan == dev)
			continue;

so the second vxlan_igmp_leave() reaches ip_mc_leave_group() with no
membership left and returns -EADDRNOTAVAIL, which is propagated as
last_err.

At that point the new adjacency and the new cfg are still committed, and
the

	if (!err && change_igmp && ...)
		err = vxlan_multicast_join(vxlan);

guard skips the rejoin, so the device keeps running with no group
membership and stops receiving flooded traffic.  Retrying the same
request does not help, since cfg->remote_ifindex now matches the
requested value and change_igmp is false, so only an administrative
down/up restores it.  Should the leave path skip groups it never joined,
or should the rejoin not be gated on the leave result?

[ ... ]

[Severity: Medium]

This isn't a bug introduced by this patch either, but vxlan_vni_update_group()
is being converted here, so while looking at it: does replacing one non-zero
per-VNI group with another leave the old flood destination behind?

oldrip is only set when there is no newrip:

	if (!newrip && !vxlan_addr_any(&old_remote_ip))
		oldrip = &old_remote_ip;

so on a replacement vxlan_update_default_fdb_entry() is called with
old_remote_ip == NULL and only runs the NLM_F_APPEND | NLM_F_CREATE
update.  For the all-zeros MAC that goes through
vxlan_fdb_update_existing() -> vxlan_fdb_append(), which keeps the
existing rdst:

	if ((flags & NLM_F_APPEND) &&
	    (is_multicast_ether_addr(f->key.eth_addr) ||
	     is_zero_ether_addr(f->key.eth_addr))) {
		rc = vxlan_fdb_append(f, ip, port, vni, ifindex, &rd);

vninode->remote_ip is then overwritten with the new group and the request
succeeds, so the zero-MAC entry ends up with two remotes:

	bridge vni add dev vx0 vni 100 group 239.1.1.1
	bridge vni add dev vx0 vni 100 group 239.1.1.2

vxlan_vni_add() routes the second request to vxlan_vni_update() ->
vxlan_vni_update_group() with create == false.  A later VNI delete only
targets vninode->remote_ip in vxlan_vni_delete_group(), so the stale rdst
survives and re-adding the VNI accumulates more.  The same applies when a
per-VNI group is switched back to a non-zero device default.

Should the replacement case pass the previous address as old_remote_ip so
the stale destination is removed?

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