Re: [PATCH net v4 2/3] net: neighbour: reinject a parked skb through a headerless output device

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] The new check in neigh_update_process_arp_queue()
  (net/core/neighbour.c) is `if (!n1->dev->header_ops) skb->dev =…

--

net: neighbour: reinject a parked skb through a headerless output device

When n1->dev has no header_ops, this patch makes
neigh_update_process_arp_queue() set skb->dev to n1->dev before it calls
n1->output. Here n1 is the top-level neighbour looked up again from the
skb's dst. The patch is aimed at sch_teql, which parks skbs on the slave's
neighbour with skb->dev set to the slave.

Link: https://lore.kernel.org/netdev/20261110101908.2e64e8e9@kernel.org/

> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index d6e9c7c59a892..1b556fa17c57c 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -1339,6 +1339,9 @@ static void neigh_update_process_arp_queue(struct neighbour *neigh)
>  			if (n2)
>  				n1 = n2;
>  		}
> +		/* headerless output transmits on skb->dev, not n1->dev */
> +		if (!n1->dev->header_ops)
> +			skb->dev = n1->dev;
>  		READ_ONCE(n1->output)(n1, skb);

[Severity: Medium]
Can this gate send a redirected skb back through the egress hooks of the
original device?

The check has no way to tell sch_teql apart from forwards that move the
skb to another device but keep the original route's dst attached. Two
forwards do this: nft fwd ... neigh, and bpf_redirect_neigh() called with
explicit nexthop parameters.

Here is the sequence:

1. One of these forwards runs at the egress hook of a device X that has no
header_ops (for example wireguard, ipip or sit). nft_fwd_validate() allows
NF_NETDEV_EGRESS. The egress hooks run before the dst is dropped, so
dst->dev is still X:

net/core/dev.c:__dev_queue_xmit() {
    ...
	skb = nf_hook_egress(skb, &rc, dev);
    ...
	skb = sch_handle_egress(skb, &rc, dev);
    ...
	if (dev->priv_flags & IFF_XMIT_DST_RELEASE)
		skb_dst_drop(skb);
    ...
}

2. The forward sets skb->dev to an ARP-capable device Y and transmits
without dropping 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);
    ...
}

On the BPF side, __bpf_redirect_neigh_v4() replaces the dst only when
nh is NULL:

	if (!nh) {
		...
		skb_dst_drop(skb);
		skb_dst_set(skb, &rt->dst);
	}

	err = bpf_out_neigh_v4(net, skb, dev, nh);

The IPv6 path through bpf_out_neigh_v6() follows the same pattern.

3. If Y's neighbour is INCOMPLETE, the skb is parked on Y's arp_queue
with the stale dst still attached.

4. When Y's neighbour resolves, the drain loop sets n1 from
dst_neigh_lookup_skb(dst, skb). That gives X's neighbour, and because X
has no header_ops, arp_constructor() set it up with neigh_direct_output:

net/ipv4/arp.c:arp_constructor() {
    ...
	if (!dev->header_ops) {
		neigh->nud_state = NUD_NOARP;
		neigh->ops = &arp_direct_ops;
		neigh->output = neigh_direct_output;
    ...
}

5. The new gate changes skb->dev from Y back to X, and
neigh_direct_output() then calls dev_queue_xmit() on X.

As a result, X's nft netdev egress chain and its tc egress BPF program
run a second time on a packet they have already handled.

nft_fwd_neigh_eval() calls ip_decrease_ttl() (or decrements hop_limit)
again, so TTL drops by 2. A packet that arrives with TTL 2 is then
dropped.

Counters, quotas and limits count the packet twice, and BPF mangling or
encapsulation is applied twice. If the second pass makes a different
decision, the packet leaves on X instead of the forward target.

Before this patch the same parked skb was also mishandled: it left Y
through neigh_direct_output with no link header. So the patch changes how
this case fails, but running the hooks a second time is new.

The commit message describes this side effect:

  Those forwards supply their own device and nexthop and do not consult
  the retained dst, so dropping it at those two sites is where that is
  fixed; that is a separate change to the producers.

That producer change does not appear in the series. The later commit
"net/sched: sch_teql: keep skb->dev consistent on the arp-queue path"
only touches sch_teql.c, so the double traversal is still there at the
end of the series.

Could skb_dst_drop() be added to nft_fwd_neigh_eval() and to the nh != NULL
paths of __bpf_redirect_neigh_v4() and __bpf_redirect_neigh_v6() as part of
this series, ordered before this patch?

>  		if (n2)
>  			neigh_release(n2);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-6HS9.v4.r0.20261111094338%40mojatatu.com