| Message ID | 20210906110057.15384-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/sched: fix sched_move_domain() for domain without vcpus | expand |
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@arm.com> > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > 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 ). 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. ~Andrew
On 06/09/2021 12: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@arm.com> >> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> 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 ). And of course, this is wrong. Turns out the domain_has_vcpus() predicate still hasn't been committed, but d->vcpu[0] is the correct internal. ~Andrew > 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. > > ~Andrew > >
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@arm.com> >> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> 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
On 06.09.2021 13:18, Andrew Cooper wrote: > On 06/09/2021 12: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@arm.com> >>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> >>> Signed-off-by: Juergen Gross <jgross@suse.com> >>> --- >>> 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 ). > > And of course, this is wrong. Turns out the domain_has_vcpus() > predicate still hasn't been committed, but d->vcpu[0] is the correct > internal. Which in turn might want to be done by setting n_units to zero when d->vcpus[0] is NULL? Jan
On 06.09.21 13:23, Jan Beulich wrote: > On 06.09.2021 13:18, Andrew Cooper wrote: >> On 06/09/2021 12: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@arm.com> >>>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> >>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>>> --- >>>> 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 ). >> >> And of course, this is wrong. Turns out the domain_has_vcpus() >> predicate still hasn't been committed, but d->vcpu[0] is the correct >> internal. > > Which in turn might want to be done by setting n_units to zero when > d->vcpus[0] is NULL? Yes, this would be possible. OTOH my variant is more robust in case not all vcpus are allocated, but I guess this will explode somewhere else anyway. In case I don't get any other comment today I'll change the patch to set n_units to 0 if d->vcpus[0] is NULL. Juergen
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 ) {
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@arm.com> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Signed-off-by: Juergen Gross <jgross@suse.com> --- xen/common/sched/core.c | 4 ++++ 1 file changed, 4 insertions(+)