Re: [PATCH v5 net-next 1/8] vxlan: update default fdb entries when the lower device changes

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (1) · Medium: 1 · Low: 0

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

- [High] vxlan_changelink() (drivers/net/vxlan/vxlan_core.c:4538-4554)
  now routes every VXLAN_F_VNIFILTER device through…
- [Medium] Both rollback paths added by this patch ignore the int return
  value of vxlan_update_default_fdb_entry(): the err_unwind loop in…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it directly
  undermines the bug class the patch targets.

--

vxlan: update default fdb entries when the lower device changes

This refreshes the all zeros mac fdb entries when a changelink only swaps
the lower device, by passing the old and new ifindex down to
vxlan_update_default_fdb_entry() and visiting every vni rather than only
the ones inheriting the device group.  It also runs the multicast
leave/join pair for VXLAN_F_VNIFILTER devices and unwinds already updated
fdb entries when the vni list update fails partway through.

> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> index 347245cc1de4e..c4e3e8e8eef57 100644
> --- a/drivers/net/vxlan/vxlan_core.c
> +++ b/drivers/net/vxlan/vxlan_core.c

[ ... ]

> @@ -4475,13 +4476,16 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
>  	if (err)
>  		return err;
>  
> +	/* vxlan_config_apply() only commits remote_ifindex if lowerdev is set */
> +	new_ifindex = lowerdev ? conf.remote_ifindex : dst->remote_ifindex;
> +
>  	rem_ip_changed = !vxlan_addr_equal(&conf.remote_ip, &dst->remote_ip);
>  	change_igmp = vxlan->dev->flags & IFF_UP &&
>  		      (rem_ip_changed ||
> -		       dst->remote_ifindex != conf.remote_ifindex);
> +		       dst->remote_ifindex != new_ifindex);
>  
>  	/* handle default dst entry */
> -	if (rem_ip_changed) {
> +	if (rem_ip_changed || dst->remote_ifindex != new_ifindex) {

[ ... ]

> @@ -4509,13 +4513,21 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
>  					   true);
>  		spin_unlock_bh(&vxlan->hash_lock);
>  
> -		/* If vni filtering device, also update fdb entries of
> -		 * all vnis that were using default remote ip
> +		/* If vni filtering device, also update default fdb entries of
> +		 * all vnis
>  		 */
>  		if (vxlan->cfg.flags & VXLAN_F_VNIFILTER) {
>  			err = vxlan_vnilist_update_group(vxlan, &dst->remote_ip,
> -							 &conf.remote_ip, extack);
> +							 &conf.remote_ip,
> +							 dst->remote_ifindex,
> +							 new_ifindex, extack);
>  			if (err) {
> +				vxlan_update_default_fdb_entry(vxlan, conf.vni,
> +							       &conf.remote_ip,
> +							       &dst->remote_ip,
> +							       new_ifindex,
> +							       dst->remote_ifindex,
> +							       NULL);
>  				netdev_adjacent_change_abort(dst->remote_dev,
>  							     lowerdev, dev);
>  				return err;
> @@ -4523,7 +4535,9 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
>  		}
>  	}
>  
> -	if (change_igmp && vxlan_addr_multicast(&dst->remote_ip))
> +	if (change_igmp &&
> +	    (vxlan_addr_multicast(&dst->remote_ip) ||
> +	     (vxlan->cfg.flags & VXLAN_F_VNIFILTER)))
>  		err = vxlan_multicast_leave(vxlan);

[Severity: High]
Can this leave/join pair end up leaving a running vnifilter device with no
multicast memberships at all?

For VXLAN_F_VNIFILTER the leave reaches
vxlan_multicast_leave() -> vxlan_multicast_leave_vnigrp(), which issues one
vxlan_igmp_leave() per vni entry:

	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;
		}
	}

A single "bridge vni add dev vxlan0 vni 100-200 group 239.1.1.1" gives every
vni in the range the same group, so all of those entries share one socket
membership.  The first vxlan_igmp_leave() drops it, and the remaining ones
return -EADDRNOTAVAIL (ip_mc_leave_group() starts with
int ret = -EADDRNOTAVAIL, and ipv6_sock_mc_drop() behaves the same), which
leave_vnigrp() records in last_err and returns.

vxlan_group_used() cannot absorb the duplicate here either: it skips
vxlan == dev and returns false early when refcount_read(&sock4->refcnt) == 1.

Note the asymmetry with the join side.  vxlan_multicast_join_vnigrp() both
skips entries whose address equals the device default remote_ip and maps
-EADDRINUSE to 0, while vxlan_multicast_leave_vnigrp() does neither.

With err set to -EADDRNOTAVAIL, netdev_adjacent_change_commit() and
vxlan_config_apply() still commit the new lower device, and then:

	if (!err && change_igmp &&
	    (vxlan_addr_multicast(&dst->remote_ip) ||
	     (vxlan->cfg.flags & VXLAN_F_VNIFILTER)))
		err = vxlan_multicast_join(vxlan);

skips the re-join entirely.  Repeating the same changelink does not repair
it, because lowerdev == dst->remote_dev makes lowerdev NULL, so
new_ifindex == dst->remote_ifindex and change_igmp becomes false.

Reproducer shape:

	ip link add vxlan0 type vxlan external vnifilter dstport 4789 \
		local 10.0.0.1 dev eth0
	bridge vni add dev vxlan0 vni 100-200 group 239.1.1.1
	ip link set vxlan0 up
	ip link set dev vxlan0 type vxlan ... dev eth1

Would it be better to rejoin unconditionally here, and/or make the per-vni
teardown act once per unique (group, ifindex) membership and tolerate
-EADDRNOTAVAIL the way the join side tolerates -EADDRINUSE?

>  
>  	if (netif_running(dev) && conf.age_interval != vxlan->cfg.age_interval)
> @@ -4535,7 +4549,8 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
>  	vxlan_config_apply(dev, &conf, lowerdev, vxlan->net, true);
>  
>  	if (!err && change_igmp &&
> -	    vxlan_addr_multicast(&dst->remote_ip))
> +	    (vxlan_addr_multicast(&dst->remote_ip) ||
> +	     (vxlan->cfg.flags & VXLAN_F_VNIFILTER)))
>  		err = vxlan_multicast_join(vxlan);
>  
>  	return err;

[ ... ]

> diff --git a/drivers/net/vxlan/vxlan_vnifilter.c b/drivers/net/vxlan/vxlan_vnifilter.c
> index dd94085e08865..12fa11a318184 100644
> --- a/drivers/net/vxlan/vxlan_vnifilter.c
> +++ b/drivers/net/vxlan/vxlan_vnifilter.c
> @@ -470,14 +470,19 @@ static const struct nla_policy vni_filter_policy[VXLAN_VNIFILTER_MAX + 1] = {
>  	[VXLAN_VNIFILTER_ENTRY] = { .type = NLA_NESTED },
>  };
>  
> -static int vxlan_update_default_fdb_entry(struct vxlan_dev *vxlan, __be32 vni,
> -					  union vxlan_addr *old_remote_ip,
> -					  union vxlan_addr *remote_ip,
> -					  struct netlink_ext_ack *extack)
> +int vxlan_update_default_fdb_entry(struct vxlan_dev *vxlan, __be32 vni,
> +				   union vxlan_addr *old_remote_ip,
> +				   union vxlan_addr *remote_ip,
> +				   u32 old_ifindex, u32 new_ifindex,
> +				   struct netlink_ext_ack *extack)
>  {
> -	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;
> +
>  	spin_lock_bh(&vxlan->hash_lock);
>  	if (remote_ip && !vxlan_addr_any(remote_ip)) {
>  		err = vxlan_fdb_update(vxlan, all_zeros_mac,
> @@ -487,7 +492,7 @@ static int vxlan_update_default_fdb_entry(struct vxlan_dev *vxlan, __be32 vni,
>  				       vxlan->cfg.dst_port,
>  				       vni,
>  				       vni,
> -				       dst->remote_ifindex,
> +				       new_ifindex,
>  				       NTF_SELF, 0, true, extack);
>  		if (err) {
>  			spin_unlock_bh(&vxlan->hash_lock);
> @@ -500,7 +505,7 @@ static int vxlan_update_default_fdb_entry(struct vxlan_dev *vxlan, __be32 vni,
>  				   *old_remote_ip,
>  				   vxlan->cfg.dst_port,
>  				   vni, vni,
> -				   dst->remote_ifindex,
> +				   old_ifindex,
>  				   true);
>  	}
>  	spin_unlock_bh(&vxlan->hash_lock);
> @@ -546,6 +551,8 @@ static int vxlan_vni_update_group(struct vxlan_dev *vxlan,
>  
>  	ret = vxlan_update_default_fdb_entry(vxlan, vninode->vni,
>  					     oldrip, newrip,
> +					     dst->remote_ifindex,
> +					     dst->remote_ifindex,
>  					     extack);
>  	if (ret)
>  		goto out;

[Severity: High]
This isn't a bug introduced by this patch, but the oldrip/newrip selection
just above this call makes the new equality guard dead for this caller, and
it also feeds the bug class the changelog describes.

In vxlan_vni_update_group() the two pointers are mutually exclusive:

	if (group && !vxlan_addr_any(group)) {
		newrip = group;
	} else {
		if (!vxlan_addr_any(&dst->remote_ip))
			newrip = &dst->remote_ip;
	}

	/* if old rip exists, and no newrip,
	 * explicitly delete old rip
	 */
	if (!newrip && !vxlan_addr_any(&old_remote_ip))
		oldrip = &old_remote_ip;

So when a vni's group is changed:

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

vxlan_vni_add() -> vxlan_vni_update() -> vxlan_vni_update_group(create=false)
passes newrip = 239.1.1.2 with oldrip still NULL, and in
vxlan_update_default_fdb_entry() the delete side is skipped:

	if (old_remote_ip && !vxlan_addr_any(old_remote_ip)) {
		__vxlan_fdb_delete(vxlan, all_zeros_mac,
				   *old_remote_ip,

The 239.1.1.1 rdst of the (all_zeros_mac, vni) entry therefore survives, and
vxlan_vni_delete_group() only deletes vninode->remote_ip, so it can never be
removed and each further group update appends one more rdst.

The new migration loop below only re-keys vent->remote_ip:

		} else {
			oldrip = &vent->remote_ip;
			newrip = &vent->remote_ip;
		}

so after a lower device change that stale rdst additionally keeps pointing
at the old ifindex.

Both the pre-existing "!create && oldrip && newrip &&
vxlan_addr_equal(oldrip, newrip)" test and the newly added
"old_remote_ip && remote_ip && vxlan_addr_equal(...) && old_ifindex ==
new_ifindex" test are unreachable for this caller, which suggests oldrip was
meant to be set on a group change.  Should vxlan_vni_update_group() set
oldrip = &old_remote_ip whenever it differs from newrip?

Verified via git blame as unchanged since f9c4bb0b245ce ("vxlan: vni
filtering support on collect metadata device") and still present at the end
of this series.

> @@ -583,8 +590,10 @@ static int vxlan_vni_update_group(struct vxlan_dev *vxlan,
>  int vxlan_vnilist_update_group(struct vxlan_dev *vxlan,
>  			       union vxlan_addr *old_remote_ip,
>  			       union vxlan_addr *new_remote_ip,
> +			       u32 old_ifindex, u32 new_ifindex,
>  			       struct netlink_ext_ack *extack)
>  {
> +	union vxlan_addr *oldrip, *newrip;
>  	struct list_head *headp, *hpos;
>  	struct vxlan_vni_group *vg;
>  	struct vxlan_vni_node *vent;
> @@ -595,17 +604,46 @@ int vxlan_vnilist_update_group(struct vxlan_dev *vxlan,
>  	headp = &vg->vni_list;
>  	list_for_each_prev(hpos, headp) {
>  		vent = list_entry(hpos, struct vxlan_vni_node, vlist);
> +
>  		if (vxlan_addr_any(&vent->remote_ip)) {
> -			ret = vxlan_update_default_fdb_entry(vxlan, vent->vni,
> -							     old_remote_ip,
> -							     new_remote_ip,
> -							     extack);
> -			if (ret)
> -				return ret;
> +			oldrip = old_remote_ip;
> +			newrip = new_remote_ip;
> +		} else {
> +			/* A vni with its own group keeps it, but its fdb entry
> +			 * is still keyed on the device remote_ifindex.
> +			 */
> +			oldrip = &vent->remote_ip;
> +			newrip = &vent->remote_ip;
>  		}
> +
> +		ret = vxlan_update_default_fdb_entry(vxlan, vent->vni,
> +						     oldrip, newrip,
> +						     old_ifindex, new_ifindex,
> +						     extack);
> +		if (ret)
> +			goto err_unwind;
>  	}
>  
>  	return 0;
> +
> +err_unwind:
> +	list_for_each_continue(hpos, headp) {
> +		vent = list_entry(hpos, struct vxlan_vni_node, vlist);
> +
> +		if (vxlan_addr_any(&vent->remote_ip)) {
> +			oldrip = old_remote_ip;
> +			newrip = new_remote_ip;
> +		} else {
> +			oldrip = &vent->remote_ip;
> +			newrip = &vent->remote_ip;
> +		}
> +
> +		vxlan_update_default_fdb_entry(vxlan, vent->vni,
> +					       newrip, oldrip,
> +					       new_ifindex, old_ifindex,
> +					       NULL);
> +	}
> +	return ret;
>  }

[Severity: Medium]
Does discarding the return value here still guarantee the changelog's claim
that "default_dst and the fdb entries do not diverge"?

vxlan_update_default_fdb_entry() is ordered add-then-delete, and it returns
early if the add fails:

		if (err) {
			spin_unlock_bh(&vxlan->hash_lock);
			return err;
		}

The re-add performed by the unwind is fallible.  It goes
vxlan_fdb_update() -> vxlan_fdb_update_existing() -> vxlan_fdb_append(),
which does:

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

and vxlan_fdb_notify() can also fail through
vxlan_fdb_switchdev_call_notifiers() on an offloading device.

When that happens the new-ifindex rdst stays installed, __vxlan_fdb_delete()
is never reached, and vxlan_changelink() returns the error without calling
vxlan_config_apply(), so default_dst.remote_ifindex keeps the old value.
Since vxlan_vni_delete_group() looks the entry up with the current (old)
dst->remote_ifindex and vxlan_fdb_find_rdst() requires an exact ifindex
match, that surviving rdst can no longer be deleted, and the next
"bridge vni add" of the same vni appends another one, which is the duplicated
BUM traffic this patch sets out to fix.

The same discarded return exists for the device default entry in
vxlan_changelink(), where the revert call is:

				vxlan_update_default_fdb_entry(vxlan, conf.vni,
							       &conf.remote_ip,
							       &dst->remote_ip,
							       new_ifindex,
							       dst->remote_ifindex,
							       NULL);

Would it make sense to attempt the delete even when the re-add fails, or to
restructure so the rollback needs no allocation, for example by keeping the
original destinations installed until every new destination has been added
successfully?

>  
>  static void vxlan_vni_delete_group(struct vxlan_dev *vxlan,

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