Re: [PATCH v2 2/2] xen/sched: core: kill unarmed timers on sched_init_vcpu() failure

[Furkan Çalışkan <frn1furkan10@xxxxxxxxx>]

On 8/19/26 11:32, Jan Beulich wrote:
> On 19.08.2026 09:53, Furkan Çalışkan wrote:
>>
>>
>> On 8/19/26 10:22, Jan Beulich wrote:
>>> On 19.08.2026 07:15, Furkan Caliskan wrote:
>>>> sched_init_vcpu() calls init_timer() for a vcpu's periodic_timer,
>>>> singleshot_timer and poll_timer before it can fail -- these
>>>> become live, linked into their target pCPU's per-cpu timer list
>>>> regardless of what happens next. If the sched_alloc_udata() call
>>>> further down then fails, the function frees the sched_unit via
>>>> sched_free_unit() and returns 1, but never unlinks these three
>>>> timers.
>>>>
>>>> The caller, vcpu_create(), makes this worse: on sched_init_vcpu()
>>>> returning nonzero it jumps to fail_wq, skipping fail_sched and
>>>> thus sched_destroy_vcpu() -- the only function on this path that
>>>> calls kill_timer() on them. vcpu_destroy() then frees the vcpu,
>>>> and the three timers embedded in it, while they are still linked
>>>> into that shared list.
>>>>
>>>> This silently corrupts that list. It only shows up later, when
>>>> something else touches a neighboring timer: sched_move_domain()
>>>> crashed with "Assertion 'entry->prev->next == entry' failed" on a
>>>> completely unrelated, valid vcpu's timer.
>>>>
>>>> Kill all three timers in sched_init_vcpu()'s own failure branch,
>>>> so it doesn't depend on the caller reaching sched_destroy_vcpu()
>>>> to undo what it set up itself.
>>>>
>>>> Fixes: 1ad5dad74cde ("[XEN] Re-jig VCPU initialisation -- VMX init 
>>>> requires generic VCPU")
>>>
>>> How did you arrive at this commit? It doesn't even touch sched_init_vcpu().
>>> All it does is move kill_timer() invocations around. I think it's
>>> d884b1077817, as that's where the "return SCHED_OP(init_vcpu, v)" was
>>> introduced (i.e. where kill_timer() would have been necessary to call in
>>> the error case). (I can't exclude the issue was pre-existing already at
>>> that time, but that would require more analysis than I think is worth to
>>> invest.)
>>
>> Commit 1ad5dad74cde moved kill_timer() calls into sched_destroy_vcpu() 
>> function, which is not called if sched_init_vcpu() fails. 
>> Before that commit, kill_timer() calls were in sched_destroy_domain(), 
>> which is called if sched_init_vcpu() returns non-zero to its caller, 
>> alloc_vcpu(). 
>>
>>         for ( i = 0; i < max; i++ )
>>         {
>>             if ( d->vcpu[i] != NULL )
>>                 continue;
>>
>>             cpu = (i == 0) ?
>>                 default_vcpu0_location() :
>>                 (d->vcpu[i-1]->processor + 1) % num_online_cpus();
>>
>>             if ( alloc_vcpu(d, i, cpu) == NULL )
>>                 goto maxvcpu_out;
>>         }
>>
>>         ret = 0;
>>
>>     maxvcpu_out:
>>         domain_unpause(d);
>>         put_domain(d);
>>     }
>>     break;
>>
>> put_domain() calls domain_destroy(), which then calls free_domain(), 
>> which ultimately calls sched_destroy_domain().
> 
> Well, yes, except that - how would that have helped for a vCPU which
> failed to be properly constructed? The function loops over all vCPU-s
> in the domain, but that wouldn't include the vCPU in question.
> alloc_vcpu() would (of course) insert the vCPU into the list only when
> sched_init_vcpu() succeeds.
> 
> Jan

Ah, I missed that compeletely - you're right.
sched_destroy_domain() wouldn't have reached the failed vCPU anyway.

In that case, d884b1077817 makes total sense here.

Furkan