Re: [PATCH net-next 2/2] bpf: drop the stale dst when redirecting to an explicit nexthop

[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] The new `else { skb_dst_drop(skb); }` branches in
  __bpf_redirect_neigh_v4() and __bpf_redirect_neigh_v6()…
- [Low] The commit fixes a user-visible bug but has no Fixes: tag (and no
  stable Cc).

--

bpf: drop the stale dst when redirecting to an explicit nexthop

With BPF_F_NEXTHOP, __bpf_redirect_neigh_v4() and __bpf_redirect_neigh_v6()
skip the FIB lookup. They leave the packet's original dst attached while
skb->dev is switched to the target device. If the packet is parked on the
target's arp_queue, the drain looks the neighbour up again from that stale
dst and builds the frame with the wrong link-layer header, so the patch
drops the dst on the explicit-nexthop branch.

Link: https://lore.kernel.org/netdev/QDISC-6HS9.v3.r0.20261103062649-2@mojatatu.com/

> Reported-by: Sashiko (nipa) <sashiko-bot@kernel.org>
> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-6HS9.v3.r0.20261103062649-2@mojatatu.com
> Link: https://lore.kernel.org/netdev/QDISC-6HS9.v3.r0.20261103062649-2@mojatatu.com/
> Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

[Severity: Low]
Should this patch carry a Fixes: tag? The explicit-nexthop branches in
__bpf_redirect_neigh_v4() and __bpf_redirect_neigh_v6() that keep the
original route dst came from:

Fixes: ba452c9e996d ("bpf: Fix bpf_redirect_neigh helper api to support supplying nexthop")

The related fix for the non-nexthop branch, commit 23f3770e1a53 ("bpf: Fix
metadata_dst leak __bpf_redirect_neigh_v{4,6}"), had a Fixes: tag.
Without one here, stable kernels that contain ba452c9e996d may not get
this fix.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index aca8160b26a1a..2af24f371648a 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2318,6 +2318,8 @@ static int __bpf_redirect_neigh_v6(struct sk_buff *skb, struct net_device *dev,
>  		skb_dst_set(skb, dst);
>  	} else if (nh->nh_family != AF_INET6) {
>  		goto out_drop;
> +	} else {
> +		skb_dst_drop(skb);
>  	}

[Severity: High]
Can this drop a metadata dst that was attached by
bpf_skb_set_tunnel_key()?

The new else branch drops any dst on the skb and does not check whether
it is a route dst. bpf_skb_set_tunnel_key() stores the TX tunnel info in
a DST_METADATA dst:

net/core/filter.c:bpf_skb_set_tunnel_key() {
    ...
	skb_dst_drop(skb);
	dst_hold((struct dst_entry *) md);
	skb_dst_set(skb, (struct dst_entry *) md);
    ...
}

Take a tc program that calls bpf_skb_set_tunnel_key() and then
bpf_redirect_neigh() to a collect_md vxlan or geneve device, using an nh
from bpf_fib_lookup().

Before this patch the metadata dst stayed on the skb. vxlan and geneve
call netif_keep_dst(), so __dev_queue_xmit() keeps it. vxlan_xmit() then
finds skb_tunnel_info() with IP_TUNNEL_INFO_TX and encapsulates.

After this patch skb_tunnel_info() returns NULL. In collect_md mode,
vxlan_xmit() then drops the packet:

drivers/net/vxlan/vxlan_core.c:vxlan_xmit() {
    ...
			if (info && info->mode & IP_TUNNEL_INFO_TX)
				vxlan_xmit_one(skb, dev, cfg, vni, NULL, false);
			else
				kfree_skb_reason(skb, SKB_DROP_REASON_TUNNEL_TXINFO);
    ...
}

Geneve behaves the same way.

If the overlay neighbour is not yet resolved, arp_solicit() also reads
the tunnel metadata for the ARP request from the parked skb's dst:

net/ipv4/arp.c:arp_solicit() {
    ...
	if (skb && !(dev->priv_flags & IFF_XMIT_DST_RELEASE))
		dst = skb_dst(skb);
    ...
}

With that dst now NULL, would the solicitation be dropped too, so the
neighbour never resolves?

The commit message says:

  That branch never consults the route dst, so nothing is lost

That is true for a routing dst. The target device's xmit path does read
a metadata dst, though.

Metadata dsts use dst_blackhole_ops, and dst_blackhole_neigh_lookup()
returns NULL. For those dsts, the arp_queue drain already falls back to
the parking neighbour.

Would skipping the drop when skb_metadata_dst(skb) is set fix the
reported bug without this regression?

The same question applies to the new else branch in
__bpf_redirect_neigh_v4().

No selftest covers this case. test_tc_neigh_fib.c only redirects with a
FIB-derived nexthop and no tunnel key.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/CORE-9NOG.v1.r0.20261111073528%40mojatatu.com