[Xen-devel] [PATCH v2 0/2] xen/x86: hap: Small clean-up/hardening in hap_enable()

[Xen-devel] [PATCH v2 0/2] xen/x86: hap: Small clean-up/hardening in hap_enable()

[Julien Grall]

Hi all,

This series contains a couple of clean-up/hardening for the function
hap_enable().

Cheers,

Julien Grall (2):
  xen/x86: hap: Fix coding style in hap_enable()
  xen/x86: hap: Clean-up and harden hap_enable()

 xen/arch/x86/mm/hap/hap.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

-- 
2.17.1


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

[Xen-devel] [PATCH v2 1/2] xen/x86: hap: Fix coding style in hap_enable()

[Julien Grall]

Signed-off-by: Julien Grall <julien@xen.org>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

---
    Changes in v2:
        - Add Roger's reviewed-by
---
 xen/arch/x86/mm/hap/hap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 3d93f3451c..31362a31b6 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -473,7 +473,8 @@ int hap_enable(struct domain *d, u32 mode)
             goto out;
     }
 
-    for (i = 0; i < MAX_NESTEDP2M; i++) {
+    for ( i = 0; i < MAX_NESTEDP2M; i++ )
+    {
         rv = p2m_alloc_table(d->arch.nested_p2m[i]);
         if ( rv != 0 )
            goto out;
-- 
2.17.1


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

Re: [Xen-devel] [PATCH v2 1/2] xen/x86: hap: Fix coding style in hap_enable()

[George Dunlap]

On 2/4/20 1:06 PM, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien@xen.org>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: George Dunlap <george.dunlap@citrix.com>

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

[Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden hap_enable()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Unlike shadow_enable(), hap_enable() can only be called once during
domain creation and with the mode equal to mode equal to
PG_external | PG_translate | PG_refcounts.

If it were called twice, then we might have something interesting
problem as the p2m tables would be re-allocated (and therefore all the
mappings would be lost).

Add code to sanity check the mode and that the function is only called
once. Take the opportunity to an if checking that PG_translate is set.

Signed-off-by: Julien Grall <jgrall@amazon.com>

---

It is not entirely clear when PG_translate was enforced.

I keep the check != 0 because this is consistent with the rest of the
file. If we want to omit comparison against 0, then this should be in a
separate patches converting the file.

    Changes in v2:
        - Fix typoes in the commit message
        - Use -EEXIST instead of -EINVAL
---
 xen/arch/x86/mm/hap/hap.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 31362a31b6..4974bd13d4 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -445,6 +445,13 @@ int hap_enable(struct domain *d, u32 mode)
     unsigned int i;
     int rv = 0;
 
+    if ( mode != (PG_external | PG_translate | PG_refcounts) )
+        return -EINVAL;
+
+    /* The function can only be called once */
+    if ( d->arch.paging.mode != 0 )
+        return -EEXIST;
+
     domain_pause(d);
 
     old_pages = d->arch.paging.hap.total_pages;
@@ -465,13 +472,10 @@ int hap_enable(struct domain *d, u32 mode)
     d->arch.paging.alloc_page = hap_alloc_p2m_page;
     d->arch.paging.free_page = hap_free_p2m_page;
 
-    /* allocate P2m table */
-    if ( mode & PG_translate )
-    {
-        rv = p2m_alloc_table(p2m_get_hostp2m(d));
-        if ( rv != 0 )
-            goto out;
-    }
+    /* allocate P2M table */
+    rv = p2m_alloc_table(p2m_get_hostp2m(d));
+    if ( rv != 0 )
+        goto out;
 
     for ( i = 0; i < MAX_NESTEDP2M; i++ )
     {
-- 
2.17.1


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

Re: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden hap_enable()

[Durrant, Paul]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
> Julien Grall
> Sent: 04 February 2020 13:06
> To: xen-devel@lists.xenproject.org
> Cc: julien@xen.org; Wei Liu <wl@xen.org>; George Dunlap
> <george.dunlap@eu.citrix.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
> Grall, Julien <jgrall@amazon.com>; Jan Beulich <jbeulich@suse.com>; Roger
> Pau Monné <roger.pau@citrix.com>
> Subject: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden
> hap_enable()
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Unlike shadow_enable(), hap_enable() can only be called once during
> domain creation and with the mode equal to mode equal to
> PG_external | PG_translate | PG_refcounts.
> 
> If it were called twice, then we might have something interesting
> problem as the p2m tables would be re-allocated (and therefore all the
> mappings would be lost).

There are two tests in p2m_alloc_tabl2: whether the domain has memory allocated, and whether the domain already has a p2m. Can these now be dropped?

  Paul

> 
> Add code to sanity check the mode and that the function is only called
> once. Take the opportunity to an if checking that PG_translate is set.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ---
> 
> It is not entirely clear when PG_translate was enforced.
> 
> I keep the check != 0 because this is consistent with the rest of the
> file. If we want to omit comparison against 0, then this should be in a
> separate patches converting the file.
> 
>     Changes in v2:
>         - Fix typoes in the commit message
>         - Use -EEXIST instead of -EINVAL
> ---
>  xen/arch/x86/mm/hap/hap.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index 31362a31b6..4974bd13d4 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -445,6 +445,13 @@ int hap_enable(struct domain *d, u32 mode)
>      unsigned int i;
>      int rv = 0;
> 
> +    if ( mode != (PG_external | PG_translate | PG_refcounts) )
> +        return -EINVAL;
> +
> +    /* The function can only be called once */
> +    if ( d->arch.paging.mode != 0 )
> +        return -EEXIST;
> +
>      domain_pause(d);
> 
>      old_pages = d->arch.paging.hap.total_pages;
> @@ -465,13 +472,10 @@ int hap_enable(struct domain *d, u32 mode)
>      d->arch.paging.alloc_page = hap_alloc_p2m_page;
>      d->arch.paging.free_page = hap_free_p2m_page;
> 
> -    /* allocate P2m table */
> -    if ( mode & PG_translate )
> -    {
> -        rv = p2m_alloc_table(p2m_get_hostp2m(d));
> -        if ( rv != 0 )
> -            goto out;
> -    }
> +    /* allocate P2M table */
> +    rv = p2m_alloc_table(p2m_get_hostp2m(d));
> +    if ( rv != 0 )
> +        goto out;
> 
>      for ( i = 0; i < MAX_NESTEDP2M; i++ )
>      {
> --
> 2.17.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden hap_enable()

[Julien Grall]

On 04/02/2020 13:16, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
>> Julien Grall
>> Sent: 04 February 2020 13:06
>> To: xen-devel@lists.xenproject.org
>> Cc: julien@xen.org; Wei Liu <wl@xen.org>; George Dunlap
>> <george.dunlap@eu.citrix.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
>> Grall, Julien <jgrall@amazon.com>; Jan Beulich <jbeulich@suse.com>; Roger
>> Pau Monné <roger.pau@citrix.com>
>> Subject: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden
>> hap_enable()
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Unlike shadow_enable(), hap_enable() can only be called once during
>> domain creation and with the mode equal to mode equal to
>> PG_external | PG_translate | PG_refcounts.
>>
>> If it were called twice, then we might have something interesting
>> problem as the p2m tables would be re-allocated (and therefore all the
>> mappings would be lost).
> 
> There are two tests in p2m_alloc_tabl2: whether the domain has memory allocated, and whether the domain already has a p2m. Can these now be dropped?

I don't think so. They are still necessary for the shadow page-tables. 
AFAICT, they are enabled via a DOMCTL.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden hap_enable()

[Julien Grall]

Hi,

Gentle ping.

Cheers,

On 04/02/2020 14:06, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Unlike shadow_enable(), hap_enable() can only be called once during
> domain creation and with the mode equal to mode equal to
> PG_external | PG_translate | PG_refcounts.
> 
> If it were called twice, then we might have something interesting
> problem as the p2m tables would be re-allocated (and therefore all the
> mappings would be lost).
> 
> Add code to sanity check the mode and that the function is only called
> once. Take the opportunity to an if checking that PG_translate is set.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ---
> 
> It is not entirely clear when PG_translate was enforced.
> 
> I keep the check != 0 because this is consistent with the rest of the
> file. If we want to omit comparison against 0, then this should be in a
> separate patches converting the file.
> 
>      Changes in v2:
>          - Fix typoes in the commit message
>          - Use -EEXIST instead of -EINVAL
> ---
>   xen/arch/x86/mm/hap/hap.c | 18 +++++++++++-------
>   1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index 31362a31b6..4974bd13d4 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -445,6 +445,13 @@ int hap_enable(struct domain *d, u32 mode)
>       unsigned int i;
>       int rv = 0;
>   
> +    if ( mode != (PG_external | PG_translate | PG_refcounts) )
> +        return -EINVAL;
> +
> +    /* The function can only be called once */
> +    if ( d->arch.paging.mode != 0 )
> +        return -EEXIST;
> +
>       domain_pause(d);
>   
>       old_pages = d->arch.paging.hap.total_pages;
> @@ -465,13 +472,10 @@ int hap_enable(struct domain *d, u32 mode)
>       d->arch.paging.alloc_page = hap_alloc_p2m_page;
>       d->arch.paging.free_page = hap_free_p2m_page;
>   
> -    /* allocate P2m table */
> -    if ( mode & PG_translate )
> -    {
> -        rv = p2m_alloc_table(p2m_get_hostp2m(d));
> -        if ( rv != 0 )
> -            goto out;
> -    }
> +    /* allocate P2M table */
> +    rv = p2m_alloc_table(p2m_get_hostp2m(d));
> +    if ( rv != 0 )
> +        goto out;
>   
>       for ( i = 0; i < MAX_NESTEDP2M; i++ )
>       {
> 

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden hap_enable()

[Julien Grall]

Ping again.

On 13/02/2020 12:44, Julien Grall wrote:
> Hi,
> 
> Gentle ping.
> 
> Cheers,
> 
> On 04/02/2020 14:06, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Unlike shadow_enable(), hap_enable() can only be called once during
>> domain creation and with the mode equal to mode equal to
>> PG_external | PG_translate | PG_refcounts.
>>
>> If it were called twice, then we might have something interesting
>> problem as the p2m tables would be re-allocated (and therefore all the
>> mappings would be lost).
>>
>> Add code to sanity check the mode and that the function is only called
>> once. Take the opportunity to an if checking that PG_translate is set.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>> ---
>>
>> It is not entirely clear when PG_translate was enforced.
>>
>> I keep the check != 0 because this is consistent with the rest of the
>> file. If we want to omit comparison against 0, then this should be in a
>> separate patches converting the file.
>>
>>      Changes in v2:
>>          - Fix typoes in the commit message
>>          - Use -EEXIST instead of -EINVAL
>> ---
>>   xen/arch/x86/mm/hap/hap.c | 18 +++++++++++-------
>>   1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
>> index 31362a31b6..4974bd13d4 100644
>> --- a/xen/arch/x86/mm/hap/hap.c
>> +++ b/xen/arch/x86/mm/hap/hap.c
>> @@ -445,6 +445,13 @@ int hap_enable(struct domain *d, u32 mode)
>>       unsigned int i;
>>       int rv = 0;
>> +    if ( mode != (PG_external | PG_translate | PG_refcounts) )
>> +        return -EINVAL;
>> +
>> +    /* The function can only be called once */
>> +    if ( d->arch.paging.mode != 0 )
>> +        return -EEXIST;
>> +
>>       domain_pause(d);
>>       old_pages = d->arch.paging.hap.total_pages;
>> @@ -465,13 +472,10 @@ int hap_enable(struct domain *d, u32 mode)
>>       d->arch.paging.alloc_page = hap_alloc_p2m_page;
>>       d->arch.paging.free_page = hap_free_p2m_page;
>> -    /* allocate P2m table */
>> -    if ( mode & PG_translate )
>> -    {
>> -        rv = p2m_alloc_table(p2m_get_hostp2m(d));
>> -        if ( rv != 0 )
>> -            goto out;
>> -    }
>> +    /* allocate P2M table */
>> +    rv = p2m_alloc_table(p2m_get_hostp2m(d));
>> +    if ( rv != 0 )
>> +        goto out;
>>       for ( i = 0; i < MAX_NESTEDP2M; i++ )
>>       {
>>
> 

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden hap_enable()

[Jan Beulich]

On 03.03.2020 13:21, Julien Grall wrote:
> Ping again.

To be honest, with the recent maintainer change it would probably
have been helpful if you had simply re-sent the patch. I did see
it back then, but had no comments to make and didn't have the
authority to ack it, so simply dropped it from my mailbox seeing
that you would be able to commit it yourself eventually.

Jan

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

Re: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden hap_enable()

[Julien Grall]

On 03/03/2020 13:27, Jan Beulich wrote:
> On 03.03.2020 13:21, Julien Grall wrote:
>> Ping again.
> 
> To be honest, with the recent maintainer change it would probably
> have been helpful if you had simply re-sent the patch.

Well, I don't think you can expect a contributor to resend a patch 
because the maintainers has changed. It would have been nicer if there 
was an handover with the list of pending patches.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden hap_enable()

[Jan Beulich]

On 04.02.2020 14:06, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Unlike shadow_enable(), hap_enable() can only be called once during
> domain creation and with the mode equal to mode equal to
> PG_external | PG_translate | PG_refcounts.
> 
> If it were called twice, then we might have something interesting
> problem as the p2m tables would be re-allocated (and therefore all the
> mappings would be lost).
> 
> Add code to sanity check the mode and that the function is only called
> once. Take the opportunity to an if checking that PG_translate is set.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
preferably with the duplicate words on the second line of the description
dropped.

> I keep the check != 0 because this is consistent with the rest of the
> file. If we want to omit comparison against 0, then this should be in a
> separate patches converting the file.

I disagree, but not enough to make the ack dependent upon the adjustment.

> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -445,6 +445,13 @@ int hap_enable(struct domain *d, u32 mode)
>      unsigned int i;
>      int rv = 0;
>  
> +    if ( mode != (PG_external | PG_translate | PG_refcounts) )
> +        return -EINVAL;
> +
> +    /* The function can only be called once */
> +    if ( d->arch.paging.mode != 0 )
> +        return -EEXIST;

I think if such a comment gets added, it should be unambiguous. The
function can be called once per domain, not just once in total.

Jan

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

Re: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden hap_enable()

[Julien Grall]

On 03/03/2020 13:25, Jan Beulich wrote:
> On 04.02.2020 14:06, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Unlike shadow_enable(), hap_enable() can only be called once during
>> domain creation and with the mode equal to mode equal to
>> PG_external | PG_translate | PG_refcounts.
>>
>> If it were called twice, then we might have something interesting
>> problem as the p2m tables would be re-allocated (and therefore all the
>> mappings would be lost).
>>
>> Add code to sanity check the mode and that the function is only called
>> once. Take the opportunity to an if checking that PG_translate is set.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>
> preferably with the duplicate words on the second line of the description
> dropped.

I will remove it on commit.

> 
>> I keep the check != 0 because this is consistent with the rest of the
>> file. If we want to omit comparison against 0, then this should be in a
>> separate patches converting the file.
> 
> I disagree, but not enough to make the ack dependent upon the adjustment.
> 
>> --- a/xen/arch/x86/mm/hap/hap.c
>> +++ b/xen/arch/x86/mm/hap/hap.c
>> @@ -445,6 +445,13 @@ int hap_enable(struct domain *d, u32 mode)
>>       unsigned int i;
>>       int rv = 0;
>>   
>> +    if ( mode != (PG_external | PG_translate | PG_refcounts) )
>> +        return -EINVAL;
>> +
>> +    /* The function can only be called once */
>> +    if ( d->arch.paging.mode != 0 )
>> +        return -EEXIST;
> 
> I think if such a comment gets added, it should be unambiguous. The
> function can be called once per domain, not just once in total.

I will replace with "The function can only be called one per domain".

Cheers,

-- 
Julien Grall

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