* [PATCH] xen/sched: fix sched_move_domain() for domain without vcpus
@ 2021-09-06 11:00 Juergen Gross
2021-09-06 11:14 ` Andrew Cooper
0 siblings, 1 reply; 6+ messages in thread
From: Juergen Gross @ 2021-09-06 11:00 UTC (permalink / raw)
To: xen-devel; +Cc: Juergen Gross, George Dunlap, Dario Faggioli, Bertrand Marquis
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/sched: fix sched_move_domain() for domain without vcpus
2021-09-06 11:00 [PATCH] xen/sched: fix sched_move_domain() for domain without vcpus Juergen Gross
@ 2021-09-06 11:14 ` Andrew Cooper
2021-09-06 11:18 ` Andrew Cooper
2021-09-06 11:22 ` Juergen Gross
0 siblings, 2 replies; 6+ messages in thread
From: Andrew Cooper @ 2021-09-06 11:14 UTC (permalink / raw)
To: Juergen Gross, xen-devel; +Cc: George Dunlap, Dario Faggioli, Bertrand Marquis
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/sched: fix sched_move_domain() for domain without vcpus
2021-09-06 11:14 ` Andrew Cooper
@ 2021-09-06 11:18 ` Andrew Cooper
2021-09-06 11:23 ` Jan Beulich
2021-09-06 11:22 ` Juergen Gross
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2021-09-06 11:18 UTC (permalink / raw)
To: Juergen Gross, xen-devel; +Cc: George Dunlap, Dario Faggioli, Bertrand Marquis
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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/sched: fix sched_move_domain() for domain without vcpus
2021-09-06 11:14 ` Andrew Cooper
2021-09-06 11:18 ` Andrew Cooper
@ 2021-09-06 11:22 ` Juergen Gross
1 sibling, 0 replies; 6+ messages in thread
From: Juergen Gross @ 2021-09-06 11:22 UTC (permalink / raw)
To: Andrew Cooper, xen-devel; +Cc: George Dunlap, Dario Faggioli, Bertrand Marquis
[-- Attachment #1.1.1: Type: text/plain, Size: 1858 bytes --]
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
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/sched: fix sched_move_domain() for domain without vcpus
2021-09-06 11:18 ` Andrew Cooper
@ 2021-09-06 11:23 ` Jan Beulich
2021-09-07 5:08 ` Juergen Gross
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2021-09-06 11:23 UTC (permalink / raw)
To: Andrew Cooper, Juergen Gross
Cc: George Dunlap, Dario Faggioli, Bertrand Marquis, xen-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/sched: fix sched_move_domain() for domain without vcpus
2021-09-06 11:23 ` Jan Beulich
@ 2021-09-07 5:08 ` Juergen Gross
0 siblings, 0 replies; 6+ messages in thread
From: Juergen Gross @ 2021-09-07 5:08 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper
Cc: George Dunlap, Dario Faggioli, Bertrand Marquis, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 2112 bytes --]
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
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-07 5:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-06 11:00 [PATCH] xen/sched: fix sched_move_domain() for domain without vcpus Juergen Gross
2021-09-06 11:14 ` Andrew Cooper
2021-09-06 11:18 ` Andrew Cooper
2021-09-06 11:23 ` Jan Beulich
2021-09-07 5:08 ` Juergen Gross
2021-09-06 11:22 ` Juergen Gross
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.