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

[Jakub Kicinski <kuba@kernel.org>]

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.

> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> index 338ecddb9542d..8b7558eac049d 100644
> --- a/drivers/net/vxlan/vxlan_core.c
> +++ b/drivers/net/vxlan/vxlan_core.c

[ ... ]

> @@ -4010,7 +4019,6 @@ static void vxlan_config_apply(struct net_device *dev,
>  			       bool changelink)
>  {
>  	struct vxlan_dev *vxlan = netdev_priv(dev);
> -	struct vxlan_rdst *dst = &vxlan->default_dst;
>  	unsigned short needed_headroom = ETH_HLEN;
>  	struct vxlan_config *old_cfg;
>  	int max_mtu = ETH_MAX_MTU;
> @@ -4028,13 +4036,7 @@ static void vxlan_config_apply(struct net_device *dev,
>  		vxlan->net = src_net;
>  	}
>  
> -	dst->remote_vni = new_cfg->vni;
> -
> -	memcpy(&dst->remote_ip, &new_cfg->remote_ip, sizeof(new_cfg->remote_ip));
> -
>  	if (lowerdev) {
> -		dst->remote_ifindex = new_cfg->remote_ifindex;
> -
>  		netif_inherit_tso_max(dev, lowerdev);
>  
>  		needed_headroom = lowerdev->hard_header_len;

[ ... ]

> @@ -4578,23 +4580,23 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
>  	if (!new_cfg)
>  		return -ENOMEM;
>  
> -	if (dst->remote_dev == lowerdev)
> -		lowerdev = NULL;
> -
> -	err = netdev_adjacent_change_prepare(dst->remote_dev, lowerdev, dev,
> -					     extack);
> -	if (err) {
> -		kfree(new_cfg);
> -		return err;
> +	lowerdev_changed = vxlan->lowerdev != lowerdev;
> +	if (lowerdev_changed) {
> +		err = netdev_adjacent_change_prepare(vxlan->lowerdev, lowerdev,
> +						     dev, extack);
> +		if (err) {
> +			kfree(new_cfg);
> +			return err;
> +		}
>  	}

[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?

> @@ -4607,19 +4609,20 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
>  					       NTF_SELF, 0, true, extack);
>  			if (err) {
>  				spin_unlock_bh(&vxlan->hash_lock);
> -				netdev_adjacent_change_abort(dst->remote_dev,
> -							     lowerdev, dev);
> +				if (lowerdev_changed)
> +					netdev_adjacent_change_abort(vxlan->lowerdev,
> +								     lowerdev, dev);
>  				kfree(new_cfg);
>  				return err;
>  			}
>  		}
> -		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);
>  
> @@ -4627,30 +4630,39 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
>  		 * all vnis that were using default remote ip
>  		 */
>  		if (cfg->flags & VXLAN_F_VNIFILTER) {
> -			err = vxlan_vnilist_update_group(vxlan, &dst->remote_ip,
> -							 &conf.remote_ip, extack);
> +			err = vxlan_vnilist_update_group(vxlan, &cfg->remote_ip,
> +							 &conf.remote_ip,
> +							 cfg->remote_ifindex,
> +							 conf.remote_ifindex,
> +							 extack);
>  			if (err) {
> -				netdev_adjacent_change_abort(dst->remote_dev,
> -							     lowerdev, dev);
> +				if (lowerdev_changed)
> +					netdev_adjacent_change_abort(vxlan->lowerdev,
> +								     lowerdev, dev);
>  				kfree(new_cfg);
>  				return err;
>  			}
>  		}
>  	}

[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);

[ ... ]

> diff --git a/drivers/net/vxlan/vxlan_vnifilter.c b/drivers/net/vxlan/vxlan_vnifilter.c
> index 6a2d0a69d7be1..55a07c20e7649 100644
> --- a/drivers/net/vxlan/vxlan_vnifilter.c
> +++ b/drivers/net/vxlan/vxlan_vnifilter.c
> @@ -487,24 +487,31 @@ static const struct nla_policy vni_filter_policy[VXLAN_VNIFILTER_MAX + 1] = {
>  };
>  
>  static int vxlan_update_default_fdb_entry(struct vxlan_dev *vxlan, __be32 vni,
> -					  union vxlan_addr *old_remote_ip,
> -					  union vxlan_addr *remote_ip,
> +					  const union vxlan_addr *old_remote_ip,
> +					  const union vxlan_addr *remote_ip,
> +					  u32 old_ifindex, u32 new_ifindex,
>  					  struct netlink_ext_ack *extack)
>  {
>  	const struct vxlan_config *cfg = rtnl_dereference(vxlan->cfg);
> -	struct vxlan_rdst *dst = &vxlan->default_dst;
>  	int err = 0;
>  
> +	if (old_remote_ip && remote_ip &&
> +	    vxlan_addr_equal(old_remote_ip, remote_ip) &&
> +	    old_ifindex == new_ifindex)
> +		return 0;
> +

[ ... ]

> @@ -563,6 +570,8 @@ static int vxlan_vni_update_group(struct vxlan_dev *vxlan,
>  
>  	ret = vxlan_update_default_fdb_entry(vxlan, vninode->vni,
>  					     oldrip, newrip,
> +					     cfg->remote_ifindex,
> +					     cfg->remote_ifindex,
>  					     extack);
>  	if (ret)
>  		goto out;

[ ... ]

> @@ -616,6 +626,8 @@ int vxlan_vnilist_update_group(struct vxlan_dev *vxlan,
>  			ret = vxlan_update_default_fdb_entry(vxlan, vent->vni,
>  							     old_remote_ip,
>  							     new_remote_ip,
> +							     old_ifindex,
> +							     new_ifindex,
>  							     extack);
>  			if (ret)
>  				return ret;

[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?

> @@ -630,20 +642,19 @@ static void vxlan_vni_delete_group(struct vxlan_dev *vxlan,
>  {
>  	struct vxlan_net *vn = net_generic(vxlan->net, vxlan_net_id);
>  	const struct vxlan_config *cfg = rtnl_dereference(vxlan->cfg);
> -	struct vxlan_rdst *dst = &vxlan->default_dst;
>  
>  	/* if per vni remote_ip not present, delete the
>  	 * default dst remote_ip previously added for this vni
>  	 */
>  	if (!vxlan_addr_any(&vninode->remote_ip) ||
> -	    !vxlan_addr_any(&dst->remote_ip)) {
> +	    !vxlan_addr_any(&cfg->remote_ip)) {
>  		spin_lock_bh(&vxlan->hash_lock);
>  		__vxlan_fdb_delete(vxlan, all_zeros_mac,
>  				   (vxlan_addr_any(&vninode->remote_ip) ?
> -				   dst->remote_ip : vninode->remote_ip),
> +				   cfg->remote_ip : vninode->remote_ip),
>  				   cfg->dst_port,
>  				   vninode->vni, vninode->vni,
> -				   dst->remote_ifindex,
> +				   cfg->remote_ifindex,
>  				   true);
>  		spin_unlock_bh(&vxlan->hash_lock);
>  	}

[ ... ]

> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
> index 7ced743ec8816..c3c9f2ccc3d1b 100644
> --- a/include/net/vxlan.h
> +++ b/include/net/vxlan.h
> @@ -204,7 +204,6 @@ struct vxlan_rdst {
>  	u8			 offloaded:1;
>  	__be32			 remote_vni;
>  	u32			 remote_ifindex;
> -	struct net_device	 *remote_dev;
>  	struct list_head	 list;
>  	struct rcu_head		 rcu;
>  	struct dst_cache	 dst_cache;

[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.

> @@ -295,7 +294,7 @@ struct vxlan_dev {
>  #endif
>  	struct net_device *dev;
>  	struct net	  *net;		/* netns for packet i/o */
> -	struct vxlan_rdst default_dst;	/* default destination */
> +	struct net_device *lowerdev;
>  
>  	struct timer_list age_timer;
>  	spinlock_t	  hash_lock;