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

[Vinicius Costa Gomes <vinicius.gomes@intel.com>]

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