Re: [PATCH net] net/sched: taprio: fix use-after-free in advance_sched() on schedule switch

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/sched: taprio: fix use-after-free in advance_sched() on schedule switch

This commit updates advance_sched() to select the next entry from the new
oper schedule immediately after switch_schedules() returns. It relies on the
pre-calculated end_time of the new schedule's first entry instead of
overwriting it with the admin schedule's base time.

> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index f721c03514f60..0316f2dee06ac 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c

[ ... ]

> @@ -972,11 +972,12 @@ static enum hrtimer_restart advance_sched(struct hrtimer *timer)
>  	}
>  
>  	if (should_change_schedules(admin, oper, end_time)) {
> -		/* Set things so the next time this runs, the new
> -		 * schedule runs.
> -		 */
> -		end_time = sched_base_time(admin);

Does removing this assignment introduce a regression where the new schedule
activates prematurely?

Since the new schedule's first entry has its end_time pre-calculated
as base_time + interval, if we activate it early (before its actual
base_time), does it stretch the entry backwards in time?

Could this cause its gate mask to apply during the time gap, violating the
strictly configured gate durations?

>  		switch_schedules(q, &admin, &oper);

Can this sequence cause a use-after-free regression due to the RCU grace
period starting too early?

Inside switch_schedules(), call_rcu() is invoked on the old oper
schedule. However, q->current_entry is not updated until later in
advance_sched() at the first_run label:

first_run:
	rcu_assign_pointer(q->current_entry, next);

If a concurrent reader calls rcu_dereference(q->current_entry) between
switch_schedules() and this assignment, can it acquire a pointer to the
old schedule after call_rcu() has already started? This would mean the
grace period will not wait for this new reader.

As a separate pre-existing issue, can switch_schedules() be called
concurrently by lockless readers?

If advance_sched() calls it under q->current_entry_lock, but a concurrent
reader like get_packet_txtime() evaluates the switch condition and calls
switch_schedules() without holding locks, can they race?

Could this lead to call_rcu() being called twice on the same old oper
object, potentially corrupting the RCU callback list?

> +		/* After changing schedules, the next entry is the first one
> +		 * in the new schedule, with a pre-calculated end_time.
> +		 */
> +		next = list_first_entry(&oper->entries, struct sched_entry, list);
> +		end_time = next->end_time;
>  	}
>  
>  	next->end_time = end_time;