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

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

Jakub Kicinski <kuba@kernel.org> writes:

> 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?
>

should_change_schedules() checks if the previous 'end_time' is after the
start of the "new" admin schedule.

> 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?

If the hrtimer triggers early, that might be possible. But one fact is
that this is on the software implementation, and as its accuracy is the
hrtimer accuracy, second guessing what the hrtimer did/is doing here
seems wrong.

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

Yeah, if the hrtimer fires early, we might have some schedule entries
that are longer than they should be. But that is an intrinsic limitation
of the software mode (at least in its current implementation). 

So, in short, not really concerned about this "violation of strictness",
as long as we are not making it worse, to be clear.

>
>>  		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.

I had to sleep on this, yeah, this is possible, but the window is very
narrow. And not introduced by this fix. I guess this problem existed
since I added the concept of an "admin" schedule.

Will fix on a separate patch. Need to think more about this.

>
> 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?

At configuration time, txtime-assisted (which calls get_packet_txtime())
mode and the software mode (which "runs" advance_sched()) are blocked to
run at the same time. This should not be possible.

>
> 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;


Cheers,
-- 
Vinicius