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

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

[CMGS]

vxlan_snoop() handles a known MAC that now arrives from another remote
by rewriting the remote's IP address in place. It leaves the remote's
dst_cache alone, so the next packet to that MAC is routed with the dst
cached for the previous remote. When the two remotes have different
next hops, the packet leaves the wrong way. For example, if the
previous remote was on-link and the new one is behind a router, the
device resolves the new remote's address on the local link, the
neighbour stays incomplete, and traffic to the MAC is blackholed until
the cached route is invalidated, such as by "ip route flush cache".

vxlan_fdb_replace(), which makes the same change through netlink,
already resets the cache. Reset it when snooping too.

Reproduced with network namespaces: a learning VXLAN device has one
peer VTEP on-link and another behind a router, and an inner MAC moves
from the first peer to the second. Without this patch, pings to the MAC
fail after the move until the route cache is flushed: in 5 of 5 runs
with one CPU, and in 1 of 3 runs with two CPUs, where it depends on
whether the sending CPU holds the stale entry. With the patch, they
succeed in all 8 runs.

This was found by our AI-driven automated live migration tests. The fix
and this changelog were written with the help of an AI coding assistant.

Fixes: 0c1d70af924b ("net: use dst_cache for vxlan device")
Signed-off-by: CMGS <ilskdw@gmail.com>
---
 drivers/net/vxlan/vxlan_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index d11028b37..6ee071cd2 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);
 	} else {
-- 
2.47.3

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

[netdev-bot+sashiko]

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

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

[CMGS]

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

Yes. The early return compares remote_ifindex and vxlan_xmit_one()
uses it as the oif, so leaving it stale can refill the cache with a
route for the previous interface, and with this patch every later
packet from that peer would reset the cache. v2 will set
remote_ifindex and clear offloaded in the same branch, as
vxlan_fdb_replace() does.

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

The window exists, but it is the same one vxlan_fdb_replace() and the
other dst_cache users that reset after a parameter change have. Closing
it needs a change to dst_cache invalidation, which is out of scope for
this fix.

pw-bot: cr