Re: [PATCH net v5 1/2] vxlan: reject adding a remote to a nexthop fdb entry

[netdev-bot+sashiko@kernel.org]

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] The new fib_nexthops.sh cases never create a nexthop-backed
  00:00:00:00:00:00 entry.
- [Low] Baseline decision: false (resolved later in the series).
- [Low] The commit message says that "vxlan_fdb_notify(), RTM_GETNEIGH
  dumps and the switchdev FDB programming all prefer the remote list…
- [Low] The new "Fdb replace with nexthop group" test (fib_nexthops.sh
  lines 555-557 for IPv6 and 714-716 for IPv4) passes with or without…

--

vxlan: reject adding a remote to a nexthop fdb entry

This patch makes vxlan_fdb_update_existing() reject NLM_F_APPEND on a
multicast or all-zeros fdb entry that is backed by a nexthop group, and
does the check before any entry state is modified. It also moves the
non-unicast NLM_F_REPLACE rejection ahead of the state/flags update. New
fib_nexthops.sh cases are added for both.

> The entry state is also misreported: vxlan_fdb_notify(), RTM_GETNEIGH
> dumps and the switchdev FDB programming all prefer the remote list
> over the nexthop, so user space sees the dead remote while the
> datapath follows the nexthop group.

[Severity: Low]
Is this accurate for the netlink paths? vxlan_fdb_info() checks the
nexthop before the remote:

drivers/net/vxlan/vxlan_core.c:vxlan_fdb_info() {
    ...
	if (nh) {
		if (nla_put_u32(skb, NDA_NH_ID, nh_id))
			goto nla_put_failure;
	} else if (rdst) {
		if (send_ip && vxlan_nla_put_addr(skb, NDA_DST,
						  &rdst->remote_ip))
    ...
}

So the RTM_NEWNEIGH notification sent through
vxlan_fdb_notify()->__vxlan_fdb_notify() and the vxlan_fdb_get() reply
both carry NDA_NH_ID, not the stray remote. vxlan_fdb_dump() handles
f->nh entries before it walks the remote list, and it passes a NULL rdst.

As far as I can tell, only the switchdev notifier part of
vxlan_fdb_notify() (vxlan_fdb_switchdev_call_notifiers() with rd) uses
the stray remote. Could the commit message be narrowed to that path?

> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> index 278d41f58d895..3aead3c7f65f0 100644
> --- a/drivers/net/vxlan/vxlan_core.c
> +++ b/drivers/net/vxlan/vxlan_core.c
> @@ -1011,6 +1011,22 @@ static int vxlan_fdb_update_existing(struct vxlan_dev *vxlan,
>  		return -EOPNOTSUPP;
>  	}
>  
> +	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;
> +	}

[Severity: Low]
With only this patch applied, can this new rejection leave a VNI
half-installed on a vnifilter device? The commit message says "so a
refused request has no side effects". However, vxlan_vni_add() publishes
the vninode before it makes the call that now fails:

drivers/net/vxlan/vxlan_vnifilter.c:vxlan_vni_add() {
    ...
	__vxlan_vni_add_list(vxlan, vg, vninode);

	if (vxlan->dev->flags & IFF_UP)
		vxlan_vs_add_del_vninode(vxlan, vninode, false);

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

	vxlan_vnifilter_notify(vxlan, vninode, RTM_NEWTUNNEL);

	return err;
}

Suppose the per-VNI all-zeros entry is nexthop-backed, and either a group
is given or cfg->remote_ip is not any. Then the call chain is:

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

After that failure, vxlan_vni_update_group() jumps to out and skips both
the remote_ip copy and the IGMP join. vxlan_vni_add() still sends
RTM_NEWTUNNEL. It returns the error without removing the vninode from
vg->vni_hash or from the list.

The next patch in this series, "vxlan: reject VNI add when the default
fdb entry is nexthop-backed", adds a check in vxlan_vni_add() before
vxlan_vni_alloc(). That check rejects this case before anything is
installed, so the problem only exists at the bisect point between the two
patches. Would it make sense to reorder or fold the two patches so that
each one is self-consistent on its own?

[ ... ]

> diff --git a/tools/testing/selftests/net/fib_nexthops.sh b/tools/testing/selftests/net/fib_nexthops.sh
> index 431d7bed76224..bdc23205bde4c 100755
> --- a/tools/testing/selftests/net/fib_nexthops.sh
> +++ b/tools/testing/selftests/net/fib_nexthops.sh
> @@ -547,6 +547,15 @@ ipv6_fdb_grp_fcnal()
>  	run_cmd "$BRIDGE fdb append 02:02:00:00:00:17 dev vx10 dst 2001:db8:91::11 self dynamic"
>  	log_test $? 255 "Fdb mac append with nexthop group and dynamic state"
>  
> +	# fdb entries with a nexthop group cannot take a remote append
> +	run_cmd "$BRIDGE fdb add 00:00:00:00:00:00 dev vx10 nhid 102 self static"
> +	run_cmd "$BRIDGE fdb append 00:00:00:00:00:00 dev vx10 dst 2001:db8:91::11 self"
> +	log_test $? 255 "Fdb append with nexthop group"

[Severity: Medium]
Does this fdb add actually create a nexthop-backed all-zeros entry?
Earlier in ipv6_fdb_grp_fcnal(), vx10 is created with a remote:

	run_cmd "$IP link add name vx10 type vxlan id 1010 local 2001:db8:91::9 remote 2001:db8:91::10 dstport 4789 nolearning noudpcsum tos inherit ttl 100"

Because cfg->remote_ip is set, __vxlan_dev_create() has already installed
a plain, non-nexthop all_zeros_mac entry:

drivers/net/vxlan/vxlan_core.c:__vxlan_dev_create() {
    ...
	/* create an fdb entry for a valid default destination */
	if (!vxlan_addr_any(&cfg->remote_ip)) {
		union vxlan_addr rip = cfg->remote_ip;

		spin_lock_bh(&vxlan->hash_lock);
		err = vxlan_fdb_update(vxlan, all_zeros_mac,
				       &rip,
				       NUD_REACHABLE | NUD_PERMANENT,
				       NLM_F_EXCL | NLM_F_CREATE,
    ...
}

bridge fdb add sends NLM_F_CREATE | NLM_F_EXCL. vxlan_fdb_update() finds
the existing entry and returns -EEXIST:

	f = vxlan_find_mac(vxlan, mac, src_vni);
	if (f) {
		if (flags & NLM_F_EXCL) {
			netdev_dbg(vxlan->dev,
				   "lost race to create %pM\n", mac);
			return -EEXIST;
		}

Nothing checks the result of that run_cmd. The append that follows then
reaches vxlan_fdb_update_existing() with f->nh == NULL. The new guard does
not fire, vxlan_fdb_append() adds the remote, and bridge exits 0.

If so, log_test $? 255 would report FAIL on a fixed kernel, and the test
would never reach the new f->nh check. ipv4_fdb_grp_fcnal() has the same
problem, because vx10 is created there with remote 10.0.0.2.

Any of these would avoid the collision: delete the default entry first,
create vx10 without a remote, or use a multicast MAC for the
nexthop-backed entry.

> +
> +	# fdb entries with a nexthop group cannot be replaced with a non-unicast MAC
> +	run_cmd "$BRIDGE fdb replace 00:00:00:00:00:00 dev vx10 dst 2001:db8:91::11 self permanent"
> +	log_test $? 255 "Fdb replace with nexthop group"

[Severity: Low]
Can this test distinguish a fixed kernel from an unfixed one? The check
it hits is the moved "Cannot replace non-unicast fdb entries" rejection
in vxlan_fdb_update_existing(), and that check does not look at f->nh.

Before this patch the same request also returned -EOPNOTSUPP. The only
difference was that it did so after f->state and f->flags had been
rewritten. bridge exits with 255 in both cases.

To cover the half-update this patch fixes, the test could check in bridge
fdb show that state and flags are unchanged after the failed replace.

Because of the setup problem above, the entry being replaced is also the
plain default entry, not a nexthop-backed one.

The comment "fdb entries with a nexthop group cannot be replaced with a
non-unicast MAC" also looks inaccurate. The check is not specific to
nexthops, and the MAC is the lookup key, not the thing being replaced.
The copy in ipv4_fdb_grp_fcnal() has the same issues.

[ ... ]

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