xen/common/sched/core.c | 4 ++++ 1 file changed, 4 insertions(+)
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 )
{
--
2.26.2
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.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
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
© 2016 - 2024 Red Hat, Inc.