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

[netdev-bot+sashiko@kernel.org]

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)`.

--

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