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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.08.2025 11:33, Stewart Hildebrand wrote:
> 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,

Oh, I see - that wasn't quite obvious, though. Yet of course ...

> 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.)

... this latter part of my remark still applies. IOW I continue to think
that discussing the correctness of this change needs to be extended.
Unless of course a scheduler maintainer wants to ack it as is.

Jan