On 05.08.2025 11:06, Stewart Hildebrand wrote:
> On 8/5/25 03:44, Jan Beulich wrote:
>> On 04.08.2025 18:57, Stewart Hildebrand wrote:
>>> 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?
>
> ^^ This ^^ is what I'm confused about
If sched_init_vcpu() took the indicated path, the if() you add here won't
help, and ...
>>>> 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.)
>>>
>>>
>>> Are you referring to this error path in sched_init_vcpu?
>>
>> No, given the context I thought it was clear that I was referring to
>>
>> static void cf_check
>> csched_free_udata(const struct scheduler *ops, void *priv)
>> {
>> struct csched_unit *svc = priv;
>>
>> BUG_ON( !list_empty(&svc->runq_elem) );
... we'd make it here (afaict).
Jan
