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

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

Critical: 0 · High: 0 · Medium: 2 · Low: 3

- [Medium] vxlan_changelink() bundles two undisclosed/untagged
  behavioural repairs into the default_dst refactor.
- [Medium] vxlan_changelink() now keys the default-FDB delete on
  cfg->remote_ifindex, which newly breaks the 'lower device detached,…
- [Low] Unrelated whitespace churn/style regression in
  vxlan_group_used(): the two socket-comparison continuation lines are…
- [Low] Undisclosed functional fix on the drop path of
  vxlan_encap_bypass().
- [Low] `struct vxlan_rdst::remote_dev` (include/net/vxlan.h:207) becomes
  dead after this patch: a tree-wide grep for…

--

vxlan: remove default_dst and use vxlan_config and lowerdev

This removes vxlan->default_dst and reads the default destination
attributes (remote_ip, remote_vni, remote_ifindex) from the RCU protected
vxlan->cfg instead.  A new vxlan->lowerdev pointer, maintained under RTNL,
takes over the upper/lower netdev adjacency tracking that
default_dst.remote_dev used to do.

[ ... ]

[ ... ]

[Severity: Low]
Should this if (dst_cfg) guard live in the previous patch of the series
instead?

In the parent commit ("vxlan: convert configuration to RCU protection") the
goto drop taken when rcu_dereference(dst_vxlan->cfg) returns NULL lands on
the drop: label which calls vxlan_vnifilter_count() unconditionally, and
that helper dereferences cfg on its very first statement:

drivers/net/vxlan/vxlan_vnifilter.c:vxlan_vnifilter_count() {
	if (!(cfg->flags & VXLAN_F_VNIFILTER))
		return;

So the parent commit has a NULL dereference on the same path (reachable
when vxlan_free_dev() has already done RCU_INIT_POINTER(vxlan->cfg, NULL)
while a local-bypass transmit is in flight), and this patch quietly repairs
it while only describing the default_dst removal.  Would it make sense to
move the guard into the commit that opened the NULL window so bisection
does not land on a broken intermediate commit?

[ ... ]

[Severity: Medium]
Can this change the MTU of an existing device as a side effect of an
unrelated changelink?

Before this patch, vxlan_changelink() set lowerdev to NULL whenever the
lower device was unchanged, so this if (lowerdev) block never ran on a
plain attribute update.  Now the real lowerdev is always passed in, so
netif_inherit_tso_max(), the needed_headroom/needed_tailroom recomputation
and the following clamp all run on every changelink:

	max_mtu = lowerdev->mtu - vxlan_headroom(flags);
	...
	if (dev->mtu > max_mtu)
		dev->mtu = max_mtu;

Concretely: create a vxlan over a 9000 byte lower device (vxlan mtu 8950),
then shrink the lower device to 1500.  Today the vxlan mtu stays 8950;
after this patch any unrelated changelink (for instance changing only ttl
or tos) silently drops dev->mtu to 1450.

The commit message mentions only "avoid clobbering lowerdev to NULL when
unchanged" - could it spell out this user visible consequence?

[ ... ]

[Severity: Low]
This was the only user of struct vxlan_rdst::remote_dev, so should that
field be dropped from include/net/vxlan.h in the same patch?

After this hunk a grep for remote_dev in drivers/net/vxlan/ finds only the
local variable in vxlan_dev_create().  FDB remotes are allocated with
kmalloc_obj() in vxlan_fdb_append() and nothing assigns remote_dev, so
every FDB remote now carries an uninitialized struct net_device * inside an
exported struct.

The commit message enumerates remote_port, offloaded, list, rcu and
dst_cache as "completely unused" - those are all still used by FDB remotes,
while remote_dev, the one field that does become dead, is kept.

[ ... ]

[ ... ]

-		if (!vxlan_addr_any(&dst->remote_ip))
+		if (!vxlan_addr_any(&cfg->remote_ip))
 			__vxlan_fdb_delete(vxlan, all_zeros_mac,
-					   dst->remote_ip,
+					   cfg->remote_ip,
 					   cfg->dst_port,
-					   dst->remote_vni,
-					   dst->remote_vni,
-					   dst->remote_ifindex,
+					   cfg->vni,
+					   cfg->vni,
+					   cfg->remote_ifindex,
 					   true);
 		spin_unlock_bh(&vxlan->hash_lock);

[Severity: Medium]
Does keying this delete on cfg->remote_ifindex leave a stale default FDB
remote behind after the lower device has been detached?

Sequence, all via changelink:

  ip link add vxlan0 type vxlan id 10 dev eth0 remote 10.0.0.1
     vxlan_dev_create() creates the all-zeros-MAC remote with
     rdst->remote_ifindex = ifindex(eth0)

  ip link set vxlan0 type vxlan nolink        (IFLA_VXLAN_LINK = 0)
     rem_ip_changed is false, so no FDB refresh happens, but the newly
     published cfg->remote_ifindex is now 0

  ip link set vxlan0 type vxlan remote 10.0.0.2
     rem_ip_changed is true, vxlan_fdb_update() with NLM_F_APPEND appends a
     second rdst, then the __vxlan_fdb_delete() above runs with
     cfg->remote_ifindex == 0

vxlan_fdb_find_rdst() requires an exact ifindex match:

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

so rd is NULL, __vxlan_fdb_delete() jumps to out: and returns 0 with no
diagnostic, and the old remote 10.0.0.1 with ifindex(eth0) stays appended.
vxlan_xmit_one() picks the egress interface from rdst->remote_ifindex, so
BUM traffic keeps being replicated to the obsolete remote over the old
lower device.

In the parent commit default_dst.remote_ifindex was only written inside
if (lowerdev) in vxlan_config_apply(), so it still held ifindex(eth0) at
the third step and the delete matched.  Is this detach case expected to
keep working?

[ ... ]

[Severity: Medium]
Is this new netdev_upper_dev_unlink() call a fix that deserves its own
patch with a Fixes: tag?

Previously, an IFLA_VXLAN_LINK = 0 changelink ended up calling
netdev_adjacent_change_prepare() and netdev_adjacent_change_commit() with
new_dev == NULL, and both of those return immediately in that case, so the
upper/lower adjacency was left in place and default_dst.remote_ifindex
stayed stale.  That means vxlan_fill_info() kept reporting the removed
IFLA_VXLAN_LINK, and a later unregister of the no longer referenced lower
device still tore the vxlan device down via
vxlan_handle_lowerdev_unregister().

This hunk repairs both, but it is bundled into a refactor with no Fixes:
tag, so it cannot be backported on its own.  Would splitting the adjacency
repair out as a separate fix be preferable?

[ ... ]

[Severity: Low]
This isn't a bug, but these two continuation lines are re-indented away
from the open parenthesis alignment (checkpatch: "Alignment should match
open parenthesis") and now read like nested statements.  Neither line has
anything to do with removing default_dst - was this intentional?

[Severity: Low]
With default_dst gone, struct vxlan_rdst::remote_dev in this same header no
longer has any writer or reader.  Should it be removed here too?

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