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

[Jan Beulich <jbeulich@xxxxxxxx>]

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