On 8/5/25 05:17, Jan Beulich wrote:
> 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,
What path? In the one I'm looking at, sched_free_unit() gets called,
setting v->sched_unit = NULL, and in that case ...
> the if() you add here won't
> help, and ...
... the condition is true, 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).
... we'd not make it here.
