Re: [PATCH net] vxlan: annotate lockless accesses to rdst->remote_ifindex

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · 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] Rolling back a failed FDB replace still writes
  rdst->remote_ifindex with a plain store, so the patch misses one of…

--

vxlan: annotate lockless accesses to rdst->remote_ifindex

The patch adds READ_ONCE() to the lockless RCU readers of
rdst->remote_ifindex in vxlan_fdb_info(),
vxlan_fdb_switchdev_notifier_info(), vxlan_snoop() and vxlan_xmit_one().
It also changes the in-place update in vxlan_fdb_replace() to use
WRITE_ONCE().

> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> index d11028b374721..13bfb1bb11b7b 100644
> --- a/drivers/net/vxlan/vxlan_core.c
> +++ b/drivers/net/vxlan/vxlan_core.c

[ ... ]

> @@ -569,7 +572,7 @@ static int vxlan_fdb_replace(struct vxlan_fdb *f,
>  	rd->remote_ip = *ip;
>  	rd->remote_port = port;
>  	rd->remote_vni = vni;
> -	rd->remote_ifindex = ifindex;
> +	WRITE_ONCE(rd->remote_ifindex, ifindex);
>  	rd->offloaded = false;
>  	return 1;
>  }

[Severity: High]
This annotates the forward write in vxlan_fdb_replace(). Is the rollback
of the same write in vxlan_fdb_update_existing() still a plain store?

drivers/net/vxlan/vxlan_core.c:vxlan_fdb_update_existing() {
    ...
		err = vxlan_fdb_notify(vxlan, f, rd, RTM_NEWNEIGH,
				       swdev_notify, extack);
		if (err)
			goto err_notify;
    ...
err_notify:
	if (nhid)
		return err;
	if ((flags & NLM_F_REPLACE) && rc)
		*rd = oldrd;
    ...
}

When vxlan_fdb_notify() fails after vxlan_fdb_replace() has run, the
struct copy rewrites rd->remote_ifindex on an rdst that is still linked
on f->remotes. Only vxlan->hash_lock is held. vxlan_xmit_one(),
vxlan_snoop() and the RCU dump paths can be reading the field with the
new READ_ONCE() at the same time.

This path can be reached in tree. mlxsw_sp_switchdev_vxlan_work_prepare()
returns -EOPNOTSUPP for a non-zero remote_ifindex ("Local interface is
not supported"), a non-default port or VNI, or a multicast destination.
It also fails if its GFP_ATOMIC allocation fails. The result is
NOTIFY_BAD, and vxlan_fdb_notify() then returns an error. So a
"bridge fdb replace ... via <dev>" on an mlxsw-offloaded VXLAN goes
through WRITE_ONCE() and then a plain-store restore while traffic is
flowing.

Would it make sense to restore the fields one at a time in err_notify,
using WRITE_ONCE(rd->remote_ifindex, oldrd.remote_ifindex)?

A related point about the same struct copy: it also copies the old
rd->dst_cache back, including reset_ts from before the
dst_cache_reset() in vxlan_fdb_replace(). Could a dst that TX cached
for the rejected destination during that window count as valid again
once the old destination is restored? Calling dst_cache_reset() again
in the rollback, instead of copying dst_cache back, would avoid that.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110072208.3273638-1-edumazet%40kernel.org