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

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] The new gate in neigh_update_process_arp_queue()
  (net/core/neighbour.c), `if (!n1->dev->header_ops) skb->dev =…
- [Low] The comment added in neigh_update_process_arp_queue()
  (net/core/neighbour.c), '/* headerless outputs (teql, shaper) transmit…

--

net/sched: sch_teql: keep skb->dev consistent on the arp-queue path

The commit sets skb->dev to the teql slave before teql_resolve(). This
means an skb parked on the slave's arp_queue no longer points at a master
that teql_exit() can free on module unload. It also changes
neigh_update_process_arp_queue() to set skb->dev to the top-level
neighbour's device when that device has no header_ops, so the skb goes
back through teql_master_xmit().

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

> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index d6e9c7c59a892..541cf699abae4 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 outputs (teql, shaper) transmit on skb->dev */

[Severity: Low]
This isn't a bug, but the shaper driver is no longer in the tree. There
is no entry for it in drivers/net/Makefile, and the older comment block
just above also names it.

Would it be clearer to name in-tree devices this gate is meant for, such
as teql and eql?

> +		if (!n1->dev->header_ops)
> +			skb->dev = n1->dev;
> +		READ_ONCE(n1->output)(n1, skb);

[Severity: Medium]
This gate is in generic neighbour code, and it doesn't check whether
sch_teql parked the skb. n1 comes from the skb's retained dst via
dst_neigh_lookup_skb().

Can it fire for any skb parked on an arp_queue whose original route uses
a device with NULL header_ops? Examples include ppp, geneve in L3 mode,
vxlan when header_ops is NULL, rmnet, qmi_wwan, mhi_net, ipa, iosm,
mhi_wwan_mbim, eql and teql.

Two producers park skbs on another device's neighbour while keeping the
original dst.

The first is nft_fwd_neigh_eval(), which nft_fwd_validate() allows on
NF_NETDEV_EGRESS:

	ip_decrease_ttl(iph);
	...
	skb->dev = dev;
	skb_clear_tstamp(skb);

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

The second is bpf_redirect_neigh() with an explicit nexthop.
__bpf_redirect_neigh_v4() replaces the dst only when nh == NULL:

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

bpf_out_neigh_v4() then sets skb->dev to the target and calls
neigh_output() with the old dst still attached.

When the target device X's neighbour resolves, the drain takes this path
(Y is the original route device):

neigh_update_process_arp_queue()
  dst_neigh_lookup_skb()  -> Y's neighbour (Y has no header_ops)
  skb->dev = Y
  neigh_direct_output()
    dev_queue_xmit() on Y

Does this run Y's egress hooks a second time? In __dev_queue_xmit(),
nf_hook_egress() runs before nf_skip_egress(skb, true) is set, so the
nft egress chain runs again. sch_handle_egress() and tc BPF also run
again.

For nft fwd, this would decrement TTL/hop_limit twice. A packet that
reaches Y's egress with TTL 2 would then be dropped.

Counters, quotas and limits would also be applied twice. A tc BPF program
that encapsulates could encapsulate twice. If the second pass gives a
different verdict, the packet would leave through Y and not through the
device it was forwarded to.

Before this patch these skbs went out X without a link header, which was
also wrong. Running Y's egress hooks a second time is new, though.

The commit message acknowledges this:

  "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, not part of this
  series."

The patch is tagged Cc: stable. Would stable trees get the new behaviour
without the producer-side fix?

Should the dst drop in nft_fwd_neigh_eval() and in the bpf_redirect_neigh()
explicit nexthop path go in the same series as this change?

>  		if (n2)
>  			neigh_release(n2);

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