Re: [PATCH v2] xen: rework error handling in vcpu_create

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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.