Re: [PATCH net v2] net: sched: prevent scheduling deactivated qdiscs

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · 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).

- [High] The patch does not cover the IFF_UP-but-no-carrier case.

--

net: sched: prevent scheduling deactivated qdiscs

This patch makes __netif_schedule() return early for qdiscs that have
__QDISC_STATE_DEACTIVATED set. The goal is to stop a late TAPRIO
advance_timer callback from scheduling a qdisc after some_qdisc_is_busy()
has reported it idle. It also makes transition_one_qdisc() reschedule
nonempty queues on reactivation.

Link: https://lore.kernel.org/netdev/20260805102514.740834-1-david.lee@trailofbits.com/

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1a09e78691180..bcd2d5197d5b7 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3478,6 +3478,9 @@ static void __netif_reschedule(struct Qdisc *q)
>  
>  void __netif_schedule(struct Qdisc *q)
>  {
> +	if (unlikely(test_bit(__QDISC_STATE_DEACTIVATED, &q->state)))
> +		return;
> +

[Severity: High]
Does this guard cover the case where the device is IFF_UP but the carrier
is off?

Only qdisc_deactivate() sets the DEACTIVATED bit, and
dev_deactivate_queue() calls it only on dev_queue->qdisc:

net/sched/sch_generic.c:dev_deactivate_queue() {
    ...
	qdisc = rtnl_dereference(dev_queue->qdisc);
	if (qdisc) {
		if (qdisc->enqueue)
			*sync_needed = true;
		qdisc_deactivate(qdisc);
		rcu_assign_pointer(dev_queue->qdisc, &noop_qdisc);
	}
}

With the carrier off, dev_activate() returns before it reaches
transition_one_qdisc():

net/sched/sch_generic.c:dev_activate() {
    ...
	if (!netif_carrier_ok(dev))
		/* Delay activation until next carrier-on event */
		return;
    ...
}

So dev_queue->qdisc stays &noop_qdisc. Because noop_qdisc is
TCQ_F_BUILTIN, qdisc_deactivate() does nothing to it. The TAPRIO in
qdisc_sleeping never gets the bit, and qdisc_alloc() does not set it
either.

Here is a possible sequence, for example on a multiqueue veth whose peer
is down, or a multiqueue TAP with no reader:

1. tc qdisc add ... root taprio (software mode). taprio_change() calls
   taprio_start_sched(), which arms advance_timer. taprio_attach() puts
   the taprio in each qdisc_sleeping, and dev_activate() returns early.

2. tc qdisc del/replace root. qdisc_graft() calls
   dev_deactivate(dev, false). Only noop_qdisc is flagged.
   dev_reset_queue() is skipped because reset_needed is false, and
   some_qdisc_is_busy() returns. advance_timer is still armed.

3. On another CPU:

   advance_sched()
     __netif_schedule(sch)    <- DEACTIVATED is clear, SCHED gets set
   net_tx_action()
     clear_bit(__QDISC_STATE_SCHED, &q->state)
     qdisc_run()
       taprio_dequeue()
         taprio_dequeue_from_txq()
           child = q->qdiscs[txq]

4. Meanwhile, on CPU0:

   notify_and_destroy()
     qdisc_put()
       __qdisc_destroy()
         qdisc_reset()        <- taprio_reset(), without the root lock
         taprio_destroy()
           hrtimer_cancel(&q->advance_timer)
           qdisc_synchronize(sch)
           kfree(q->qdiscs)
           q->qdiscs = NULL

qdisc_synchronize() waits only for SCHED to clear. It does not wait for a
qdisc_run() that has already started, and net_tx_action() clears SCHED
before it calls qdisc_run().

Can taprio_dequeue_from_txq() then read the freed q->qdiscs array and call
child->ops->peek() through the pointer it loaded? And if it runs after
q->qdiscs = NULL, does it hit a NULL dereference instead?

The skb double-ownership in the commit message is probably not possible
here. Enqueues go to noop_qdisc, so the child queues are empty. It is
still the same race, though: TAPRIO gets scheduled after the busy check.

Before 47e8dbb6e763, graft-time dev_deactivate() always ran
dev_reset_queue() on qdisc_sleeping. That reached taprio_reset() and
hrtimer_cancel() before some_qdisc_is_busy(), whatever the carrier state.

Would it make sense to stop or neutralize the TAPRIO advance_timer for the
sleeping qdisc during graft deactivation? For example, the timer could be
cancelled before the busy wait, or qdisc_sleeping could be flagged as well
as dev_queue->qdisc.

This next part is a pre-existing issue, not one introduced by this patch.
If the device is not IFF_UP, qdisc_graft() skips dev_deactivate()
entirely:

net/sched/sch_api.c:qdisc_graft() {
    ...
		if (dev->flags & IFF_UP)
			dev_deactivate(dev, false);
    ...
}

So a taprio created on a down interface can still be scheduled by its own
timer while it is being destroyed.

>  	/* If q->defer_list is not empty, at least one thread is
>  	 * in __dev_xmit_skb() before llist_del_all(&q->defer_list).
>  	 * This thread will attempt to run the queue.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023194712.package1-20-v2-david.lee%40trailofbits.com