Re: [PATCH net 2/2 repost2] net/sched: sch_teql: keep skb->dev consistent on the arp-queue path

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email says "Hi Jamal, passing this on for your consideration as it
seems to have got stuck somewhere."
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-6HS9.v1.r2.20261020051718-1@mojatatu.com?part=2
---
net/sched: sch_teql: keep skb->dev consistent on the arp-queue path

The patch sets skb->dev = slave before teql_resolve(). An skb parked on
the slave's neighbour arp_queue then names the slave, not the teql master.
It also adds skb->dev = n1->dev at the reinjection point in
neigh_update_process_arp_queue(), so parked teql skbs go back through
teql_master_xmit() when the neighbour resolves.

Link: https://lore.kernel.org/netdev/20260824115928.4099988-1-victor@mojatatu.com/

> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 7448320f7ad52..5823f5007812b 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -1336,6 +1336,7 @@ static void neigh_update_process_arp_queue(struct neighbour *neigh)
>  			if (n2)
>  				n1 = n2;
>  		}
> +		skb->dev = n1->dev;
>  		READ_ONCE(n1->output)(n1, skb);
>  		if (n2)
>  			neigh_release(n2);

[Severity: Medium]
The commit message says this assignment "is the identity for every other
parker (neigh_resolve_output() parks with skb->dev == neigh->dev, ndisc
probes carry no skb)". Does that hold when the skb's dst points at a
different device than the neighbour it was parked on?

If the dst is not DST_OBSOLETE_DEAD, n1 is not the parking neighbour. It is
replaced by n2 from dst_neigh_lookup_skb(), and ipv4_neigh_lookup() looks
that neighbour up on the dst's device:

net/ipv4/route.c:ipv4_neigh_lookup() {
    ...
	dev = dst_dev_rcu(dst);
	if (likely(rt->rt_gw_family == AF_INET)) {
		n = ip_neigh_gw4(dev, rt->rt_gw4);
    ...
}

So the new line is only a no-op when dst->dev == skb->dev.
skb->dev == neigh->dev is not enough.

At least two parkers seem to break that. The first is bpf_redirect_neigh()
with explicit nexthop params. bpf_out_neigh_v4() moves skb->dev to the
redirect target B and resolves the nexthop on B:

net/core/filter.c:bpf_out_neigh_v4() {
    ...
	skb->dev = dev;
    ...
	} else if (nh->nh_family == AF_INET) {
		neigh = ip_neigh_gw4(dev, nh->ipv4_nh);
    ...
}

However, __bpf_redirect_neigh_v4() only replaces the dst in the !nh branch,
so the routed dst for the original device A stays attached:

net/core/filter.c:__bpf_redirect_neigh_v4() {
    ...
		skb_dst_drop(skb);
		skb_dst_set(skb, &rt->dst);
	}
    ...
}

bpf_out_neigh_v6() follows the same pattern with ip_neigh_gw6().

The second is nft fwd ... neigh in a netdev egress chain.
nft_fwd_neigh_eval() changes skb->dev and parks the skb without touching
the dst:

net/netfilter/nft_fwd_netdev.c:nft_fwd_neigh_eval() {
    ...
	skb->dev = dev;
	skb_clear_tstamp(skb);

	nf_dev_xmit_recursion_inc();
	neigh_xmit(neigh_table, dev, addr, skb);
    ...
}

In __dev_queue_xmit(), nf_hook_egress() and sch_handle_egress() run before
the IFF_XMIT_DST_RELEASE skb_dst_drop(), so A's routed dst is still
attached at these hooks. If B's neighbour is incomplete, the skb is parked
on B's arp_queue with skb->dev == B and dst->dev == A.

When B's neighbour resolves, n2 is A's neighbour. With this patch skb->dev
becomes A, and n1->output() builds A's link-layer header and calls
dev_queue_xmit() on A.

Before the patch, the frame still left on B, though with A's link-layer
header, which was already a pre-existing problem. After the patch, would
the packets queued during resolution go through A's egress qdisc, tc and
nft hooks a second time?

That could mean nft_fwd_neigh_eval() calls ip_decrease_ttl() or
decrements hop_limit again. Non-idempotent BPF programs (encap,
adjust_room) would run again, and counting or stateful rules would see
the packet twice. If the redirect is not applied again on A's path, the
packets would leave on A and bypass the configured BPF or nft steering.

Since this is a change to core neighbour code tagged for stable, could the
reinjection fix be limited to the teql case? Alternatively, could the
commit message describe the behaviour change for these other parkers?