Re: [PATCH net] net/sched: sch_taprio: do not requeue a deactivated qdisc

[Jakub Kicinski]

On Wed, 05 Aug 2026 13:19:36 -0700 Vinicius Costa Gomes wrote:
> David Lee <david.lee@xxxxxxxxxxxxxxx> writes:
> 
> > From: Kyle Zeng <kylebot@xxxxxxxxxx>
> >
> > Root qdisc replacement and deletion call dev_deactivate() without
> > resetting the old qdisc. This marks the qdisc deactivated and waits for
> > existing runs to finish, but leaves TAPRIO's private hrtimer active.
> > advance_sched() can therefore requeue the old root after the final busy
> > check, allowing a new run to overlap reset and destruction.
> >
> > Do not schedule TAPRIO after its root has been deactivated. Keep the
> > test in the existing RCU read-side critical section so that it pairs
> > with the synchronize_net() in dev_deactivate_many(): a callback which
> > observes an active qdisc must finish before the final busy check, while
> > a later callback observes the deactivated state and skips the requeue.
> >
> > Fixes: 5a781ccbd19e ("tc: Add support for configuring the taprio scheduler")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Assisted-by: Codex:gpt-5.6-sol Codex:gpt-5.5-cyber
> > Signed-off-by: Kyle Zeng <kylebot@xxxxxxxxxx>
> > Co-developed-by: David Lee <david.lee@xxxxxxxxxxxxxxx>
> > Signed-off-by: David Lee <david.lee@xxxxxxxxxxxxxxx>
> > ---
> > Bug found and triaged by OpenAI Security Research and
> > validated by Trail of Bits.
> >
> > The supplied v7.2-rc3 trace contains a KASAN use-after-free. The
> > reproducer did not trigger a sanitizer report in the current v7.2-rc5
> > campaign and can be shared if needed.
> >
> >  net/sched/sch_taprio.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> > index 299234a5f..2cf76df43 100644
> > --- a/net/sched/sch_taprio.c
> > +++ b/net/sched/sch_taprio.c
> > @@ -990,7 +990,8 @@ static enum hrtimer_restart advance_sched(struct hrtimer *timer)
> >  	hrtimer_set_expires(&q->advance_timer, end_time);
> >  
> >  	rcu_read_lock();
> > -	__netif_schedule(sch);
> > +	if (!test_bit(__QDISC_STATE_DEACTIVATED, &sch->state))
> > +		__netif_schedule(sch);
> >  	rcu_read_unlock();
> >  
> 
> I'll be the first one to admit that taprio is a weird one (that it keeps
> a timer around while it's running among others), but it looks to me that
> this check would make more sense inside __netif_schedule().
> 
> Let's see what others think.

see the clashiko AI comment below. If that's true and indeed problem
did not exist before 47e8dbb6e763e5 -- then the fix is misplaced,
like you say. (I'm not sure about __netif_schedule(), to be clear,
but some_qdisc_is_busy() is not strong enough?)


The changelog opens with:
  "Root qdisc replacement and deletion call dev_deactivate() without
   resetting the old qdisc."
Is that true for the trees the Fixes: tag points at?
That behaviour looks like it arrives with 47e8dbb6e763e5 ("net/sched: do
not reset queues in graft operations"), which added the reset_needed
argument and made qdisc_graft() use:
net/sched/sch_api.c:qdisc_graft() {
	...
		if (dev->flags & IFF_UP)
			dev_deactivate(dev, false);
	...
}
Before that, dev_deactivate_many() ran dev_reset_queue() on every txq
unconditionally, and it did so before the some_qdisc_is_busy() wait loop.
dev_reset_queue() resets rtnl_dereference(dev_queue->qdisc_sleeping), which
in the root-graft path is still the old taprio qdisc, so
qdisc_reset() -> taprio_reset() -> hrtimer_cancel() disarmed advance_timer
before the busy check, and nothing re-arms it outside
taprio_change()/taprio_start_sched() under RTNL.
If that reading is right, the requeue-after-busy-check window does not
exist without 47e8dbb6e763e5, but the patch carries
Fixes: 5a781ccbd19e ("tc: Add support for configuring the taprio
scheduler") plus Cc: stable, which aims it at every stable tree back to
v4.20.
Should the Fixes: tag name the commit that made dev_deactivate() skip the
reset, and should the changelog mention that this reset-skipping behaviour
is a recent change, so the backport range is clear?
-- 
pw-bot: cr