# Re: \[PATCH net v2 2/2\] net/sched: sch\_teql: fix use-after-free on lower qdisc drop - From: Jamal Hadi Salim \ - Date: 2026-10-27 21:19:38 - List: netdev - Page: https://ratatoskr.run/netdev/2026/10/17813446 - Thread: https://ratatoskr.run/netdev/2026/10/17798328/t.md ```text On Tue, Oct 27, 2026 at 2:29 PM Simon Horman [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/ > 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) > > [ ... ] ```