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

Commit Message

Julien Grall Feb. 4, 2020, 1:06 p.m. UTC

From: Julien Grall <[e-mail]>

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 <[e-mail]>

---

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(-)

Comments

Durrant, Paul Feb. 4, 2020, 1:16 p.m. UTC | #1

> -----Original Message-----
> From: Xen-devel <[e-mail]> On Behalf Of
> Julien Grall
> Sent: 04 February 2020 13:06
> To: [e-mail]
> Cc: [e-mail]; Wei Liu <[e-mail]>; George Dunlap
> <[e-mail]>; Andrew Cooper <[e-mail]>;
> Grall, Julien <[e-mail]>; Jan Beulich <[e-mail]>; Roger
> Pau Monné <[e-mail]>
> Subject: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden
> hap_enable()
> 
> From: Julien Grall <[e-mail]>
> 
> 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 <[e-mail]>
> 
> ---
> 
> 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
> [e-mail]
> https://lists.xenproject.org/mailman/listinfo/xen-devel

Julien Grall Feb. 4, 2020, 1:31 p.m. UTC | #2

On 04/02/2020 13:16, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Xen-devel <[e-mail]> On Behalf Of
>> Julien Grall
>> Sent: 04 February 2020 13:06
>> To: [e-mail]
>> Cc: [e-mail]; Wei Liu <[e-mail]>; George Dunlap
>> <[e-mail]>; Andrew Cooper <[e-mail]>;
>> Grall, Julien <[e-mail]>; Jan Beulich <[e-mail]>; Roger
>> Pau Monné <[e-mail]>
>> Subject: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden
>> hap_enable()
>>
>> From: Julien Grall <[e-mail]>
>>
>> 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 Feb. 13, 2020, 12:44 p.m. UTC | #3

Hi,

Gentle ping.

Cheers,

On 04/02/2020 14:06, Julien Grall wrote:
> From: Julien Grall <[e-mail]>
> 
> 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 <[e-mail]>
> 
> ---
> 
> 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 March 3, 2020, 12:21 p.m. UTC | #4

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 <[e-mail]>
>>
>> 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 <[e-mail]>
>>
>> ---
>>
>> 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++ )
>>       {
>>
>

Jan Beulich March 3, 2020, 1:25 p.m. UTC | #5

On 04.02.2020 14:06, Julien Grall wrote:
> From: Julien Grall <[e-mail]>
> 
> 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 <[e-mail]>

Acked-by: Jan Beulich <[e-mail]>
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

Jan Beulich March 3, 2020, 1:27 p.m. UTC | #6

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

Julien Grall March 3, 2020, 2:02 p.m. UTC | #7

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 March 4, 2020, 2:01 p.m. UTC | #8

On 03/03/2020 13:25, Jan Beulich wrote:
> On 04.02.2020 14:06, Julien Grall wrote:
>> From: Julien Grall <[e-mail]>
>>
>> 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 <[e-mail]>
> 
> Acked-by: Jan Beulich <[e-mail]>
> 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,

Patch

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++ )
     {