Re: [RFC PATCH] xen/sched/null: avoid crash after failed domU creation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.04.2023 08:36, Juergen Gross wrote:
> On 24.04.23 23:00, Stewart Hildebrand wrote:
>> When creating a domU, but the creation fails, we may end up in a state
>> where a vcpu has not yet been added to the null scheduler, but
>> unit_deassign() is invoked.
> 
> This is not really true. The vcpu has been added, but it was offline at
> that time. This resulted in null_unit_insert() returning early and not
> calling unit_assign().
> 
> Later the vcpu was onlined during XEN_DOMCTL_setvcpucontext handling,
> resulting in null_unit_remove() calling unit_deassign().
> 
>> In this case, when running a debug build of
>> Xen, we will hit an ASSERT and crash Xen:
>>
>> (XEN) ****************************************
>> (XEN) Panic on CPU 0:
>> (XEN) Assertion 'npc->unit == unit' failed at common/sched/null.c:379
>> (XEN) ****************************************
>>
>> To work around this, remove the ASSERT and introduce a check for the
>> case where npc->unit is NULL and simply return false from
>> unit_deassign().
> 
> I think the correct fix would be to call unit_deassign() from
> null_unit_remove() only, if npc->unit isn't NULL. Dario might have a
> different opinion, though. :-)

Furthermore, even if the proposed solution was (roughly) followed, ...

>> --- a/xen/common/sched/null.c
>> +++ b/xen/common/sched/null.c
>> @@ -376,7 +376,14 @@ static bool unit_deassign(struct null_private *prv, 
>> const struct sched_unit *uni
>>       struct null_pcpu *npc = get_sched_res(cpu)->sched_priv;
>>   
>>       ASSERT(list_empty(&null_unit(unit)->waitq_elem));
>> -    ASSERT(npc->unit == unit);
>> +
>> +    if ( !npc->unit )
>> +    {
>> +        dprintk(XENLOG_G_INFO, "%d <-- NULL (%pdv%d)\n", cpu, unit->domain,
>> +                unit->unit_id);
>> +        return false;
>> +    }
>> +

... shouldn't the assertion be kept, with the new if() inserted ahead of
it? Plus the log message probably better wouldn't print a unit ID like a
vCPU one, but instead use e.g. %pdu%u?

Jan