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

[Simon Horman <horms@kernel.org>]

On Tue, Oct 20, 2026 at 09:13:54AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
> 
> 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] In teql_master_xmit() (net/sched/sch_teql.c), the patch
>   replaces `== NETDEV_TX_OK` with `dev_xmit_complete(rc)`.

Hi Jamal,

Maybe things have moved on for some reason.
But this concern does seem to warrant a response.

> 
> --
> 
> net/sched: sch_teql: fix use-after-free on lower qdisc drop
> 
> This patch changes the check on the slave's netdev_start_xmit() return
> value in teql_master_xmit(). It now uses dev_xmit_complete() instead of
> comparing against NETDEV_TX_OK, so a slave that consumes the skb and
> returns NET_XMIT_DROP is no longer treated as still owning it. This fixes
> a use-after-free when a VLAN slave's lower qdisc drops the packet.
> 
> > diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
> > index c7dcec25ef420..b3efd5c281988 100644
> > --- a/net/sched/sch_teql.c
> > +++ b/net/sched/sch_teql.c
> > @@ -350,8 +350,8 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev)
> >  
> >  				skb->dev = slave;
> >  				if (!netif_xmit_frozen_or_stopped(slave_txq) &&
> > -				    netdev_start_xmit(skb, slave, slave_txq, false) ==
> > -				    NETDEV_TX_OK) {
> > +				    dev_xmit_complete(netdev_start_xmit(skb, slave,
> > +									slave_txq, false))) {
> 
> [Severity: Medium]
> This closes the use-after-free. Does the same condition now also count
> dropped packets as successful transmits on teql0?
> 
> dev_xmit_complete() returns true for any rc < NET_XMIT_MASK. That covers
> NET_XMIT_DROP and negative errno values as well as NETDEV_TX_OK. All of
> these now take the success block in teql_master_xmit():
> 
>     netif_wake_queue(dev);
>     master->tx_packets++;
>     master->tx_bytes += length;
>     rcu_read_unlock();
>     return NETDEV_TX_OK;
> 
> They also advance master->slaves to the next slave.
> 
> teql_master_stats64() copies these counters directly into struct
> rtnl_link_stats64. There, tx_packets is defined as "Number of packets
> successfully transmitted".
> 
> With the reproducer from the commit message (a VLAN slave on a dummy
> device with a pfifo limit 0 root), every packet the lower qdisc drops
> would appear as a good transmit in ip -s link show teql0. tx_dropped and
> tx_errors would stay at zero.
> 
> The VLAN slave counts the same rc as a drop in vlan_dev_hard_start_xmit():
> 
>     if (likely(ret == NET_XMIT_SUCCESS || ret == NET_XMIT_CN)) {
>         ...
>         u64_stats_inc(&stats->tx_packets);
>         ...
>     } else {
>         this_cpu_inc(vlan->vlan_pcpu_stats->tx_dropped);
>     }
> 
> Could the rc be saved once? dev_xmit_complete(rc) would then decide only
> whether the skb is still owned, which keeps the use-after-free fix. In
> that case, tx_packets and tx_bytes would only grow for NETDEV_TX_OK and
> NET_XMIT_CN. The other consumed codes would go to tx_dropped (or
> tx_errors), without touching the skb.
> 
> The commit message describes this only as a use-after-free fix. Could it
> also mention that teql0's counters behave differently now?
> 
> >  					__netif_tx_unlock(slave_txq);
> >  					raw_spin_lock_irqsave(&master->slaves_lock, flags);
> >  					if (rcu_dereference_protected(master->slaves,
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-Y0BY.v1.r0.20261015184107%40mojatatu.com