[PATCH net v4 0/3] net/sched: sch_teql: keep the transmit path's skb geometry and device lifetime intact

[PATCH net v4 0/3] net/sched: sch_teql: keep the transmit path's skb geometry and device lifetime intact

[Jamal Hadi Salim]

Three pre-existing defects on the teql transmit path, all around
teql_master_xmit() -> teql_resolve(), plus the core helper they depend on
all derived from sashiko reports.

Patch 1 makes the headroom private before the slave's header write and
pulls back only the header that slave added, so a header-cloned skb is not
corrupted and a userspace-supplied link header survives a retry.

Patch 2 restores skb->dev at the arp-queue reinjection point when the
neighbour's device builds no link header. It is the neighbour half of the
version 3 patch 2, split out on review so it gets its own commit
log and its own Cc set; requested by Jakub.

Patch 3 moves skb->dev = slave ahead of teql_resolve() so a skb parked on a
slave's neighbour arp_queue names the device whose neighbour took it, and
so cannot outlive the master it came from.  With patch 2 in place it is
reinjected through the master and leaves with a link header.  Its commit
log carries the KASAN report the unfixed kernel prints when that queue is
drained.

Patch 3 is a follow-up to commit dc4b95b8fee9 ("net/sched: sch_teql:
restore skb->dev on the slave failure path") by Victor. Patch 1 is the
same commit's sibling on the successful-transmit path.

To prempt the sashikos:
- The pre-existing teql issues reported alongside this series are fixed
separately. For "the lower-qdisc drop use-after-free" see:
https://lore.kernel.org/netdev/QDISC-Y0BY.v2.r0.20261024060625-2@mojatatu.com/

- For the pre-existing consumer nft/bpf complaint see:
https://lore.kernel.org/netdev/CORE-9NOG.v1.r0.20261111073528@mojatatu.com/
Note: nft/bpf combination _has never worked_: it is a producer-side
mismatch on two separate entry points, not a regression of anything here -
hence the net-next target.

Hope the sashikos read these and dont complain again about those issues.

Note also: part of the testing of patch 2 runs for packets that reach
neigh_xmit() from nft's `fwd ... neigh` statement, or from
bpf_redirect_neigh() with explicit nexthop parameters. Both keep the
route's dst attached while replacing skb->dev, so the neighbour that
queues the packet is not the one the dst names.

Changes v3 -> v4:
- patch 1, no code changes. Reviewed-by from Simon Horman stays, and a
  Link to the review of the v3 posting is added.
- patch 2 is new: the net/core/neighbour.c hunk of the v3 patch 2 is split
  out into its own fix, with the neighbour maintainers Cc'd.  Asked for on
  the v3 posting by Jakub.
- patch 3 is the v3 patch 2 with that hunk removed; the sch_teql change is
  unchanged in approach. Reviewed-by from Simon Horman stays.

Changes v2 -> v3:
- patch 1, no code changes.
- patch 2 corrects the conditions to recreate the bug: reaching a freed
  master needs sch_teql built as a module, the slave qdisc removed and the
  module unloaded while the skb still waits.  The message now says which
  part of it reproduces without the unload, and names the neighbour failure
  path that drains the same parked skb.  Considered comments from
  Sashiko (nipa).

Changes v1 -> v2:
- patch 1, no code changes.
- patch 2 limits the reinjection restore to a top-level neighbour whose
  device has no header_ops, the shaper/eql/sch_teql class.  Commit message
  also describes how a master is freed.  Considered comments from
  Sashiko (nipa) and Sashiko (gemini).

Jamal Hadi Salim (3):
  net/sched: sch_teql: unshare the header before writing and fix retry
    strip
  net: neighbour: reinject a parked skb through a headerless output
    device
  net/sched: sch_teql: keep skb->dev consistent on the arp-queue path

 net/core/neighbour.c |  3 +++
 net/sched/sch_teql.c | 26 ++++++++++++++++++++------
 2 files changed, 23 insertions(+), 6 deletions(-)

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
-- 
2.43.0

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

[Jamal Hadi Salim]

The issue:
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.

The Fix:
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.

Conditions to recreate the bug:
CONFIG_NET_SCH_TEQL=y and CONFIG_VETH=y, run as root. A teql master with
two veth slaves, the first slave returning NETDEV_TX_BUSY without
consuming the skb so that teql retries the frame on the second one:

  ip link add vethBAD type veth peer name vethBADp
  ip link add vethGOOD type veth peer name vethGOODp
  ip link set vethBAD up;  ip link set vethBADp up
  ip link set vethGOOD up; ip link set vethGOODp up
  tc qdisc add dev vethBAD root teql0
  tc qdisc add dev vethGOOD root teql0
  ip link set teql0 up

Send a SOCK_RAW frame that carries its own link header on teql0 and
capture it on the second slave's peer. A veth transmit does not return
NETDEV_TX_BUSY on its own; to hold the skb pending this reproduction used
a test-only IFF_DEBUG accelerator in veth_xmit(), which is not part of
this patch and was applied to both the unfixed and the fixed build.

On the unfixed tree the captured frame arrives with its header stripped
(176 of the 208 bytes sent, starting at the payload) because the retry
pulls skb_network_offset() bytes while this slave added none. With the
patch the frame arrives intact.

The shared headroom case needs a cloned skb: a PF_PACKET tap on the
master whose socket is never drained keeps the clone alive across the
transmit. A kprobe on eth_header then reports cloned=1 on the unfixed
tree and cloned=0 with the patch.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
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/
Link: https://lore.kernel.org/netdev/20261110101908.2e64e8e9@kernel.org/
Cc: stable@vger.kernel.org
Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

---
v3 -> v4:
- added Simon Horman's Reviewed-by, given on the v3 posting. The patch
  itself is unchanged; the series is resent because 2/2 had to be split
  as asked in the review of that patch.

 net/sched/sch_teql.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
index 70c948a26f80..ed8faa848f18 100644
--- a/net/sched/sch_teql.c
+++ b/net/sched/sch_teql.c
@@ -257,7 +257,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;
@@ -288,15 +288,24 @@ __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);
+		/* Unshare the header before dev_hard_header() writes to 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 bytes prepended above skb->data, pulled back on retry. */
+		*hlen = skb_network_offset(skb) - off;
 	} else {
 		err = (skb_res == NULL) ? -EAGAIN : 1;
 	}
+out:
 	neigh_release(n);
 	return err;
 }
@@ -304,11 +313,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;
 
@@ -316,7 +327,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;
@@ -330,6 +341,7 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct Qdisc *start, *q;
 	unsigned long flags;
 	int nores;
+	int hlen;
 	int busy;
 
 restart:
@@ -356,7 +368,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);
@@ -403,8 +415,10 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev)
 			nores = 1;
 			break;
 		}
+		/* Pull back only the header this slave added. */
+		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 v4 2/3] net: neighbour: reinject a parked skb through a headerless output device

[Jamal Hadi Salim]

The issue:
neigh_update_process_arp_queue() drains a neighbour's arp_queue and
reinjects every skb through READ_ONCE(n1->output)(n1, skb), where n1 is
the top-level neighbour re-looked-up from the skb's dst, not the neighbour
the skb was queued on. skb->dev is whatever the producer left there.
Note: skb->dev and n1->dev are not guaranteed to be equal.

When n1->dev has no header_ops, arp_constructor() and ndisc_constructor()
install neigh_direct_output(), a plain dev_queue_xmit() that transmits on
skb->dev and builds no link header. Such an output sends the skb out
whatever device skb->dev names, not out n1->dev, so a producer that
parked the skb naming a lower device has it leave without the link header
its own transmit path builds.

sch_teql is the producer that hits this: it parks the skb on the slave's
neighbour while the top-level neighbour is the master's, so skb->dev and
n1->dev differ and the reinjection has to come back through the master to
leave with a header. It names the slave on purpose, so that a parked skb
does not outlive the master it came from.

This half was split out of the sch_teql fix on review, so that the
neighbour restore carries its own commit log and its own Cc set.

The Fix:
Put skb->dev back to n1->dev before the output call when that device
builds no link header, so a headerless output transmits on the device the
neighbour belongs to, whatever the producer left in skb->dev.

A forward that steers the skb to another device, nft fwd ... neigh or
bpf_redirect_neigh() with explicit nexthop parameters, parks it with the
dst of the original route still attached. When the original route is
itself a headerless device this gate matches there and the skb goes back
out the original device, running its egress hook a second time. 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.

Conditions to recreate the bug:
CONFIG_NET_SCH_TEQL=m and CONFIG_VETH=y, run as root. This is the
neighbour half of the sch_teql parked-skb fix, reached once sch_teql parks
the skb naming the slave: create a veth pair, make one end a teql slave,
give the master an address and a route, flush the slave's neighbour entry
and send one packet:

  ip link add v0 type veth peer name v0p
  ip link set v0 up;  ip link set v0p up
  tc qdisc add dev v0 root teql0
  ip link set teql0 up
  ip addr add 10.0.0.1/24 dev teql0
  ip route add 10.0.0.99/32 dev teql0
  ip neigh flush dev v0
  ping -I teql0 -c 1 10.0.0.99

The packet parks on v0's arp_queue. When the solicitation resolves,
neigh_update_process_arp_queue() reinjects it; without this change it
leaves v0 directly, with no link header, and never reaches the peer.

The window that frees the master, and the unload recipe that shows it,
are in the sch_teql patch.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Link: https://lore.kernel.org/netdev/20261110101908.2e64e8e9@kernel.org/
Cc: stable@vger.kernel.org
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/core/neighbour.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index d6e9c7c59a89..541cf699abae 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);
 		if (n2)
 			neigh_release(n2);
-- 
2.34.1

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

[Jamal Hadi Salim]

This is a follow-up to commit dc4b95b8fee9 ("net/sched: sch_teql: restore
skb->dev on the slave failure path") by Victor Nogueira, which covered the
slave side of the same mismatch.

The issue:
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. teql_exit() frees the masters on module unload and a slave can
outlive them, so a queued skb can be reinjected through freed memory.

The Fix:
Set skb->dev = slave before teql_resolve() so the parked skb names the
device whose neighbour owns it.

The parked skb has a second consumer: when the neighbour fails instead of
resolving, neigh_invalidate() hands it to error_report() and
dst_link_failure(), which read skb->dev to report the failure, so it names
the slave on that path too.

A parked skb is reinjected by neigh_update_process_arp_queue(), and a
device that transmits straight on skb->dev has to be handed its skb back
there. That is a change to net/core/neighbour.c, posted as its own fix,
and it has to be applied before this one: without it the parked packet
leaves the slave directly, with no link header, because teql_master_xmit()
has already pulled it.

Some context:
Only a module unload frees a master. teql_master_setup() installs no
rtnl_link_ops and default_device_exit_net() moves the device to init_net
when its namespace exits, so the window is a parked skb plus CAP_SYS_MODULE
on an unloaded sch_teql.

Conditions to recreate the bug:
CONFIG_NET_SCH_TEQL=m and CONFIG_VETH=y, run as root. Create a veth pair,
make one end a teql slave, give the master an address and a route, flush
the slave's neighbour entry and send one packet:

  ip link add v0 type veth peer name v0p
  ip link set v0 up;  ip link set v0p up
  tc qdisc add dev v0 root teql0
  ip link set teql0 up
  ip addr add 10.0.0.1/24 dev teql0
  ip route add 10.0.0.99/32 dev teql0
  ip neigh flush dev v0
  ping -I teql0 -c 1 10.0.0.99 &
  sleep 1

The skb waits on v0's arp_queue while the solicitation is in flight.
Remove the qdisc, which drops the module reference it holds, and unload
the module while the skb still waits:

  tc qdisc del dev v0 root
  rmmod sch_teql

teql_exit() frees teql0 there. When the solicitation then fails,
neigh_invalidate() drains the queue through arp_error_report() and
ipv4_link_failure(), whose __icmp_send() reads skb_in->dev; that is the
freed master before this patch and v0 after it. A resolved neighbour
drains the same queue through neigh_update_process_arp_queue().

On the unfixed tree the skb is parked on v0's arp_queue while skb->dev
still names teql0, so the reinjection can reach a master that teql_exit()
has freed.

The park and the headerless delivery reproduce without the unload.
Freeing the master while the skb waits needs sch_teql built as a module,
as above.

An unfixed kernel reports this when the timer drains that queue:

  ==================================================================
  BUG: KASAN: slab-use-after-free in __icmp_send+0x3f9/0x1680
  Read of size 4 at addr ffff8880039bc0b0 by task swapper/3/0

  CPU: 3 UID: 0 PID: 0 Comm: swapper/3
  Call Trace:
   <IRQ>
   ...
   __icmp_send+0x3f9/0x1680
   ipv4_link_failure+0x3a0/0x5b0
   arp_error_report+0x5b/0xc0
   neigh_invalidate+0x125/0x240
   neigh_timer_handler+0x47d/0x750
   call_timer_fn+0x138/0x410
   ...
  Allocated by task 307:
   alloc_netdev_mqs+0x81/0x930
   ...
  Freed by task 335:
   netdev_release+0x45/0x60
   device_release+0x7d/0x120
   kobject_put+0xf3/0x2e0
   put_device+0x13/0x30
   free_netdev+0x3b3/0x4a0
   ...

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
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/
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/
Link: https://lore.kernel.org/netdev/20261110101908.2e64e8e9@kernel.org/
Cc: stable@vger.kernel.org
Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---

v3 -> v4:
- Move the net/core/neighbour.c change out of this patch into its own fix,
  as asked in the review. The sch_teql change is unchanged. Considered
  comments from Jakub Kicinski.

---
 net/sched/sch_teql.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
index 3f781a3ffb41..2a8383546ce8 100644
--- a/net/sched/sch_teql.c
+++ b/net/sched/sch_teql.c
@@ -373,13 +373,13 @@ 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);
 				int rc = NETDEV_TX_BUSY;
 
-				skb->dev = slave;
 				if (!netif_xmit_frozen_or_stopped(slave_txq))
 					rc = netdev_start_xmit(skb, slave, slave_txq, false);
 				__netif_tx_unlock(slave_txq);
-- 
2.34.1

Re: [PATCH net v4 1/3] net/sched: sch_teql: unshare the header before writing and fix retry strip

[netdev-bot+sashiko]

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 fix leaves a gap: `skb_cow_head()` in `__teql_resolve()`
  (net/sched/sch_teql.c) only runs when `neigh_event_send(n, skb_res) ==…

--

net/sched: sch_teql: unshare the header before writing and fix retry strip

This patch calls skb_cow_head() in __teql_resolve() before
dev_hard_header() writes the slave's link header, so that a header-cloned
skb does not corrupt the other clone. It also records how many header bytes
each slave added in teql_master_xmit() and pulls back only that many on
retry. As a result, an AF_PACKET frame with no dst keeps its user-supplied
header.

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

> diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
> index dc21cd9704a25..3f781a3ffb41a 100644
> --- a/net/sched/sch_teql.c
> +++ b/net/sched/sch_teql.c

[ ... ]

> @@ -293,15 +293,24 @@ __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);
> +		/* Unshare the header before dev_hard_header() writes to 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 bytes prepended above skb->data, pulled back on retry. */
> +		*hlen = skb_network_offset(skb) - off;
>  	} else {
>  		err = (skb_res == NULL) ? -EAGAIN : 1;
>  	}

[Severity: Medium]
The skb_cow_head() only runs when neigh_event_send() returns 0. What
happens to a header-cloned skb that takes the else branch instead?

Suppose every slave neighbour is unresolved on the first pass of
teql_master_xmit(). Then neigh_event_send(n, NULL) fails, teql_resolve()
returns -EAGAIN and nores is set, and no unshare has run. teql then
restarts with the same skb as skb_res:

teql_master_xmit() {
    ...
	if (nores && skb_res == NULL) {
		skb_res = skb;
		rcu_read_unlock();
		goto restart;
	}
    ...
}

On the restart, neigh_event_send(n, skb_res) parks the skb on the slave
neighbour's arp_queue with its header still cloned:

net/core/neighbour.c:__neigh_event_send() {
    ...
			skb_dst_force(skb);
			__skb_queue_tail(&neigh->arp_queue, skb);
    ...
}

The usual reinjection path is covered. The skb's dst resolves to teql0's
own neighbour, whose output is neigh_direct_output(). That re-enters
teql_master_xmit(), where the new skb_cow_head() runs.

However, neigh_update_process_arp_queue() can fall back to a different
neighbour:

net/core/neighbour.c:neigh_update_process_arp_queue() {
    ...
		n2 = NULL;
		if (dst &&
		    READ_ONCE(dst->obsolete) != DST_OBSOLETE_DEAD) {
			n2 = dst_neigh_lookup_skb(dst, skb);
			if (n2)
				n1 = n2;
		}
		READ_ONCE(n1->output)(n1, skb);
    ...
}

This happens in two cases:

- The dst became DST_OBSOLETE_DEAD while the skb was parked (for example,
  a route delete that ends in dst_dev_put()), or dst_neigh_lookup_skb()
  fails. Then n1 stays the slave's neighbour.

- In the tc-mirred setup from the commit message, the dst belongs to the
  physical device, so n1 is that device's neighbour.

In both cases neigh_resolve_output() writes the link header without
checking skb_header_cloned():

net/core/neighbour.c:neigh_resolve_output() {
    ...
			__skb_pull(skb, skb_network_offset(skb));
			seq = read_seqbegin(&neigh->ha_lock);
			err = dev_hard_header(skb, dev, ntohs(skb->protocol),
					      neigh->ha, NULL, skb->len);
    ...
}

Can this overwrite the link header of the other clone, such as the mirred
original still pending on the physical device or a PF_PACKET tap clone? In
the mirred case the bytes written are usually identical, but they differ if
the lladdr changed in the meantime.

The commit message says "a header-cloned skb is unshared before the write",
but the arp_queue handoff does not seem to be covered.

The later patches in the series don't add an unshare on this path either:

- "net: neighbour: reinject a parked skb through a headerless output
  device" only rewrites skb->dev for a headerless n1.

- "net/sched: sch_teql: keep skb->dev consistent on the arp-queue path"
  only moves the skb->dev = slave assignment.

Would it work to call skb_cow_head() before neigh_event_send(n, skb_res),
or once at the top of teql_master_xmit()?

> +out:
>  	neigh_release(n);
>  	return err;
>  }

[ ... ]

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

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

[netdev-bot+sashiko]

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