Re: [PATCH] xen/sched: fix sched_move_domain() for domain without vcpus

[Juergen Gross <jgross@xxxxxxxx>]

On 06.09.21 13:14, Andrew Cooper wrote:
> On 06/09/2021 12:00, Juergen Gross wrote:
> > In case a domain is created with a cpupool other than Pool-0 specified
> > it will be moved to that cpupool before any vcpus are allocated.
> >
> > This will lead to a NULL pointer dereference in sched_move_domain().
> >
> > Fix that by tolerating vcpus not being allocated yet.
> >
> > Fixes: 70fadc41635b9b6 ("xen/cpupool: support moving domain between cpupools with 
> > different granularity")
> > Reported-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> > Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > ---
> >   xen/common/sched/core.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> > index 8d178baf3d..79c9100680 100644
> > --- a/xen/common/sched/core.c
> > +++ b/xen/common/sched/core.c
> > @@ -671,6 +671,10 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
> >   
> >       for ( unit_idx = 0; unit_idx < n_units; unit_idx++ )
> >       {
> > +        /* Special case for move at domain creation time. */
> > +        if ( !d->vcpu[unit_idx * gran] )
> > +            break;
> > +
> >           unit = sched_alloc_unit_mem();
> >           if ( unit )
> >           {
> I think the logic would be clearer if you wrap the entire for loop in if
> ( d->max_vcpus ).
No, d->max_vcpus is not 0 here, otherwise n_units would be 0.

> This loop is only allocating units in the new
> scheduler for existing vcpus, so there's no point entering the loop at
> all during domain creation.
>
> Also, this removes a non-speculatively-guarded d->vcpu[] deference.
I don't think this dereference is a real problem. In case you are
worried about it we should replace the one further below, too.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature