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

[Jan Beulich <jbeulich@xxxxxxxx>]

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