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

On Tue, Oct 27, 2026 at 2:29 PM Simon Horman [off-list ref] 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/ (local)
Link: https://lore.kernel.org/netdev/20261023162821.GN708417@horms.kernel.org/ (local)

quoted

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.

quoted

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?

quoted

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

[ ... ]