xen/x86: domain: Free all the pages associated to struct domain

[Xen-devel] [PATCH] xen/x86: domain: Free all the pages associated to struct domain

Posted by Julien Grall 4 years, 3 months ago

From: Julien Grall <jgrall@amazon.com>

The structure domain may be bigger than a page size when lock profiling
is enabled. However, the function free_domheap_struct will only free the
first page.

This is not a security issue because struct domain can only be bigger
than a page size for lock profiling. The feature can only be selected
in DEBUG and EXPERT mode.

Fixes: 8916fcf4577 ("x86/domain: compile with lock_profile=y enabled")
Reported-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/x86/domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 28fefa1f81..a5380b9bab 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -344,7 +344,7 @@ struct domain *alloc_domain_struct(void)
 
 void free_domain_struct(struct domain *d)
 {
-    free_xenheap_page(d);
+    free_xenheap_pages(d, get_order_from_bytes(sizeof(*d)));
 }
 
 struct vcpu *alloc_vcpu_struct(const struct domain *d)
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/x86: domain: Free all the pages associated to struct domain

Posted by Andrew Cooper 4 years, 3 months ago

On 20/01/2020 14:31, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
>
> The structure domain may be bigger than a page size when lock profiling
> is enabled. However, the function free_domheap_struct will only free the
> first page.
>
> This is not a security issue because struct domain can only be bigger
> than a page size for lock profiling. The feature can only be selected
> in DEBUG and EXPERT mode.
>
> Fixes: 8916fcf4577 ("x86/domain: compile with lock_profile=y enabled")
> Reported-by: David Woodhouse <dwmw@amazon.co.uk>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
>  xen/arch/x86/domain.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 28fefa1f81..a5380b9bab 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -344,7 +344,7 @@ struct domain *alloc_domain_struct(void)
>  
>  void free_domain_struct(struct domain *d)
>  {
> -    free_xenheap_page(d);
> +    free_xenheap_pages(d, get_order_from_bytes(sizeof(*d)));

:(

I'm entirely certain I raised this during the review of the original patch.

I'd much rather see the original patch reverted.  The current size of
struct domain with lockprofile enabled is 3200 bytes.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/x86: domain: Free all the pages associated to struct domain

Posted by Julien Grall 4 years, 3 months ago

Hi Andrew,

On 22/01/2020 12:52, Andrew Cooper wrote:
> On 20/01/2020 14:31, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> The structure domain may be bigger than a page size when lock profiling
>> is enabled. However, the function free_domheap_struct will only free the
>> first page.
>>
>> This is not a security issue because struct domain can only be bigger
>> than a page size for lock profiling. The feature can only be selected
>> in DEBUG and EXPERT mode.
>>
>> Fixes: 8916fcf4577 ("x86/domain: compile with lock_profile=y enabled")
>> Reported-by: David Woodhouse <dwmw@amazon.co.uk>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>> ---
>>   xen/arch/x86/domain.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>> index 28fefa1f81..a5380b9bab 100644
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -344,7 +344,7 @@ struct domain *alloc_domain_struct(void)
>>   
>>   void free_domain_struct(struct domain *d)
>>   {
>> -    free_xenheap_page(d);
>> +    free_xenheap_pages(d, get_order_from_bytes(sizeof(*d)));
> 
> :(
> 
> I'm entirely certain I raised this during the review of the original patch.
> 
> I'd much rather see the original patch reverted.  The current size of
> struct domain with lockprofile enabled is 3200 bytes.

Let me have a look first to see when/why struct domain is less than 4K 
with lockprofile.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/x86: domain: Free all the pages associated to struct domain

Posted by Andrew Cooper 4 years, 3 months ago

On 22/01/2020 13:13, Julien Grall wrote:
> Hi Andrew,
>
> On 22/01/2020 12:52, Andrew Cooper wrote:
>> On 20/01/2020 14:31, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> The structure domain may be bigger than a page size when lock profiling
>>> is enabled. However, the function free_domheap_struct will only free
>>> the
>>> first page.
>>>
>>> This is not a security issue because struct domain can only be bigger
>>> than a page size for lock profiling. The feature can only be selected
>>> in DEBUG and EXPERT mode.
>>>
>>> Fixes: 8916fcf4577 ("x86/domain: compile with lock_profile=y enabled")
>>> Reported-by: David Woodhouse <dwmw@amazon.co.uk>
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>> ---
>>>   xen/arch/x86/domain.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>>> index 28fefa1f81..a5380b9bab 100644
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -344,7 +344,7 @@ struct domain *alloc_domain_struct(void)
>>>     void free_domain_struct(struct domain *d)
>>>   {
>>> -    free_xenheap_page(d);
>>> +    free_xenheap_pages(d, get_order_from_bytes(sizeof(*d)));
>>
>> :(
>>
>> I'm entirely certain I raised this during the review of the original
>> patch.
>>
>> I'd much rather see the original patch reverted.  The current size of
>> struct domain with lockprofile enabled is 3200 bytes.
>
> Let me have a look first to see when/why struct domain is less than 4K
> with lockprofile.

In the intervening time, Juergen has totally rewritten lock profiling,
and the virtual timer infrastructure has been taken out-of-line.

Its probably the latter which is the dominating factor.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/x86: domain: Free all the pages associated to struct domain

Posted by Roger Pau Monné 4 years, 3 months ago

On Mon, Jan 20, 2020 at 02:31:42PM +0000, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The structure domain may be bigger than a page size when lock profiling
> is enabled. However, the function free_domheap_struct will only free the
> first page.
> 
> This is not a security issue because struct domain can only be bigger
> than a page size for lock profiling. The feature can only be selected
> in DEBUG and EXPERT mode.
> 
> Fixes: 8916fcf4577 ("x86/domain: compile with lock_profile=y enabled")
> Reported-by: David Woodhouse <dwmw@amazon.co.uk>
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/x86: domain: Free all the pages associated to struct domain

Posted by Jan Beulich 4 years, 3 months ago

On 20.01.2020 15:31, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The structure domain may be bigger than a page size when lock profiling
> is enabled. However, the function free_domheap_struct will only free the
> first page.
> 
> This is not a security issue because struct domain can only be bigger
> than a page size for lock profiling. The feature can only be selected
> in DEBUG and EXPERT mode.
> 
> Fixes: 8916fcf4577 ("x86/domain: compile with lock_profile=y enabled")
> Reported-by: David Woodhouse <dwmw@amazon.co.uk>
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel