Re: [PATCH net v2 2/2] net/sched: sch_teql: fix use-after-free on lower qdisc drop
flat view
From: Simon Horman <horms@kernel.org>
Date: 2026-10-27 18:29:25
Also in:
stable
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/ (local) Link: https://lore.kernel.org/netdev/20261023162821.GN708417@horms.kernel.org/ (local)
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 hunk ↗ jump to hunk
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)
[ ... ]