Re: [PATCH v3 net-next 5/6] vxlan: remove default_dst and use vxlan_config and lowerdev

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
vxlan: remove default_dst and use vxlan_config and lowerdev

This removes vxlan->default_dst, moving the default destination attributes
(remote_ip, remote_vni, remote_ifindex) over to the RCU-protected
vxlan->cfg, and adds a plain struct net_device *lowerdev to struct
vxlan_dev for upper/lower adjacency tracking under RTNL.  It also drops
the remote_dev field from struct vxlan_rdst and updates
mlx5e_tc_tun_get_remote_ifindex() to read remote_ifindex from vxlan->cfg.

A few questions below.

[ ... ]

[ ... ]

[Severity: Medium]
Can dropping the "lowerdev = NULL" shortcut here silently shrink the MTU of a
live vxlan device?

Previously, when the lower device was unchanged, lowerdev was set to NULL, so
vxlan_config_apply() skipped its whole "if (lowerdev)" block, max_mtu stayed at
ETH_MAX_MTU and the trailing clamp was a no-op:

	if (lowerdev) {
		...
		max_mtu = lowerdev->mtu - vxlan_headroom(flags);
		if (max_mtu < ETH_MIN_MTU)
			max_mtu = ETH_MIN_MTU;
		...
	}

	if (dev->mtu > max_mtu)
		dev->mtu = max_mtu;

Now the real lower device is always passed on changelink, so max_mtu is
recomputed every time.  vxlan_change_mtu() only validates against lowerdev->mtu
at the time of the request, and vxlan_netdevice_event() handles only
NETDEV_UNREGISTER and the udp tunnel port events, so nothing reacts to a lower
device MTU reduction:

	ip link add vxlan0 type vxlan id 10 dev eth0 dstport 4789 [mtu 1450]
	ip link set eth0 mtu 1000		# vxlan0 mtu unchanged
	ip link set dev vxlan0 type vxlan ageing 100

The last command inherits IFLA_VXLAN_LINK from the old config, so lowerdev is
non-NULL and unchanged, and dev->mtu is clamped down to
1000 - vxlan_headroom(flags).

The commit message describes passing lowerdev only as needed "to preserve
needed_headroom and needed_tailroom" and says nothing about MTU or the TSO
limits from netif_inherit_tso_max().  Was the MTU effect intended?

 
-	rem_ip_changed = !vxlan_addr_equal(&conf.remote_ip, &dst->remote_ip);
+	rem_ip_changed = !vxlan_addr_equal(&conf.remote_ip, &cfg->remote_ip);
 	change_igmp = vxlan->dev->flags & IFF_UP &&
 		      (rem_ip_changed ||
-		       dst->remote_ifindex != conf.remote_ifindex);
+		       cfg->remote_ifindex != conf.remote_ifindex);
 
 	/* handle default dst entry */
-	if (rem_ip_changed) {
+	if (rem_ip_changed || cfg->remote_ifindex != conf.remote_ifindex) {
 		spin_lock_bh(&vxlan->hash_lock);
 		if (!vxlan_addr_any(&conf.remote_ip)) {
 			err = vxlan_fdb_update(vxlan, all_zeros_mac,

[Severity: Medium]
Should this widened condition come with a Fixes: tag in a separate patch?

Before this change, a changelink that altered only the lower device
(for example "ip link set dev vxlan0 type vxlan group 239.1.1.1 dev eth1")
skipped this block because rem_ip_changed was false, while
vxlan_config_apply() updated dst->remote_ifindex.  The all-zeros-MAC rdst kept
the old ifindex, and vxlan_xmit_one() uses it as the route oif:

	ifindex = rdst->remote_ifindex;

so traffic kept leaving the old device.  The same applies to the
old_ifindex/new_ifindex plumbing added to vxlan_update_default_fdb_entry() in
vxlan_vnifilter.c.  These look like fixes of user-visible misbehaviour bundled
into a refactor, so they will not be picked up for stable.

The same is true of needed_headroom/needed_tailroom no longer being reset to
ETH_HLEN + vxlan_headroom() on changelink, which is mentioned only as an
implementation detail.

There is also an undocumented behaviour change further down in this function:
when IFLA_VXLAN_LINK is explicitly set to 0, the new "else
netdev_upper_dev_unlink(vxlan->lowerdev, dev);" branch now tears down the
old upper/lower adjacency, whereas netdev_adjacent_change_prepare() and
netdev_adjacent_change_commit() returned early for a NULL new device and the
adjacency survived.  Could that be called out in the changelog?

[Severity: Low]
Can this error path leave forwarding state half applied for an ifindex-only
change?

By the time vxlan_vnilist_update_group() runs, the default destination has
already been appended with the new remote/ifindex and the old one deleted, and
the loop in vxlan_vnilist_update_group() commits each per-VNI update in turn
and returns on the first failure without undoing the earlier ones.
vxlan_fdb_append() can fail:

	rd = kmalloc_obj(*rd, GFP_ATOMIC);
	if (rd == NULL)
		return -ENOMEM;

On that failure this branch only aborts the adjacency change and frees new_cfg,
so vxlan_config_apply() is never reached: vxlan->cfg and vxlan->lowerdev keep
the old values while the default fdb entry and some per-VNI entries already
carry the requested ifindex.  The rdsts allocated for the ifindex that was
never committed stay linked until explicit fdb or device teardown.  The
append-then-delete rollback gap already existed for remote-IP changes, but
ifindex-only changes are newly routed through it.

 
-	if (change_igmp && vxlan_addr_multicast(&dst->remote_ip))
+	if (change_igmp && vxlan_addr_multicast(&cfg->remote_ip))
 		err = vxlan_multicast_leave(vxlan);
 
 	if (netif_running(dev) && conf.age_interval != cfg->age_interval)
 		mod_timer(&vxlan->age_timer, jiffies);
 
-	netdev_adjacent_change_commit(dst->remote_dev, lowerdev, dev);
-	if (lowerdev && lowerdev != dst->remote_dev)
-		dst->remote_dev = lowerdev;
+	if (lowerdev_changed) {
+		if (lowerdev)
+			netdev_adjacent_change_commit(vxlan->lowerdev, lowerdev,
+						      dev);
+		else
+			netdev_upper_dev_unlink(vxlan->lowerdev, dev);
+		vxlan->lowerdev = lowerdev;
+	}
 	vxlan_config_apply(dev, new_cfg, lowerdev, vxlan->net, true);

[ ... ]

[ ... ]

[ ... ]

[Severity: Medium]
Is the new ifindex refresh complete for VNI entries that carry their own group
address?

The loop in vxlan_vnilist_update_group() only visits nodes whose remote_ip is
"any":

	if (vxlan_addr_any(&vent->remote_ip)) {
		ret = vxlan_update_default_fdb_entry(...);

But vxlan_vni_update_group() creates entries with an explicit group using the
device configuration ifindex, as seen in the hunk above
(cfg->remote_ifindex passed for both old and new), so those rdsts also store
the device's remote_ifindex and are skipped here.  After a changelink that
alters remote_ifindex, vxlan_config_apply() publishes the new cfg while those
rdsts keep the old value.

	bridge vni add vni 100 group 239.1.1.1 dev vx	# rdst ifindex = eth0
	ip link set dev vxlan0 type vxlan dev eth1	# or IFLA_VXLAN_LINK = 0
	bridge vni del vni 100 dev vx

vxlan_vni_delete_group() then calls __vxlan_fdb_delete() with the new
cfg->remote_ifindex, and vxlan_fdb_find_rdst() requires an exact match:

		if (vxlan_addr_equal(&rd->remote_ip, ip) &&
		    rd->remote_port == port &&
		    rd->remote_vni == vni &&
		    rd->remote_ifindex == ifindex)
			return rd;

so no rdst is found, __vxlan_fdb_delete() takes the "goto out; return 0" path
and the all-zeros-MAC entry survives the vni delete.  Re-adding the same vni
appends a second rdst, duplicating transmitted BUM traffic.  Meanwhile
vxlan_xmit_one() keeps using the stale "ifindex = rdst->remote_ifindex" as
route oif, and the vxlan_igmp_leave(vxlan, &vninode->remote_ip, 0) in the same
function resolves rifindex 0 to the new cfg->remote_ifindex although the group
was joined on the old one.

The IFLA_VXLAN_LINK = 0 variant looks newly broken: previously
default_dst.remote_ifindex was left untouched when lowerdev was NULL, so the
delete still matched.  Given the commit message says default fdb entries
"including VNI filter entries" are updated if remote_ifindex changed, should
this loop also handle nodes with their own group?

[ ... ]

[Severity: Low]
The commit message says:

    Furthermore, several fields of struct vxlan_rdst (remote_dev,
    remote_port, offloaded, list, rcu, dst_cache) in default_dst were
    completely unused.

Is that accurate for remote_dev?  In the baseline, default_dst.remote_dev was
the only user of that field: vxlan_dev_create() assigned it
("dst->remote_dev = remote_dev;"), vxlan_changelink() compared it
("if (dst->remote_dev == lowerdev) lowerdev = NULL;") and passed it to
netdev_adjacent_change_prepare()/commit()/abort(), and vxlan_dellink() used it
for netdev_upper_dev_unlink().  The patch moves that active role to
vxlan->lowerdev rather than removing dead state, and changes the surrounding
changelink logic along the way.