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

[Jan Beulich <jbeulich@xxxxxxxx>]

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