On 8/4/25 03:57, Jan Beulich wrote:
> On 01.08.2025 22:24, Stewart Hildebrand wrote:
>> @@ -839,6 +839,9 @@ void sched_destroy_vcpu(struct vcpu *v)
>> {
>> struct sched_unit *unit = v->sched_unit;
>>
>> + if ( !unit )
>> + return;
>> +
>> kill_timer(&v->periodic_timer);
>> kill_timer(&v->singleshot_timer);
>> kill_timer(&v->poll_timer);
>
> What if it's the 2nd error path in sched_init_vcpu() that is taken? Then we
> might take this path (just out of context here)
>
> if ( unit->vcpu_list == v )
> {
> rcu_read_lock(&sched_res_rculock);
>
> sched_remove_unit(vcpu_scheduler(v), unit);
> sched_free_udata(vcpu_scheduler(v), unit->priv);
>
> and at least Credit1's hook doesn't look to be safe against being passed NULL.
> (Not to speak of the risk of unit->priv being used elsewhere while cleaning
> up.)
>
> Jan
Are you referring to this error path in sched_init_vcpu?
unit->priv = sched_alloc_udata(dom_scheduler(d), unit, d->sched_priv);
if ( unit->priv == NULL )
{
sched_free_unit(unit, v);
rcu_read_unlock(&sched_res_rculock);
return 1;
}
If so, my understanding is that sched_free_unit sets
v->sched_unit = NULL, so sched_destroy_vcpu (with this patch applied)
would return before reaching that just-out-of-context condition.
Albeit I have not yet tested this with any sort of non-default
scheduling granularity, so I could have missed something...
