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

[PATCH net v3 0/2] 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().

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 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
restores skb->dev at the arp-queue reinjection point so the parked skb goes
back through the master and leaves with a link header.

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

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:
Link: https://lore.kernel.org/netdev/QDISC-Y0BY.v2.r0.20261024060625-2@mojatatu.com/

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 (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 |  3 +++
 net/sched/sch_teql.c | 26 ++++++++++++++++++++------
 2 files changed, 23 insertions(+), 6 deletions(-)

-- 
2.43.0

[PATCH net v3 1/2] 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/
Cc: stable@vger.kernel.org
Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

---
 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) {

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

[Simon Horman]

On Tue, Nov 03, 2026 at 07:39:15AM -0500, Jamal Hadi Salim wrote:
> 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/
> Cc: stable@vger.kernel.org
> Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH net v3 2/2] 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, 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 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.

The restore is limited to a top-level neighbour whose device has no
header_ops, which is the shaper/eql/sch_teql class: those devices get
neigh_direct_output() from arp_constructor() and ndisc_constructor(), an
output that transmits on skb->dev and builds no link header, so the skb
has to be moved back.

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. The drain looks the neighbour
up from that dst and not from the forwarding device, so 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, not part of this series.

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. Moving skb->dev to the slave without restoring it at the
reinjection point leaves the packet going out v0 without a link header.

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.

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/
Cc: stable@vger.kernel.org
Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

---

v2 -> v3:
- Fix 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, and say which part of it reproduces
  without the unload.  Considered comments from Sashiko (nipa).
- Name the neighbour failure path that drains the same parked skb.

v1 -> v2:
- Limit the skb->dev restore in neigh_update_process_arp_queue() to a
  top-level neighbour whose device has no header_ops: shaper, eql and
  sch_teql output straight on skb->dev without building a link header, so
  the skb has to be moved back for them.
- Say how a master is freed and add the conditions to recreate the bug.
  Considered comments from Sashiko (nipa) and Sashiko (gemini).
---
 net/core/neighbour.c | 3 +++
 net/sched/sch_teql.c | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

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 outputs (teql, shaper) transmit on skb->dev */
+		if (!n1->dev->header_ops)
+			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 ed8faa848f18..4eac1f9c8ddf 100644
--- a/net/sched/sch_teql.c
+++ b/net/sched/sch_teql.c
@@ -368,13 +368,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);

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

[netdev-bot+sashiko]

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

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

[Jamal Hadi Salim]

On Sat, Nov 7, 2026 at 10:16 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> 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…
>

I think i missed responding to this one. Sashiko-nipa responses are
very erratic. For some threads (not mine) a review shows up 2-3 days
later. For mine the average time seems to be 7 days and sometimes a vN
review appears a few days later after a vN+1.

Summary: I will not be spinning anything for this series since there
are no new code change requirements. I have addressed the
neigh_update_process_arp_queue() issue as a separate (producer) issue
in the past; it's a separate net-next focusing on producers. The
comment (Low) i would have rectified if there was something meaningful
to make me respin.

cheers,
jamal

> --
>
> 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

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

[Simon Horman]

On Mon, Nov 09, 2026 at 06:44:09AM -0500, Jamal Hadi Salim wrote:
> On Sat, Nov 7, 2026 at 10:16 AM <netdev-bot+sashiko@kernel.org> wrote:
> >
> > 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…
> >
> 
> I think i missed responding to this one. Sashiko-nipa responses are
> very erratic. For some threads (not mine) a review shows up 2-3 days
> later. For mine the average time seems to be 7 days and sometimes a vN
> review appears a few days later after a vN+1.
> 
> Summary: I will not be spinning anything for this series since there
> are no new code change requirements. I have addressed the
> neigh_update_process_arp_queue() issue as a separate (producer) issue
> in the past; it's a separate net-next focusing on producers. The
> comment (Low) i would have rectified if there was something meaningful
> to make me respin.

Reviewed-by: Simon Horman <horms@kernel.org>

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

[Jakub Kicinski]

On Mon, 9 Nov 2026 06:44:09 -0500 Jamal Hadi Salim wrote:
> I think i missed responding to this one. Sashiko-nipa responses are
> very erratic. For some threads (not mine) a review shows up 2-3 days
> later. For mine the average time seems to be 7 days and sometimes a vN
> review appears a few days later after a vN+1.

Response time is based on contributor score, you are pretty deep in
the red given that you basically stopped reviewing patches from other
people, and keep mis-posting stuff.

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

[Jamal Hadi Salim]

On Tue, Nov 10, 2026 at 1:19 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 9 Nov 2026 06:44:09 -0500 Jamal Hadi Salim wrote:
> > I think i missed responding to this one. Sashiko-nipa responses are
> > very erratic. For some threads (not mine) a review shows up 2-3 days
> > later. For mine the average time seems to be 7 days and sometimes a vN
> > review appears a few days later after a vN+1.
>
> Response time is based on contributor score, you are pretty deep in
> the red given that you basically stopped reviewing patches from other
> people, and keep mis-posting stuff.

Yes, i just saw my grade, it's not good ;->
Mis-posting was for debugging what was happening;-> but I admit to not
reviewing.
Note: I still review credible submissions (such as Alexandre's) or if
one of you guys asked me to look at those (and what Victor posts as
well).

I stopped reviewing the drive-by AI patches because the returns
were/are low. My cost analysis shows reviewing and chasing someone,
testing, etc., takes about 5–6 times longer than knocking one of 100+
issues i have on my plate. Pre-AI there were much lower submissions
and i would review to encourage more contribution etc. But now it
costs someone 30 sec to send a patch and the count is just too high.
Worse, a lot of these patch authors just disappear when it comes down
to sending revision after revision. But my main motivation is a
majority of them now get their sources from sashiko "pre-existing
issues" which we are already collecting, analyzing and sending fixes
for.

My suggestion for the algorithm:
dont give authors that just submitted a high mark just because they
are new. "New contributor" scope has totally changed in meaning, I
would grade perseverance as a high mark. I just sent a v9 on one of
the patch series - and back and forth for over a month, I  feel i
deserve a better grade than someone posting a patch and disapearing.


cheers,
jamal

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

[Jakub Kicinski]

On Tue,  3 Nov 2026 07:39:16 -0500 Jamal Hadi Salim wrote:
> 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, 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 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.
> 
> The restore is limited to a top-level neighbour whose device has no
> header_ops, which is the shaper/eql/sch_teql class: those devices get
> neigh_direct_output() from arp_constructor() and ndisc_constructor(), an
> output that transmits on skb->dev and builds no link header, so the skb
> has to be moved back.
> 
> 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. The drain looks the neighbour
> up from that dst and not from the forwarding device, so 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, not part of this series.
> 
> 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. Moving skb->dev to the slave without restoring it at the
> reinjection point leaves the packet going out v0 without a link header.
> 
> 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.
> 
> 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/
> Cc: stable@vger.kernel.org
> Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> 
> ---
> 
> v2 -> v3:
> - Fix 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, and say which part of it reproduces
>   without the unload.  Considered comments from Sashiko (nipa).
> - Name the neighbour failure path that drains the same parked skb.
> 
> v1 -> v2:
> - Limit the skb->dev restore in neigh_update_process_arp_queue() to a
>   top-level neighbour whose device has no header_ops: shaper, eql and
>   sch_teql output straight on skb->dev without building a link header, so
>   the skb has to be moved back for them.
> - Say how a master is freed and add the conditions to recreate the bug.
>   Considered comments from Sashiko (nipa) and Sashiko (gemini).
> ---
>  net/core/neighbour.c | 3 +++
>  net/sched/sch_teql.c | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> 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 outputs (teql, shaper) transmit on skb->dev */
> +		if (!n1->dev->header_ops)
> +			skb->dev = n1->dev;
>  		READ_ONCE(n1->output)(n1, skb);
>  		if (n2)
>  			neigh_release(n2);

Looks odd, please split this out to its own fix, and make sure you CC
neigh maintainers.

> diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
> index ed8faa848f18..4eac1f9c8ddf 100644
> --- a/net/sched/sch_teql.c
> +++ b/net/sched/sch_teql.c
> @@ -368,13 +368,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);
-- 
pw-bot: cr