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

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

[David Lee]

From: Kyle Zeng <kylebot@openai.com>

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@vger.kernel.org
Assisted-by: Codex:gpt-5.6-sol Codex:gpt-5.5-cyber
Signed-off-by: Kyle Zeng <kylebot@openai.com>
Co-developed-by: David Lee <david.lee@trailofbits.com>
Signed-off-by: David Lee <david.lee@trailofbits.com>
---
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();
 
 	return HRTIMER_RESTART;
-- 
2.53.0

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

[Vinicius Costa Gomes]

David Lee <david.lee@trailofbits.com> writes:

> From: Kyle Zeng <kylebot@openai.com>
>
> 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@vger.kernel.org
> Assisted-by: Codex:gpt-5.6-sol Codex:gpt-5.5-cyber
> Signed-off-by: Kyle Zeng <kylebot@openai.com>
> Co-developed-by: David Lee <david.lee@trailofbits.com>
> Signed-off-by: David Lee <david.lee@trailofbits.com>
> ---
> 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.


Cheers,
-- 
Vinicius

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@trailofbits.com> writes:
> 
> > From: Kyle Zeng <kylebot@openai.com>
> >
> > 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@vger.kernel.org
> > Assisted-by: Codex:gpt-5.6-sol Codex:gpt-5.5-cyber
> > Signed-off-by: Kyle Zeng <kylebot@openai.com>
> > Co-developed-by: David Lee <david.lee@trailofbits.com>
> > Signed-off-by: David Lee <david.lee@trailofbits.com>
> > ---
> > 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