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

Would something like below be okay?

diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index d3a0a97e1d..36704a5836 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -589,8 +589,8 @@ int sched_init_vcpu(struct vcpu *v)
     unit->priv = sched_alloc_udata(dom_scheduler(d), unit, d->sched_priv);
     if ( unit->priv == NULL )
     {
-        sched_free_unit(unit, v);
         rcu_read_unlock(&sched_res_rculock);
+        sched_destroy_vcpu(v);
         return 1;
     }
 
@@ -869,8 +869,11 @@ void sched_destroy_vcpu(struct vcpu *v)
     {
         rcu_read_lock(&sched_res_rculock);
 
-        sched_remove_unit(vcpu_scheduler(v), unit);
-        sched_free_udata(vcpu_scheduler(v), unit->priv);
+        if ( unit->priv )
+        {
+            sched_remove_unit(vcpu_scheduler(v), unit);
+            sched_free_udata(vcpu_scheduler(v), unit->priv);
+        }
         sched_free_unit(unit, v);
 
         rcu_read_unlock(&sched_res_rculock);


Furkan