Re: [PATCH net v5 2/2] vxlan: reject VNI add when the default fdb entry is nexthop-backed

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The series order leaves a bisect window, and nothing protects the
  stable backport.
- [Low] The commit message gets the order of operations in
  vxlan_vni_add() wrong.

--

vxlan: reject VNI add when the default fdb entry is nexthop-backed

This commit adds vxlan_fdb_entry_nh_backed() and calls it from
vxlan_vni_add(). A VNI add with a group, or with a device default remote,
is now rejected when the all-zeros fdb entry for that VNI is backed by a
nexthop. The check runs before the VNI is installed, so a later
-EOPNOTSUPP from vxlan_fdb_update_existing() can no longer leave the VNI
half-installed.

> vxlan_vni_add() installs the new VNI - hash table insert, group list,
> socket attach and the RTM_NEWTUNNEL notification - before calling
> vxlan_vni_update_group(), which in turn calls
> vxlan_update_default_fdb_entry() to store the group (or the device's
> default remote) into the all-zeros fdb entry of that VNI.

[Severity: Low]
Does this ordering match vxlan_vni_add()? In the code, the notification
goes out after vxlan_vni_update_group() returns, and it goes out whatever
the return value is:

	err = vxlan_vni_update_group(vxlan, vninode, group, true, &changed,
				     extack);

	vxlan_vnifilter_notify(vxlan, vninode, RTM_NEWTUNNEL);

	return err;

The end state described later in the message is accurate: the VNI is
installed and announced despite the error. But it is announced after the
failed update, not before it.

Could this paragraph be reworded to match?

> diff --git a/drivers/net/vxlan/vxlan_vnifilter.c b/drivers/net/vxlan/vxlan_vnifilter.c
> index 124ba47758e91..07e17f4b93530 100644
> --- a/drivers/net/vxlan/vxlan_vnifilter.c
> +++ b/drivers/net/vxlan/vxlan_vnifilter.c
> @@ -837,6 +837,20 @@ static int vxlan_vni_add(struct vxlan_dev *vxlan,
>  		return err;
>  	}
>  
> +	/* A default fdb entry that is backed by a nexthop group can
> +	 * never take a remote; reject before the VNI is installed,
> +	 * otherwise a failure in vxlan_vni_update_group() would leave
> +	 * the VNI half-installed.
> +	 */
> +	if ((group && !vxlan_addr_any(group)) ||
> +	    !vxlan_addr_any(&cfg->remote_ip)) {
> +		if (vxlan_fdb_entry_nh_backed(vxlan, all_zeros_mac, v)) {
> +			NL_SET_ERR_MSG(extack,
> +				       "Cannot add a remote to a nexthop fdb entry");
> +			return -EOPNOTSUPP;
> +		}
> +	}

[Severity: Low]
Does the series order leave a bisect window between this patch and the
earlier "vxlan: reject adding a remote to a nexthop fdb entry"?

That earlier patch adds this check to vxlan_fdb_update_existing():

	if (rcu_access_pointer(f->nh) &&
	    (flags & NLM_F_APPEND) &&
	    (is_multicast_ether_addr(f->key.eth_addr) ||
	     is_zero_ether_addr(f->key.eth_addr))) {
		NL_SET_ERR_MSG(extack,
			       "Cannot add a remote to a nexthop fdb entry");
		return -EOPNOTSUPP;
	}

With only that patch applied, a sequence like this:

  bridge fdb add 00:00:00:00:00:00 dev vx0 nhid X self permanent src_vni N
  bridge vni add dev vx0 vni N group G

goes through this path:

vxlan_vni_add()
  vxlan_vni_update_group()
    vxlan_update_default_fdb_entry()
      vxlan_fdb_update()
        vxlan_fdb_update_existing() returns -EOPNOTSUPP

By then the VNI is already in the hash table and the group list, and
RTM_NEWTUNNEL is still sent. That is the half-installed state this patch
is meant to prevent. Leaving out the group has the same effect when the
device has a default remote. Before the earlier patch, the same request
succeeded and appended a stray remote.

Both patches carry Cc: stable and the same Fixes: tag, but neither one
records the dependency.

The earlier patch only touches vxlan_fdb_update_existing(), so it will
probably apply cleanly to older trees. This patch was reworked in v5 for
the RCU conversion of vxlan->cfg (the local cfg from
rtnl_dereference(vxlan->cfg)), so its context is unlikely to apply there.

Could stable end up taking the earlier patch on its own? This guard does
not depend on the earlier patch, so would it make sense to put it first
in the series, or to squash the two?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028111626.3460759-1-bsdhenrymartin%40gmail.com