[PATCH net 0/2 repost3] net/sched: sch_teql: fix header-cloned write and retry strip

[PATCH net 0/2 repost3] net/sched: sch_teql: fix header-cloned write and retry strip

[Jamal Hadi Salim]

Two pre-existing defects on the teql transmit path, both around
teql_master_xmit() -> teql_resolve().  The first makes the slave's
header write corrupt a header-cloned skb and strips a userspace-supplied
link header on retry; the second leaves skb->dev pointing at the teql
master while the skb is parked on a slave's neighbour arp_queue.

Patch 1 makes the header private before the write and pulls back only
the header the slave added.
Patch 2 moves skb->dev = slave ahead of teql_resolve() and restores
skb->dev at the arp-queue reinjection point, so a parked skb names the
device whose neighbour took it and is delivered with a link header.

Both are follow-ups to commit dc4b95b8fee9 ("net/sched: sch_teql: restore
skb->dev on the slave failure path").

The pre-existing teql issues reported alongside this series are fixed
separately, not here.  In particular, the lower-qdisc drop use-after-free
(`NETDEV_TX_OK` vs `dev_xmit_complete()`) is fixed by:
https://lore.kernel.org/netdev/QDISC-Y0BY.v2.r0.20261024060625-2@mojatatu.com/

This is repost3 of the same two patches; there is no code change.  The
only changes since repost2 are the commit messages: the patch 1/2
message now says "header-cloned" instead of "shared", and the patch 2/2
commit message now describes the reinjection behaviour change for
bpf_redirect_neigh() and nft fwd ... neigh instead of claiming the
restore is an identity for every parker.

Jamal Hadi Salim (2):
  net/sched: sch_teql: unshare the header before writing and fix retry
    strip
  net/sched: sch_teql: keep skb->dev consistent on the arp-queue path

 net/core/neighbour.c |  1 +
 net/sched/sch_teql.c | 34 ++++++++++++++++++++++++++++------
 2 files changed, 29 insertions(+), 6 deletions(-)

-- 
2.43.0

[PATCH net 1/2 repost3] net/sched: sch_teql: unshare the header before writing and fix retry strip

[Jamal Hadi Salim]

teql_master_xmit() writes the slave's link header into the skb in
teql_resolve() and on retry pulls it back with __skb_pull().  Two bugs:

1. When the skb's header is cloned (e.g. a tc-mirred action on a
   physical device's clsact egress redirects to the teql master), the
   write corrupts the other clone.  Call skb_cow_head() first.

2. The pull always removes skb_network_offset() bytes.  For an
   AF_PACKET/SOCK_RAW frame with no dst entry, teql_resolve() adds no
   header, so the retry strips the user-supplied one.  Pull back only
   the length this slave added.

Track the added length per iteration; a header-cloned skb is unshared
before the write and an AF_PACKET frame retries with its header intact.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Reported-by: Sashiko (gemini) <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260824115928.4099988-1-victor@mojatatu.com
Link: https://lore.kernel.org/netdev/20260824115928.4099988-1-victor@mojatatu.com/
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_teql.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
index 409ce50cc0db..acd03f9afc6b 100644
--- a/net/sched/sch_teql.c
+++ b/net/sched/sch_teql.c
@@ -245,7 +245,7 @@ static int teql_qdisc_init(struct Qdisc *sch, struct nlattr *opt,
 static int
 __teql_resolve(struct sk_buff *skb, struct sk_buff *skb_res,
 	       struct net_device *dev, struct netdev_queue *txq,
-	       struct dst_entry *dst)
+	       struct dst_entry *dst, int *hlen)
 {
 	struct neighbour *n;
 	int err = 0;
@@ -265,15 +265,28 @@ __teql_resolve(struct sk_buff *skb, struct sk_buff *skb_res,
 	}
 
 	if (neigh_event_send(n, skb_res) == 0) {
+		int off = skb_network_offset(skb);
 		char haddr[MAX_ADDR_LEN];
 
 		neigh_ha_snapshot(haddr, n, dev);
+		/* The skb may be shared (e.g. a packet tap clone); make the
+		 * headroom private before dev_hard_header() writes into it.
+		 */
+		if (skb_cow_head(skb, LL_RESERVED_SPACE(dev)) < 0) {
+			err = -ENOMEM;
+			goto out;
+		}
 		if (dev_hard_header(skb, dev, ntohs(skb_protocol(skb, false)),
 				    haddr, NULL, skb->len) < 0)
 			err = -EINVAL;
+		/* The header, if any, is prepended above skb->data, so the
+		 * network offset grew by exactly the bytes to undo later.
+		 */
+		*hlen = skb_network_offset(skb) - off;
 	} else {
 		err = (skb_res == NULL) ? -EAGAIN : 1;
 	}
+out:
 	neigh_release(n);
 	return err;
 }
@@ -281,11 +294,13 @@ __teql_resolve(struct sk_buff *skb, struct sk_buff *skb_res,
 static inline int teql_resolve(struct sk_buff *skb,
 			       struct sk_buff *skb_res,
 			       struct net_device *dev,
-			       struct netdev_queue *txq)
+			       struct netdev_queue *txq,
+			       int *hlen)
 {
 	struct dst_entry *dst = skb_dst(skb);
 	int res;
 
+	*hlen = 0;
 	if (rcu_access_pointer(txq->qdisc) == &noop_qdisc)
 		return -ENODEV;
 
@@ -293,7 +308,7 @@ static inline int teql_resolve(struct sk_buff *skb,
 		return 0;
 
 	rcu_read_lock();
-	res = __teql_resolve(skb, skb_res, dev, txq, dst);
+	res = __teql_resolve(skb, skb_res, dev, txq, dst, hlen);
 	rcu_read_unlock();
 
 	return res;
@@ -305,6 +320,7 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct Qdisc *start, *q;
 	int busy;
 	int nores;
+	int hlen;
 	int subq = skb_get_queue_mapping(skb);
 	struct sk_buff *skb_res = NULL;
 
@@ -332,7 +348,7 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev)
 			continue;
 		}
 
-		switch (teql_resolve(skb, skb_res, slave, slave_txq)) {
+		switch (teql_resolve(skb, skb_res, slave, slave_txq, &hlen)) {
 		case 0:
 			if (__netif_tx_trylock(slave_txq)) {
 				unsigned int length = qdisc_pkt_len(skb);
@@ -374,8 +390,14 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev)
 			nores = 1;
 			break;
 		}
+		/* Undo only the header teql_resolve() pushed for this slave.
+		 * Pulling skb_network_offset() instead would strip a
+		 * userspace-supplied header when the slave added none, e.g.
+		 * an AF_PACKET/SOCK_RAW frame with no dst entry.
+		 */
+		if (hlen > 0)
+			__skb_pull(skb, hlen);
 		skb->dev = dev;
-		__skb_pull(skb, skb_network_offset(skb));
 	} while ((q = rcu_dereference(NEXT_SLAVE(q))) != start);
 
 	if (nores && skb_res == NULL) {
-- 
2.43.0

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

[Jamal Hadi Salim]

teql_master_xmit() sets skb->dev = slave only right before the slave's
ndo_start_xmit(). If the slave's neighbour is unresolved, teql_resolve()
parks the skb on the slave's arp_queue with skb->dev still naming the
teql master. The master can be freed while the slave lives, so the
queued skb can dereference freed memory.

Set skb->dev = slave before teql_resolve() so the parked skb names the
device whose neighbour owns it, and restore skb->dev = n1->dev at the
neigh_update_process_arp_queue() reinjection point so the skb goes back
through teql_master_xmit() instead of leaving headerless.

The reinjection restore is not an identity for other parkers:
bpf_redirect_neigh() with explicit nexthop parameters and nft
fwd ... neigh can park with dst->dev != skb->dev. For those paths it
moves skb->dev to the dst device and the packet can re-enter that
device's egress path. This is a behaviour change for those paths.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Reported-by: Sashiko (nipa) <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824115928.4099988-1-victor@mojatatu.com
Link: https://lore.kernel.org/netdev/20260824115928.4099988-1-victor@mojatatu.com/
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/core/neighbour.c | 1 +
 net/sched/sch_teql.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 7448320f7ad5..5823f5007812 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);
diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
index acd03f9afc6b..3fbd5af10028 100644
--- a/net/sched/sch_teql.c
+++ b/net/sched/sch_teql.c
@@ -348,12 +348,12 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev)
 			continue;
 		}
 
+		skb->dev = slave;
 		switch (teql_resolve(skb, skb_res, slave, slave_txq, &hlen)) {
 		case 0:
 			if (__netif_tx_trylock(slave_txq)) {
 				unsigned int length = qdisc_pkt_len(skb);
 
-				skb->dev = slave;
 				if (!netif_xmit_frozen_or_stopped(slave_txq) &&
 				    netdev_start_xmit(skb, slave, slave_txq, false) ==
 				    NETDEV_TX_OK) {
-- 
2.43.0

Re: [PATCH net 0/2 repost3] net/sched: sch_teql: fix header-cloned write and retry strip

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net 0/2 repost3] net/sched: sch_teql: fix header-cloned write and retry strip

[Jamal Hadi Salim]

cheers

On Tue, Oct 27, 2026 at 4:01 AM <netdev-bot+sinfo@kernel.org> wrote:
>
> Hi!
>
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
>
>  - Whether the issue was actually triggered, or is only theoretical
>    (e.g. found by code inspection). If it was triggered please include
>    the symptoms, like the stack trace or error messages.
>
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
>
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

My bad for not explicitly calling it out in the commit this time to
avoid you sending this email; answer is the same as before:
https://lore.kernel.org/netdev/CAM0EoM=yCSeB9+Q8gFsB8sjQhddM9qWh8DOfSqqNtp6byubv2w@mail.gmail.com/
https://lore.kernel.org/netdev/CAM0EoMnb78SoNwrqgH5308B_+Mh1sBtm+wJ=SFXtMHS-L2zfXg@mail.gmail.com/

cheers,
jamal

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

[Simon Horman]

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?

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

[Jamal Hadi Salim]

On Sat, Oct 31, 2026 at 1:00 PM Simon Horman <horms@kernel.org> wrote:
>
> 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.
>

Man, this thing is relentless (double edged sword, i like it's
improving the code but on the other side it's sucking my time with
more pedantry than I have ever been subjected to by any human, and i
have met human pedantry).
Like ive said in other emails: No good deed goes unpunished by this thing.

To be fair: nipa's repost2 review was on the mark already - i should
have invested more time analyzing.

cheers,
jamal

(I am finding myself using that word now and other annoying words like
"shape", etc)
> 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?