Re: [PATCH net] vxlan: reset the remote's dst cache when snooping moves a MAC

[netdev-bot+sashiko@kernel.org]

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

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

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] In vxlan_snoop() (drivers/net/vxlan/vxlan_core.c), the patch
  adds only the dst_cache_reset() from vxlan_fdb_replace(), even though…
- [Low] The new dst_cache_reset() in vxlan_snoop()
  (drivers/net/vxlan/vxlan_core.c:1485) runs in RX softirq context with…

--

vxlan: reset the remote's dst cache when snooping moves a MAC

This patch makes vxlan_snoop() reset the remote's dst_cache when a known
MAC shows up from a different remote and the remote IP is rewritten in
place. vxlan_fdb_replace() already does this on the netlink path. The
goal is to stop traffic to the moved MAC from using a dst cached for the
previous remote.

> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> index d11028b374721..6ee071cd2d280 100644
> --- a/drivers/net/vxlan/vxlan_core.c
> +++ b/drivers/net/vxlan/vxlan_core.c
> @@ -1482,6 +1482,7 @@ static enum skb_drop_reason vxlan_snoop(struct net_device *dev,
>  				    "%pM migrated from %pIS to %pIS\n",
>  				    src_mac, &rdst->remote_ip.sa, &src_ip->sa);
>  
> +		dst_cache_reset(&rdst->dst_cache);
>  		rdst->remote_ip = *src_ip;
>  		vxlan_fdb_notify(vxlan, f, rdst, RTM_NEWNEIGH, true, NULL);

[Severity: High]
Should this branch also update rdst->remote_ifindex and clear
rdst->offloaded, as vxlan_fdb_replace() does?

The commit message names vxlan_fdb_replace() as the reference. That
function does more than reset the cache:

drivers/net/vxlan/vxlan_core.c:vxlan_fdb_replace() {
    ...
	dst_cache_reset(&rd->dst_cache);
	rd->remote_ip = *ip;
	rd->remote_port = port;
	rd->remote_vni = vni;
	rd->remote_ifindex = ifindex;
	rd->offloaded = false;
    ...
}

The early return in vxlan_snoop() checks both the address and the
ifindex:

	if (likely(vxlan_addr_equal(&rdst->remote_ip, src_ip) &&
		   rdst->remote_ifindex == ifindex))
		return SKB_NOT_DROPPED_YET;

ifindex is src_ifindex for IPv6 link-local sources and 0 for everything
else. In these cases remote_ifindex still holds the old remote's value
after this branch runs:

- a MAC moves from a global address to an fe80:: address
- a MAC moves from an fe80:: address to a global address
- the same fe80:: address arrives on a different lower device
- a dynamic entry was created over netlink with an explicit via ifindex

On TX, vxlan_xmit_one() passes that stale value to the route lookup as
the oif:

	ifindex = rdst->remote_ifindex;

After the reset, could the cache be refilled with a route computed for
the previous remote's interface? If so, traffic to the moved MAC would
still go the wrong way, which is the problem this patch is meant to fix.

Also, remote_ifindex never matches again after this, so every later
packet from that peer re-enters this migration branch. With this patch
each of those packets now calls dst_cache_reset(), so every TX that
follows an RX does a full route lookup. Each one also sends an
RTM_NEWNEIGH notification and calls the switchdev notifiers. Neither is
ratelimited.

A stale offloaded bit would also make vxlan_fdb_info() report
NTF_OFFLOADED for a remote IP that no switchdev driver has acknowledged.

The missing remote_ifindex update goes back to commit 87613de95041
("vxlan: fix snooping for link-local IPv6 addresses"). The per-packet
dst_cache_reset() is new in this patch, though.

Would adding these here cover it?

	rdst->remote_ifindex = ifindex;
	rdst->offloaded = false;

[Severity: Low]
Can a concurrent transmitter still cache a route to the previous remote
after this reset?

The reset and the remote_ip store run in RX softirq context. No lock is
shared with the TX path. vxlan_xmit_one() copies remote_ip into the flow
key before it looks up rdst->dst_cache:

	key.u.ipv4.dst = rdst->remote_ip.sin.sin_addr.s_addr;

dst_cache_per_cpu_get() treats an entry as stale only in this case:

	if (unlikely(!time_after(idst->refresh_ts,
				 READ_ONCE(dst_cache->reset_ts)) || ...

Consider this interleaving:

CPU0 (TX, vxlan_xmit_one)          CPU1 (RX, vxlan_snoop)
reads old remote_ip into key
                                   dst_cache_reset()   /* reset_ts = J1 */
                                   rdst->remote_ip = *src_ip;
<jiffies ticks>
dst_cache_get_ip4() misses,
  refresh_ts = J2 > J1
route lookup to the old address
dst_cache_set_ip4()

Later packets on CPU0 put the new remote in the key, but they hit the
cached dst for the old next hop. vxlan_snoop() now sees matching
addresses, so nothing resets the cache again until the route is flushed
or the dst becomes obsolete.

Swapping the store and the reset would not close this, because TX reads
remote_ip without serialization. With IPv6, TX could also read a torn
value from the plain struct store to remote_ip and cache a route for it
the same way.

The window is narrow. vxlan_fdb_replace() and other dst_cache users that
reset after changing tunnel parameters have the same limitation, so this
may come from the lockless dst_cache invalidation design. Since the fix
depends on this reset, is that window acceptable here?

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