[PATCH net v2 0/2] net/sched: sch_teql: fix transmit-path regressions

[PATCH net v2 0/2] net/sched: sch_teql: fix transmit-path regressions

[Jamal Hadi Salim]

v2: resend of the two-patch series.

- Patch 1/2 is unchanged

- Patch 2/2 now fixes the drop accounting raised by the Sashiko
  review of v1 (both providers): only NETDEV_TX_OK and NET_XMIT_CN
  count as accepted transmits (CN can accompany acceptance of the
  current packet); any other consumed result, e.g. NET_XMIT_DROP,
  is charged to tx_dropped instead of being counted as a good
  transmit.  The skb-ownership fix is unchanged (Sashiko-gemini).

The v2 revision follows the review of patch 2/2
(https://lore.kernel.org/netdev/179248763469.3135.1739559328868319628@kernel.org/)
and Simon Horman's request for a response
(https://lore.kernel.org/netdev/20261023162821.GN708417@horms.kernel.org/).

Two independent sch_teql regressions on the transmit path.

Patch 1 makes slaves_lock safe for the hard-IRQ-disabled netpoll path and
for PREEMPT_RT by converting it to a raw spinlock taken with IRQs saved.

Patch 2 fixes a use-after-free when a lower virtual device consumes an skb
but returns NET_XMIT_DROP rather than NETDEV_TX_OK, and stops counting
those consumed drops as good transmits.

Jamal Hadi Salim (1):
  net/sched: sch_teql: fix slaves_lock IRQ deadlock

owen kwan (1):
  net/sched: sch_teql: fix use-after-free on lower qdisc drop

 net/sched/sch_teql.c | 60 +++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 22 deletions(-)

-- 
2.43.0

[PATCH net v2 1/2] net/sched: sch_teql: fix slaves_lock IRQ deadlock

[Jamal Hadi Salim]

This is a followup of commit e5b811fe7931 ("net/sched: sch_teql:
Introduce slaves_lock to avoid race condition and UAF") as reported by
Sashiko.

The issue: master->slaves_lock serializes the master's ring of slave
qdiscs.  The control paths -- teql_dequeue(), teql_destroy() and
teql_qdisc_init() -- take it with spin_lock_bh(), while
teql_master_xmit() (the master's ndo_start_xmit) takes it with plain
spin_lock().

netpoll_send_skb() wraps the transmit in local_irq_save(), and
__netpoll_send_skb() asserts lockdep_assert_irqs_disabled(), so
teql_master_xmit() reached from netconsole always acquires slaves_lock
with hard IRQs disabled.  On PREEMPT_RT spinlock_t is an rtmutex, and
Documentation/locking/locktypes.rst names

	local_irq_disable();
	spin_lock(&lock);

as a sequence that breaks there because the rtmutex requires a fully
preemptible context.  This is reachable without any panic: the nbcon
printer thread calls netconsole's write_thread() callback on every
message.  The usual remedy -- folding the IRQ disable into the lock with
spin_lock_irqsave() -- is unavailable because the local_irq_save() is
netpoll's, not ours.

On non-RT the same asymmetry leaves slaves_lock acquirable with hard IRQs
disabled on one path and held with hard IRQs enabled on another.  That is
only exploitable during the panic console flush (netconsole is
CON_NBCON_ATOMIC_UNSAFE, so console_is_usable() admits its write_atomic()
only from nbcon_atomic_flush_unsafe(), which panic() alone calls), and
even then an IRQ-saving lock does not help if the holder oopsed inside the
section or was stopped by panic_other_cpus_shutdown().  So the non-RT half
is hardening; the PREEMPT_RT half is the bug.

The Fix: take slaves_lock as a raw spinlock with IRQs saved at every
site.  Since skb_queue_purge() takes the queue's spinlock_t and may not
nest inside a raw lock on PREEMPT_RT, move its call in teql_destroy()
outside the raw section; dat->q is the dying qdisc's own queue and was
never serialized by slaves_lock, so the move is behaviour-neutral.

The new critical sections contain only RCU pointer assignments and
netdev_get_tx_queue(); skb_queue_purge(), netif_wake_queue() and
dev_reset_queue() are all outside them.  teql_destroy()'s ring walk is
O(slaves), two pointer loads and a compare per iteration, so even a large
ring is microseconds with IRQs off.  Keeping the purge conditional rather
than dropping it outright is the conservative call for a backport.

Some context: commit 6301f6a34ed8 ("net/sched: sch_teql: move
rcu_read_lock()/spin_lock() from _bh variants") hardened the parent
fix; together they addressed the master->slaves resurrection
use-after-free and the xmit-side BH warning but left the reverse
direction -- a hard IRQ re-entering xmit while a control path holds the
lock -- unaddressed.

Conditions to recreate the bug:
- CONFIG_PREEMPT_RT, CONFIG_NET_SCH_TEQL, CONFIG_DUMMY and
  CONFIG_NETCONSOLE.
- Run `ip link add dev teqlslave0 type dummy`, bring teqlslave0 up, attach
  it with `tc qdisc add dev teqlslave0 root teql0`, and bring teql0 up.
- Configure a netconsole target on teql0 with explicit local and remote
  IP addresses and a remote MAC address, then emit a console message.
  The nbcon printer thread reaches teql_master_xmit() with hard IRQs
  disabled by netpoll; taking slaves_lock as a spinlock_t is invalid on
  PREEMPT_RT. No panic or concurrent qdisc change is required.
- Reachable by an admin with CAP_NET_ADMIN; teql devices exist only in the
  initial netns (no per-netns ops), so a user namespace does not suffice.

Fixes: e5b811fe7931 ("net/sched: sch_teql: Introduce slaves_lock to avoid race condition and UAF")
Reported-by: Sashiko (gemini) <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260630150922.238714-1-jhs%40mojatatu.com
Link: https://lore.kernel.org/netdev/20260630150922.238714-1-jhs@mojatatu.com/
Cc: stable@vger.kernel.org
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---

 net/sched/sch_teql.c | 45 +++++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
index 409ce50cc0db..c7dcec25ef42 100644
--- a/net/sched/sch_teql.c
+++ b/net/sched/sch_teql.c
@@ -53,7 +53,7 @@ struct teql_master {
 	struct Qdisc_ops qops;
 	struct net_device *dev;
 	struct Qdisc __rcu	*slaves;
-	spinlock_t		slaves_lock; /* serializes writes to ->slaves */
+	raw_spinlock_t		slaves_lock; /* serializes writes to ->slaves */
 	struct list_head master_list;
 	unsigned long	tx_bytes;
 	unsigned long	tx_packets;
@@ -92,6 +92,7 @@ teql_dequeue(struct Qdisc *sch)
 {
 	struct teql_sched_data *dat = qdisc_priv(sch);
 	struct netdev_queue *dat_queue;
+	unsigned long flags;
 	struct sk_buff *skb;
 	struct Qdisc *q;
 
@@ -102,9 +103,9 @@ teql_dequeue(struct Qdisc *sch)
 	if (skb == NULL) {
 		struct net_device *m = qdisc_dev(q);
 		if (m) {
-			spin_lock_bh(&dat->m->slaves_lock);
+			raw_spin_lock_irqsave(&dat->m->slaves_lock, flags);
 			rcu_assign_pointer(dat->m->slaves, sch);
-			spin_unlock_bh(&dat->m->slaves_lock);
+			raw_spin_unlock_irqrestore(&dat->m->slaves_lock, flags);
 			netif_wake_queue(m);
 		}
 	} else {
@@ -137,11 +138,13 @@ teql_destroy(struct Qdisc *sch)
 	struct teql_master *master = dat->m;
 	struct netdev_queue *txq = NULL;
 	bool reset_master_queue = false;
+	unsigned long flags;
+	bool purge = false;
 
 	if (!master)
 		return;
 
-	spin_lock_bh(&master->slaves_lock);
+	raw_spin_lock_irqsave(&master->slaves_lock, flags);
 	prev = rcu_dereference_protected(master->slaves,
 					 lockdep_is_held(&master->slaves_lock));
 	if (prev) {
@@ -169,12 +172,18 @@ teql_destroy(struct Qdisc *sch)
 					reset_master_queue = true;
 				}
 			}
-			skb_queue_purge(&dat->q);
+			purge = true;
 			break;
 		} while (prev != rcu_dereference_protected(master->slaves,
 							   lockdep_is_held(&master->slaves_lock)));
 	}
-	spin_unlock_bh(&master->slaves_lock);
+	raw_spin_unlock_irqrestore(&master->slaves_lock, flags);
+
+	/* skb_queue_purge() takes the queue's spinlock_t, which cannot nest
+	 * inside a raw lock on PREEMPT_RT; purge after the raw section.
+	 */
+	if (purge)
+		skb_queue_purge(&dat->q);
 
 	if (reset_master_queue)
 		dev_reset_queue(master->dev, txq, NULL);
@@ -187,6 +196,7 @@ static int teql_qdisc_init(struct Qdisc *sch, struct nlattr *opt,
 	struct teql_master *m = (struct teql_master *)sch->ops;
 	struct teql_sched_data *q = qdisc_priv(sch);
 	struct Qdisc *first;
+	unsigned long flags;
 
 	if (dev->hard_header_len > m->dev->hard_header_len)
 		return -EINVAL;
@@ -203,7 +213,7 @@ static int teql_qdisc_init(struct Qdisc *sch, struct nlattr *opt,
 
 	skb_queue_head_init(&q->q);
 
-	spin_lock_bh(&m->slaves_lock);
+	raw_spin_lock_irqsave(&m->slaves_lock, flags);
 	first = rcu_dereference_protected(m->slaves, lockdep_is_held(&m->slaves_lock));
 	if (first) {
 		if (m->dev->flags & IFF_UP) {
@@ -214,7 +224,7 @@ static int teql_qdisc_init(struct Qdisc *sch, struct nlattr *opt,
 			    (m->dev->flags & IFF_MULTICAST &&
 			     !(dev->flags & IFF_MULTICAST)) ||
 			    dev->mtu < m->dev->mtu) {
-				spin_unlock_bh(&m->slaves_lock);
+				raw_spin_unlock_irqrestore(&m->slaves_lock, flags);
 				return -EINVAL;
 			}
 		} else {
@@ -237,7 +247,7 @@ static int teql_qdisc_init(struct Qdisc *sch, struct nlattr *opt,
 		m->dev->mtu = dev->mtu;
 		m->dev->flags = (m->dev->flags&~FMASK)|(dev->flags&FMASK);
 	}
-	spin_unlock_bh(&m->slaves_lock);
+	raw_spin_unlock_irqrestore(&m->slaves_lock, flags);
 	return 0;
 }
 
@@ -302,11 +312,12 @@ static inline int teql_resolve(struct sk_buff *skb,
 static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct teql_master *master = netdev_priv(dev);
-	struct Qdisc *start, *q;
-	int busy;
-	int nores;
 	int subq = skb_get_queue_mapping(skb);
 	struct sk_buff *skb_res = NULL;
+	struct Qdisc *start, *q;
+	unsigned long flags;
+	int nores;
+	int busy;
 
 restart:
 	nores = 0;
@@ -342,13 +353,13 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev)
 				    netdev_start_xmit(skb, slave, slave_txq, false) ==
 				    NETDEV_TX_OK) {
 					__netif_tx_unlock(slave_txq);
-					spin_lock(&master->slaves_lock);
+					raw_spin_lock_irqsave(&master->slaves_lock, flags);
 					if (rcu_dereference_protected(master->slaves,
 								      lockdep_is_held(&master->slaves_lock)) == q)
 						rcu_assign_pointer(master->slaves,
 								   rcu_dereference_protected(NEXT_SLAVE(q),
 											     lockdep_is_held(&master->slaves_lock)));
-					spin_unlock(&master->slaves_lock);
+					raw_spin_unlock_irqrestore(&master->slaves_lock, flags);
 					netif_wake_queue(dev);
 					master->tx_packets++;
 					master->tx_bytes += length;
@@ -361,13 +372,13 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev)
 				busy = 1;
 			break;
 		case 1:
-			spin_lock(&master->slaves_lock);
+			raw_spin_lock_irqsave(&master->slaves_lock, flags);
 			if (rcu_dereference_protected(master->slaves,
 						      lockdep_is_held(&master->slaves_lock)) == q)
 				rcu_assign_pointer(master->slaves,
 						   rcu_dereference_protected(NEXT_SLAVE(q),
 									     lockdep_is_held(&master->slaves_lock)));
-			spin_unlock(&master->slaves_lock);
+			raw_spin_unlock_irqrestore(&master->slaves_lock, flags);
 			rcu_read_unlock();
 			return NETDEV_TX_OK;
 		default:
@@ -489,7 +500,7 @@ static __init void teql_master_setup(struct net_device *dev)
 	struct teql_master *master = netdev_priv(dev);
 	struct Qdisc_ops *ops = &master->qops;
 
-	spin_lock_init(&master->slaves_lock);
+	raw_spin_lock_init(&master->slaves_lock);
 	master->dev	= dev;
 	ops->priv_size  = sizeof(struct teql_sched_data);
 
-- 
2.43.0

[PATCH net v2 2/2] net/sched: sch_teql: fix use-after-free on lower qdisc drop

[Jamal Hadi Salim]

From: owen kwan <owen@verialabs.com>

The issue: teql_master_xmit() tests the slave's ndo_start_xmit() return
value against NETDEV_TX_OK alone. A slave that consumes the skb but
returns an error code (rc < NET_XMIT_MASK, e.g. NET_XMIT_DROP) is treated
as if the skb were still owned, so the loop restores skb->dev and calls
__skb_pull() on it. A VLAN slave returns the result of dev_queue_xmit()
from its ndo_start_xmit() (propagated since cbbef5e18307), and when the
lower qdisc drops the packet that value is NET_XMIT_DROP after the skb
has been freed, giving a use-after-free in teql_master_xmit().

The Fix: test for consumption with dev_xmit_complete(), which accepts
NETDEV_TX_OK and every rc < NET_XMIT_MASK, matching other callers that
need to distinguish consumed results from busy. Only NETDEV_TX_OK keeps
the packet counted in tx_packets/tx_bytes and advances the slave; a
consumed result that is neither a success nor NET_XMIT_CN ends the
attempt and is charged to tx_dropped, so lower-qdisc drops are no longer
reported as good transmits. NET_XMIT_CN is counted like success because
it can accompany acceptance of the current packet: pfifo_head_drop drops
the old head, enqueues the current skb and returns NET_XMIT_CN.

Conditions to recreate the bug:
With CONFIG_NET_SCH_TEQL,
CONFIG_VLAN_8021Q and CONFIG_DUMMY enabled, add lkrdummy0 (dummy) and
lkrvlan0 (VLAN id 37 on lkrdummy0); set a pfifo limit 0 root on lkrdummy0
and a teql0 root on lkrvlan0; bring teql0 up with an address and send one
UDP packet to it. The VLAN xmit returns NET_XMIT_DROP from the lower
qdisc while holding a freed skb.

Fixes: 572a9d7b6fc7 ("net: allow to propagate errors through ->ndo_hard_start_xmit()")
Link: https://lore.kernel.org/netdev/179248763469.3135.1739559328868319628@kernel.org/
Link: https://lore.kernel.org/netdev/20261023162821.GN708417@horms.kernel.org/
Cc: stable@vger.kernel.org
Signed-off-by: owen kwan <owen@verialabs.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

---
v1 -> v2 (feedback by Sashiko-gemini):
- Save the slave ndo_start_xmit() result once; use
  dev_xmit_complete() only to decide whether the skb was consumed.
- Count tx_packets/tx_bytes and advance the slave only for
  NETDEV_TX_OK; charge other consumed results to tx_dropped, so
  lower-qdisc drops are not reported as good transmits.  NET_XMIT_CN
  is counted like success: it can accompany acceptance of the current
  packet.
- Describe the counter behaviour in the commit message.
---
 net/sched/sch_teql.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
index c7dcec25ef42..b60ab79a6572 100644
--- a/net/sched/sch_teql.c
+++ b/net/sched/sch_teql.c
@@ -347,12 +347,18 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev)
 		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) &&
-				    netdev_start_xmit(skb, slave, slave_txq, false) ==
-				    NETDEV_TX_OK) {
-					__netif_tx_unlock(slave_txq);
+				if (!netif_xmit_frozen_or_stopped(slave_txq))
+					rc = netdev_start_xmit(skb, slave, slave_txq, false);
+				__netif_tx_unlock(slave_txq);
+				if (dev_xmit_complete(rc)) {
+					if (rc != NETDEV_TX_OK && rc != NET_XMIT_CN) {
+						master->tx_dropped++;
+						rcu_read_unlock();
+						return NETDEV_TX_OK;
+					}
 					raw_spin_lock_irqsave(&master->slaves_lock, flags);
 					if (rcu_dereference_protected(master->slaves,
 								      lockdep_is_held(&master->slaves_lock)) == q)
@@ -366,7 +372,6 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev)
 					rcu_read_unlock();
 					return NETDEV_TX_OK;
 				}
-				__netif_tx_unlock(slave_txq);
 			}
 			if (netif_xmit_stopped(netdev_get_tx_queue(dev, 0)))
 				busy = 1;
-- 
2.34.1

Re: [PATCH net v2 2/2] net/sched: sch_teql: fix use-after-free on lower qdisc drop

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-Y0BY.v2.r0.20261024060625-1@mojatatu.com?part=2
---
net/sched: sch_teql: fix use-after-free on lower qdisc drop

This patch changes teql_master_xmit() to use dev_xmit_complete() to decide
whether a slave consumed the skb. Before, a slave such as VLAN could free
the skb and return NET_XMIT_DROP, and the loop would keep using the freed
skb. Consumed results other than NETDEV_TX_OK and NET_XMIT_CN are now
charged to tx_dropped, and the function returns.

Link: https://lore.kernel.org/netdev/179248763469.3135.1739559328868319628@kernel.org/
Link: https://lore.kernel.org/netdev/20261023162821.GN708417@horms.kernel.org/

> Only NETDEV_TX_OK keeps
> the packet counted in tx_packets/tx_bytes and advances the slave; a
> consumed result that is neither a success nor NET_XMIT_CN ends the
> attempt and is charged to tx_dropped, so lower-qdisc drops are no longer
> reported as good transmits. NET_XMIT_CN is counted like success because
> it can accompany acceptance of the current packet: pfifo_head_drop drops
> the old head, enqueues the current skb and returns NET_XMIT_CN.

[Severity: Low]
Is this description accurate? The code sends NET_XMIT_CN down the same
path as NETDEV_TX_OK. So NET_XMIT_CN also advances master->slaves and
increments master->tx_packets and master->tx_bytes. The "Only
NETDEV_TX_OK" sentence seems to contradict the NET_XMIT_CN sentence that
follows it, and the v1 -> v2 notes say the same thing.

Several qdiscs also drop the current skb and still return NET_XMIT_CN.
One example is noop_enqueue():

	dev_core_stats_tx_dropped_inc(skb->dev);
	__qdisc_drop(skb, to_free);
	return NET_XMIT_CN;

Another is red_enqueue():

	qdisc_drop_reason(skb, sch, to_free, reason);
	return NET_XMIT_CN;

choke, gred, fq_pie and sfb behave the same way. When a VLAN or macvlan
slave sits on top of one of these, teql would still count those drops as
good transmits in tx_packets and tx_bytes.

Treating NET_XMIT_CN as success matches net_xmit_eval() and
vlan_dev_hard_start_xmit(), so the code choice is probably fine. Could
the commit message drop the "Only NETDEV_TX_OK" wording? It also says
that lower-qdisc drops "are no longer reported as good transmits", which
does not hold for these qdiscs.

> diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
> index 6764f0dd38dfb..70c948a26f809 100644
> --- a/net/sched/sch_teql.c
> +++ b/net/sched/sch_teql.c
> @@ -360,12 +360,18 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev)
>  		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) &&
> -				    netdev_start_xmit(skb, slave, slave_txq, false) ==
> -				    NETDEV_TX_OK) {
> -					__netif_tx_unlock(slave_txq);
> +				if (!netif_xmit_frozen_or_stopped(slave_txq))
> +					rc = netdev_start_xmit(skb, slave, slave_txq, false);
> +				__netif_tx_unlock(slave_txq);
> +				if (dev_xmit_complete(rc)) {
> +					if (rc != NETDEV_TX_OK && rc != NET_XMIT_CN) {
> +						master->tx_dropped++;
> +						rcu_read_unlock();
> +						return NETDEV_TX_OK;
> +					}

[Severity: Medium]
Should this drop path also move master->slaves on to NEXT_SLAVE(q)
before it returns?

Only the success block below and the case 1 path rotate master->slaves.
Every call to teql_master_xmit() starts from the same pointer:

	start = rcu_dereference(master->slaves);

If a slave keeps returning NET_XMIT_DROP or a negative errno, each later
packet would go to that slave first and be dropped. The other slaves
would never be tried.

A VLAN slave can do this for a long time. vlan_dev_hard_start_xmit()
returns the raw dev_queue_xmit() result and never stops its own queue:

	ret = dev_queue_xmit(skb);
	...
	return ret;

So neither the netif_xmit_stopped() check nor the netif_running() check
would skip it. macvlan_start_xmit() behaves the same way. This can
happen when the lower device has a pfifo with limit 0, a full fifo or
tbf, or an egress tc or netfilter drop.

When a slave is busy, teql retries the same skb on the next slave. A
dropped skb can't be retried, so without rotation the drops build up on
one slave. If that slave drops every packet, is all traffic through the
teql master lost?

The case 1 path already rotates master->slaves after the skb is consumed
by neighbour queueing:

	case 1:
		raw_spin_lock_irqsave(&master->slaves_lock, flags);
		if (rcu_dereference_protected(master->slaves, ...) == q)
			rcu_assign_pointer(master->slaves, ... NEXT_SLAVE(q) ...);

Would doing the same rotation here keep traffic spread across the slaves?

>  					raw_spin_lock_irqsave(&master->slaves_lock, flags);
>  					if (rcu_dereference_protected(master->slaves,
>  								      lockdep_is_held(&master->slaves_lock)) == q)

[ ... ]

Re: [PATCH net v2 2/2] net/sched: sch_teql: fix use-after-free on lower qdisc drop

[Jamal Hadi Salim]

On Tue, Oct 27, 2026 at 2:29 PM Simon Horman <horms@kernel.org> wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-Y0BY.v2.r0.20261024060625-1@mojatatu.com?part=2

So this email was stuck at sashiko because the email fields wouldnt
allow it. Set at:
email_status: Muted, smtp_enabled: false
The same with patch 2/2

I will respond to both here. Summary is: i will not be sending an update.

For patch [1/2] net/sched: sch_teql: fix slaves_lock IRQ deadlock
The High is pre-existing and not introduced by this patch, which only
converts slaves_lock to a raw spinlock.
I have a separate fix that bounds the walk by a slave_count maintained
under slaves_lock; it will be posted after this  series gets merged.

Patch [2/2] net/sched: sch_teql: fix use-after-free on lower qdisc drop:

1) Low: agreed, the changelog is misleading (the code is intended as written).
NETDEV_TX_OK and NET_XMIT_CN are both counted as good transmits,
matching  net_xmit_eval(); any other consumed result is charged to
tx_dropped.
I don't think it warrants a respin on its own unless something even
semi-serious shows up in the code.

2) Medium: the non-rotation is pre-existing.  Before this patch the
drop path never advanced master->slaves either; it just retried the
freed skb, which is the use-after-free being fixed.
Advancing to NEXT_SLAVE(q) here is a  load-distribution improvement,
not a correctness requirement of this fix.
 IMO:   I would rather not inclued a policy change into the UAF fix; I
can send it  separately.


cheers,
jamal
> net/sched: sch_teql: fix use-after-free on lower qdisc drop
>
> This patch changes teql_master_xmit() to use dev_xmit_complete() to decide
> whether a slave consumed the skb. Before, a slave such as VLAN could free
> the skb and return NET_XMIT_DROP, and the loop would keep using the freed
> skb. Consumed results other than NETDEV_TX_OK and NET_XMIT_CN are now
> charged to tx_dropped, and the function returns.
>
> Link: https://lore.kernel.org/netdev/179248763469.3135.1739559328868319628@kernel.org/
> Link: https://lore.kernel.org/netdev/20261023162821.GN708417@horms.kernel.org/
>
> > Only NETDEV_TX_OK keeps
> > the packet counted in tx_packets/tx_bytes and advances the slave; a
> > consumed result that is neither a success nor NET_XMIT_CN ends the
> > attempt and is charged to tx_dropped, so lower-qdisc drops are no longer
> > reported as good transmits. NET_XMIT_CN is counted like success because
> > it can accompany acceptance of the current packet: pfifo_head_drop drops
> > the old head, enqueues the current skb and returns NET_XMIT_CN.
>
> [Severity: Low]
> Is this description accurate? The code sends NET_XMIT_CN down the same
> path as NETDEV_TX_OK. So NET_XMIT_CN also advances master->slaves and
> increments master->tx_packets and master->tx_bytes. The "Only
> NETDEV_TX_OK" sentence seems to contradict the NET_XMIT_CN sentence that
> follows it, and the v1 -> v2 notes say the same thing.
>
> Several qdiscs also drop the current skb and still return NET_XMIT_CN.
> One example is noop_enqueue():
>
>         dev_core_stats_tx_dropped_inc(skb->dev);
>         __qdisc_drop(skb, to_free);
>         return NET_XMIT_CN;
>
> Another is red_enqueue():
>
>         qdisc_drop_reason(skb, sch, to_free, reason);
>         return NET_XMIT_CN;
>
> choke, gred, fq_pie and sfb behave the same way. When a VLAN or macvlan
> slave sits on top of one of these, teql would still count those drops as
> good transmits in tx_packets and tx_bytes.
>
> Treating NET_XMIT_CN as success matches net_xmit_eval() and
> vlan_dev_hard_start_xmit(), so the code choice is probably fine. Could
> the commit message drop the "Only NETDEV_TX_OK" wording? It also says
> that lower-qdisc drops "are no longer reported as good transmits", which
> does not hold for these qdiscs.
>
> > diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
> > index 6764f0dd38dfb..70c948a26f809 100644
> > --- a/net/sched/sch_teql.c
> > +++ b/net/sched/sch_teql.c
> > @@ -360,12 +360,18 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev)
> >               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) &&
> > -                                 netdev_start_xmit(skb, slave, slave_txq, false) ==
> > -                                 NETDEV_TX_OK) {
> > -                                     __netif_tx_unlock(slave_txq);
> > +                             if (!netif_xmit_frozen_or_stopped(slave_txq))
> > +                                     rc = netdev_start_xmit(skb, slave, slave_txq, false);
> > +                             __netif_tx_unlock(slave_txq);
> > +                             if (dev_xmit_complete(rc)) {
> > +                                     if (rc != NETDEV_TX_OK && rc != NET_XMIT_CN) {
> > +                                             master->tx_dropped++;
> > +                                             rcu_read_unlock();
> > +                                             return NETDEV_TX_OK;
> > +                                     }
>
> [Severity: Medium]
> Should this drop path also move master->slaves on to NEXT_SLAVE(q)
> before it returns?
>
> Only the success block below and the case 1 path rotate master->slaves.
> Every call to teql_master_xmit() starts from the same pointer:
>
>         start = rcu_dereference(master->slaves);
>
> If a slave keeps returning NET_XMIT_DROP or a negative errno, each later
> packet would go to that slave first and be dropped. The other slaves
> would never be tried.
>
> A VLAN slave can do this for a long time. vlan_dev_hard_start_xmit()
> returns the raw dev_queue_xmit() result and never stops its own queue:
>
>         ret = dev_queue_xmit(skb);
>         ...
>         return ret;
>
> So neither the netif_xmit_stopped() check nor the netif_running() check
> would skip it. macvlan_start_xmit() behaves the same way. This can
> happen when the lower device has a pfifo with limit 0, a full fifo or
> tbf, or an egress tc or netfilter drop.
>
> When a slave is busy, teql retries the same skb on the next slave. A
> dropped skb can't be retried, so without rotation the drops build up on
> one slave. If that slave drops every packet, is all traffic through the
> teql master lost?
>
> The case 1 path already rotates master->slaves after the skb is consumed
> by neighbour queueing:
>
>         case 1:
>                 raw_spin_lock_irqsave(&master->slaves_lock, flags);
>                 if (rcu_dereference_protected(master->slaves, ...) == q)
>                         rcu_assign_pointer(master->slaves, ... NEXT_SLAVE(q) ...);
>
> Would doing the same rotation here keep traffic spread across the slaves?
>
> >                                       raw_spin_lock_irqsave(&master->slaves_lock, flags);
> >                                       if (rcu_dereference_protected(master->slaves,
> >                                                                     lockdep_is_held(&master->slaves_lock)) == q)
>
> [ ... ]

Re: [PATCH net v2 2/2] net/sched: sch_teql: fix use-after-free on lower qdisc drop

[Simon Horman]

On Tue, Oct 27, 2026 at 05:19:26PM -0400, Jamal Hadi Salim wrote:
> On Tue, Oct 27, 2026 at 2:29 PM Simon Horman <horms@kernel.org> wrote:
> >
> > This is an AI-generated review of your patch. The human sending this
> > email has considered the AI review valid, or at least plausible.
> > Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-Y0BY.v2.r0.20261024060625-1@mojatatu.com?part=2
> 
> So this email was stuck at sashiko because the email fields wouldnt
> allow it. Set at:
> email_status: Muted, smtp_enabled: false
> The same with patch 2/2

Fun times in AI-land.

> I will respond to both here. Summary is: i will not be sending an update.
> 
> For patch [1/2] net/sched: sch_teql: fix slaves_lock IRQ deadlock
> The High is pre-existing and not introduced by this patch, which only
> converts slaves_lock to a raw spinlock.
> I have a separate fix that bounds the walk by a slave_count maintained
> under slaves_lock; it will be posted after this  series gets merged.
> 
> Patch [2/2] net/sched: sch_teql: fix use-after-free on lower qdisc drop:
> 
> 1) Low: agreed, the changelog is misleading (the code is intended as written).
> NETDEV_TX_OK and NET_XMIT_CN are both counted as good transmits,
> matching  net_xmit_eval(); any other consumed result is charged to
> tx_dropped.
> I don't think it warrants a respin on its own unless something even
> semi-serious shows up in the code.
> 
> 2) Medium: the non-rotation is pre-existing.  Before this patch the
> drop path never advanced master->slaves either; it just retried the
> freed skb, which is the use-after-free being fixed.
> Advancing to NEXT_SLAVE(q) here is a  load-distribution improvement,
> not a correctness requirement of this fix.
>  IMO:   I would rather not inclued a policy change into the UAF fix; I
> can send it  separately.

Thanks for the feedback.
I agree with your suggested approaches.

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

Re: [PATCH net v2 1/2] net/sched: sch_teql: fix slaves_lock IRQ deadlock

[Simon Horman]

On Sat, Oct 24, 2026 at 10:34:14AM -0400, Jamal Hadi Salim wrote:
> This is a followup of commit e5b811fe7931 ("net/sched: sch_teql:
> Introduce slaves_lock to avoid race condition and UAF") as reported by
> Sashiko.
> 
> The issue: master->slaves_lock serializes the master's ring of slave
> qdiscs.  The control paths -- teql_dequeue(), teql_destroy() and
> teql_qdisc_init() -- take it with spin_lock_bh(), while
> teql_master_xmit() (the master's ndo_start_xmit) takes it with plain
> spin_lock().
> 
> netpoll_send_skb() wraps the transmit in local_irq_save(), and
> __netpoll_send_skb() asserts lockdep_assert_irqs_disabled(), so
> teql_master_xmit() reached from netconsole always acquires slaves_lock
> with hard IRQs disabled.  On PREEMPT_RT spinlock_t is an rtmutex, and
> Documentation/locking/locktypes.rst names
> 
> 	local_irq_disable();
> 	spin_lock(&lock);
> 
> as a sequence that breaks there because the rtmutex requires a fully
> preemptible context.  This is reachable without any panic: the nbcon
> printer thread calls netconsole's write_thread() callback on every
> message.  The usual remedy -- folding the IRQ disable into the lock with
> spin_lock_irqsave() -- is unavailable because the local_irq_save() is
> netpoll's, not ours.
> 
> On non-RT the same asymmetry leaves slaves_lock acquirable with hard IRQs
> disabled on one path and held with hard IRQs enabled on another.  That is
> only exploitable during the panic console flush (netconsole is
> CON_NBCON_ATOMIC_UNSAFE, so console_is_usable() admits its write_atomic()
> only from nbcon_atomic_flush_unsafe(), which panic() alone calls), and
> even then an IRQ-saving lock does not help if the holder oopsed inside the
> section or was stopped by panic_other_cpus_shutdown().  So the non-RT half
> is hardening; the PREEMPT_RT half is the bug.
> 
> The Fix: take slaves_lock as a raw spinlock with IRQs saved at every
> site.  Since skb_queue_purge() takes the queue's spinlock_t and may not
> nest inside a raw lock on PREEMPT_RT, move its call in teql_destroy()
> outside the raw section; dat->q is the dying qdisc's own queue and was
> never serialized by slaves_lock, so the move is behaviour-neutral.
> 
> The new critical sections contain only RCU pointer assignments and
> netdev_get_tx_queue(); skb_queue_purge(), netif_wake_queue() and
> dev_reset_queue() are all outside them.  teql_destroy()'s ring walk is
> O(slaves), two pointer loads and a compare per iteration, so even a large
> ring is microseconds with IRQs off.  Keeping the purge conditional rather
> than dropping it outright is the conservative call for a backport.
> 
> Some context: commit 6301f6a34ed8 ("net/sched: sch_teql: move
> rcu_read_lock()/spin_lock() from _bh variants") hardened the parent
> fix; together they addressed the master->slaves resurrection
> use-after-free and the xmit-side BH warning but left the reverse
> direction -- a hard IRQ re-entering xmit while a control path holds the
> lock -- unaddressed.
> 
> Conditions to recreate the bug:
> - CONFIG_PREEMPT_RT, CONFIG_NET_SCH_TEQL, CONFIG_DUMMY and
>   CONFIG_NETCONSOLE.
> - Run `ip link add dev teqlslave0 type dummy`, bring teqlslave0 up, attach
>   it with `tc qdisc add dev teqlslave0 root teql0`, and bring teql0 up.
> - Configure a netconsole target on teql0 with explicit local and remote
>   IP addresses and a remote MAC address, then emit a console message.
>   The nbcon printer thread reaches teql_master_xmit() with hard IRQs
>   disabled by netpoll; taking slaves_lock as a spinlock_t is invalid on
>   PREEMPT_RT. No panic or concurrent qdisc change is required.
> - Reachable by an admin with CAP_NET_ADMIN; teql devices exist only in the
>   initial netns (no per-netns ops), so a user namespace does not suffice.
> 
> Fixes: e5b811fe7931 ("net/sched: sch_teql: Introduce slaves_lock to avoid race condition and UAF")
> Reported-by: Sashiko (gemini) <sashiko-bot@kernel.org>
> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260630150922.238714-1-jhs%40mojatatu.com
> Link: https://lore.kernel.org/netdev/20260630150922.238714-1-jhs@mojatatu.com/
> Cc: stable@vger.kernel.org
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

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

Re: [PATCH net v2 0/2] net/sched: sch_teql: fix transmit-path regressions

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

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

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 v2 0/2] net/sched: sch_teql: fix transmit-path regressions

[Jamal Hadi Salim]

Patch 1 is a followup of a report by sashiko and  two was an
independent bug reported by Owen that our reproduction system verified
- i believe they found it by using an LLM to look for similar patterns
of some previously fixed bug.

cheers,
jamal

On Sat, Oct 24, 2026 at 10:36 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:
>
>  - How the issue was discovered, e.g. hit in production, hit during
>    development, syzbot report, manual code inspection, LLM or static
>    analysis tool scan.
>
> 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 v2 0/2] net/sched: sch_teql: fix transmit-path regressions

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Sat, 24 Oct 2026 10:34:13 -0400 you wrote:
> v2: resend of the two-patch series.
> 
> - Patch 1/2 is unchanged
> 
> - Patch 2/2 now fixes the drop accounting raised by the Sashiko
>   review of v1 (both providers): only NETDEV_TX_OK and NET_XMIT_CN
>   count as accepted transmits (CN can accompany acceptance of the
>   current packet); any other consumed result, e.g. NET_XMIT_DROP,
>   is charged to tx_dropped instead of being counted as a good
>   transmit.  The skb-ownership fix is unchanged (Sashiko-gemini).
> 
> [...]

Here is the summary with links:
  - [net,v2,1/2] net/sched: sch_teql: fix slaves_lock IRQ deadlock
    https://git.kernel.org/netdev/net/c/8b2043279dae
  - [net,v2,2/2] net/sched: sch_teql: fix use-after-free on lower qdisc drop
    https://git.kernel.org/netdev/net/c/782d6bb8f9b8

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html