Re: [PATCH net 2/2 repost3] 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 "Sorry Jamal, another round of AI-feedback for this one.
It is superficually simliar to the \AI-generated feedback for
repost2, but does raise some specific points that I think warrant
you expressing an opinion on. Thanks!"
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-6HS9.v1.r3.20261027034729-1@mojatatu.com?part=2
---
net/sched: sch_teql: keep skb->dev consistent on the arp-queue path

This patch makes teql_master_xmit() set skb->dev to the slave before
calling teql_resolve(). An skb parked on the slave's arp_queue then no
longer points at the teql master. It also makes
neigh_update_process_arp_queue() set skb->dev = n1->dev before it
reinjects queued skbs.

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

> The master can be freed while the slave lives, so the
> queued skb can dereference freed memory.

[Severity: Low]
Could the commit message say how the master gets freed? As far as I can
tell, teql_exit() is the only place that frees the master net_device, and
it only runs on module unload:

net/sched/sch_teql.c:teql_exit() {
    ...
	unregister_qdisc(&master->qops);
	unregister_netdev(master->dev);
	free_netdev(master->dev);
    ...
}

teql_master_setup() never sets rtnl_link_ops, so rtnl_delete_link()
refuses to delete the master:

	ops = dev->rtnl_link_ops;
	if (!ops || !ops->dellink)
		return -EOPNOTSUPP;

default_device_exit_net() moves devices like this back to init_net
instead of freeing them, so destroying a netns doesn't free it either.

That leaves one window. Every teql qdisc has been removed from the
slaves, an skb is still sitting on a slave neighbour's arp_queue with
skb->dev pointing at the master, and then sch_teql is unloaded. The
unload needs CAP_SYS_MODULE.

This is the justification for a stable-bound change to shared neighbour
reinjection. Would it help to describe this rmmod-only trigger in the
message?

> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index d6e9c7c59a892..120c3369406fb 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -1339,6 +1339,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]
This runs for every neighbour user, not only sch_teql. Here n1 isn't
always the neighbour the skb was queued on. It comes from
dst_neigh_lookup_skb() on the skb's retained dst, and
ipv4_dst_neigh_lookup() looks it up on dst_dev_rcu(dst).

Two existing parkers set skb->dev to a target device B but leave an
original route dst whose device is A.

The first is bpf_redirect_neigh() with explicit nexthop params.
__bpf_redirect_neigh_v4() and __bpf_redirect_neigh_v6() replace the dst
only when nh is NULL. bpf_out_neigh_v4() and bpf_out_neigh_v6() then set
skb->dev = B and call neigh_output() on B's nexthop neighbour.

The second is nft fwd ... neigh. nft_fwd_neigh_eval() decrements
TTL/hop_limit, sets skb->dev = B and calls neigh_xmit(), and it never
touches the dst. nft_fwd_validate() allows NF_NETDEV_EGRESS.

In __dev_queue_xmit(), nf_hook_egress() and sch_handle_egress() run
before the IFF_XMIT_DST_RELEASE skb_dst_drop(). __neigh_event_send()
also calls skb_dst_force() before queueing. So the dst is still attached
when the skb is parked on B's arp_queue.

The resulting sequence looks like this:

egress on A
  bpf_redirect_neigh(B, params) or nft fwd ... neigh
    skb->dev = B, skb_dst(skb)->dev == A
    __neigh_event_send() on B's NUD_INCOMPLETE neighbour, skb queued

B's neighbour resolves
  neigh_update_process_arp_queue()
    dst_neigh_lookup_skb() -> n1 is A's gateway neighbour
    skb->dev = n1->dev;    /* now A */
    n1->output() -> dev_queue_xmit(A)
      nf_hook_egress() / sch_handle_egress() on A again

Doesn't this run A's egress hooks a second time on packets they already
handled? With nft fwd neigh the TTL is decremented twice, and a packet
that arrived with TTL 2 would hit this in nft_fwd_neigh_eval():

	if (iph->ttl <= 1) {
		verdict = NF_DROP;
		goto out;
	}

Counters and quotas would also count the packet twice. Tc egress BPF
programs would run twice and could apply encap or NAT twice.

The second pass might not redirect again, for example if the first pass
changed a mark the rule depends on. In that case, would the packet leave
on A toward the original gateway instead of the nexthop the policy chose?

Before this patch skb->dev stayed B. These packets went out on B with A's
L2 header. That was already wrong, but they did not go back through A's
egress path.

The bpf_redirect_neigh uAPI docs still say the packet is redirected to
ifindex, and NFTA_FWD_SREG_DEV is still documented as the output
interface. The commit message says only "This is a behaviour change for
those paths", and the patch is Cc: stable.

Could the skb->dev restore be limited to the teql case, or handled in
sch_teql itself, rather than applied to every neighbour user here?