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

> 
>> --- a/xen/common/sched/core.c
>> +++ b/xen/common/sched/core.c
>> @@ -589,6 +589,9 @@ int sched_init_vcpu(struct vcpu *v)
>>      unit->priv = sched_alloc_udata(dom_scheduler(d), unit, d->sched_priv);
>>      if ( unit->priv == NULL )
>>      {
>> +        kill_timer(&v->periodic_timer);
>> +        kill_timer(&v->singleshot_timer);
>> +        kill_timer(&v->poll_timer);
>>          sched_free_unit(unit, v);
>>          rcu_read_unlock(&sched_res_rculock);
>>          return 1;
> 
> This almost, but not quite open-codes sched_destroy_vcpu(). Would be nice
> if the cleanup logic was shared. The sched_free_unit() call there could be
> leveraged here as well; what would need skipping are the sched_free_udata()
> and sched_remove_unit(). And of course the RCU-locking would need sorting.
> 
> Jan

Furkan