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

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

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

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] vxlan_config_apply() unconditionally stores
  dev->needed_tailroom = 0 (vxlan_core.c:4046) and only afterwards,…
- [Low] The patch subject advertises a pure data-structure cleanup
  ("remove default_dst and use vxlan_config and lowerdev") but it also…
- [Low] vxlan_config_apply() now explicitly resets dev->needed_tailroom =
  0 before the `if (lowerdev)` block (and re-derives needed_headroom…
- [Low] Two newly added declarations violate the netdev reverse-xmas-tree
  local variable ordering rule: in vxlan_fdb_add()…

--

vxlan: remove default_dst and use vxlan_config and lowerdev

This drops vxlan->default_dst, replacing it with a plain
struct net_device *lowerdev pointer in struct vxlan_dev and moving all
remaining readers of remote_ip / remote_vni / remote_ifindex over to
vxlan->cfg.  vxlan_config_apply() gains a lowerdev_changed argument, and
vxlan_changelink() now only touches the upper/lower adjacency when the
lower device actually changes.

A few questions below.

[ ... ]

[Severity: Low]
This isn't a bug, but does this break the reverse xmas tree ordering netdev
asks for?  The new declaration

	const struct vxlan_config *cfg;

is shorter than the following

	struct vxlan_dev *vxlan = netdev_priv(dev);

The same inversion appears in vxlan_mdb_get_parse() in
drivers/net/vxlan/vxlan_mdb.c, where const struct vxlan_config *cfg; is
inserted ahead of struct vxlan_dev *vxlan = netdev_priv(dev);.  The other
hunks in this patch (vxlan_changelink(), vxlan_change_mtu(),
vxlan_get_link_ksettings()) do keep the ordering.

[ ... ]

[Severity: Low]
Should these behaviour changes be split out of the cleanup, with Fixes:
tags?  The subject describes a data structure cleanup, but three separate
user-visible changes ride along:

First, the new -EINVAL above rejects a request that previously succeeded as
a no-op: clearing IFLA_VXLAN_LINK on a VNIFILTER device that has per-VNI
multicast groups.  That is a netlink policy change.

Second, the commit message says:

    In vxlan_changelink(), pass lowerdev to vxlan_config_apply() to preserve
    needed_headroom and needed_tailroom.

This looks like a fix for a real bug.  Before the patch vxlan_changelink()
did:

	if (dst->remote_dev == lowerdev)
		lowerdev = NULL;
	...
	vxlan_config_apply(dev, new_cfg, lowerdev, vxlan->net, true);

and vxlan_config_apply() only adds the lower device contributions inside
if (lowerdev), so any changelink that did not touch IFLA_VXLAN_LINK (for
example ageing or learning) recomputed dev->needed_headroom as bare
ETH_HLEN + vxlan_headroom(flags), losing the lower device's
hard_header_len and needed_headroom.

Third, the commit message says:

    Setting IFLA_VXLAN_LINK to 0 now also tears down the upper/lower
    adjacency, which netdev_adjacent_change_commit() used to skip for a NULL
    new device

which is a second fix, since netdev_adjacent_change_commit() early-returns:

net/core/dev.c:netdev_adjacent_change_commit() {
	if (!new_dev || !old_dev)
		return;
	...
}

Would it be reasonable to split the two repairs into their own patches with
Fixes: tags so they can be picked up for stable independently of the RCU
cfg conversion this patch depends on?

[Severity: Medium]
Does this open a window where lockless readers observe
dev->needed_tailroom == 0?

The field is zeroed unconditionally here and only restored a few lines
later inside if (lowerdev):

	dev->needed_tailroom = 0;
	if (lowerdev) {
		...
		dev->needed_tailroom = lowerdev->needed_tailroom;

Since vxlan_changelink() now always passes the (possibly unchanged)
lowerdev, this zero/restore sequence runs on every changelink of a live
device, including ones that only change ageing, learning or proxy.  Before
the patch the field was left alone in that case because lowerdev had been
nulled.

The readers of dev->needed_tailroom hold no lock at all, for example:

net/core/skbuff.c:skb_ensure_writable_head_tail() {
	int needed_tailroom = dev->needed_tailroom;
	...
}

and arp_create(), igmp_send_report(), ndisc_alloc_skb(), af_packet
sendmsg/tpacket and xsk do the same.  With a lower device that has non-zero
needed_tailroom (DSA tag_trailer/tag_ksz/tag_sja1105, xfrm interface,
mac802154) a reader sampling 0 ends up allocating without the required
tailroom and pays a pskb_expand_head() later.

Both stores are also plain, while the neighbouring dev->mtu stores in the
same function are deliberately converted to WRITE_ONCE() by this patch.
Would computing the value into a local and doing a single
WRITE_ONCE(dev->needed_tailroom, val) match the treatment given to
dev->mtu here?

[Severity: Low]
Should netif_inherit_tso_max() be undone as well when the lower device goes
away?

dev->needed_tailroom is now explicitly reset to 0 and needed_headroom is
re-derived from the local ETH_HLEN initializer, so clearing
IFLA_VXLAN_LINK drops those two lower device contributions.  The third
attribute inherited in the same if (lowerdev) block,

		netif_inherit_tso_max(dev, lowerdev);

has no counterpart reset.

After a changelink that removes the lower device (a state this patch makes
reachable, since vxlan_changelink() now unlinks and passes
lowerdev == NULL), does the vxlan device keep advertising the removed lower
device's tso_max_size/tso_max_segs, along with any gso_max_size /
gso_ipv4_max_size reduction netif_set_tso_max_size() applied?  A later
changelink without a link takes the same NULL branch, so nothing appears to
recompute those values.

[ ... ]

[ ... ]

Thanks.

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