[PATCH v2] xen/p2m: Fix p2m_flush_table for non-nested cases

[PATCH v2] xen/p2m: Fix p2m_flush_table for non-nested cases

[George Dunlap]

Commit 71bb7304e7a7a35ea6df4b0cedebc35028e4c159 added flushing of
nested p2m tables whenever the host p2m table changed.  Unfortunately
in the process, it added a filter to p2m_flush_table() function so
that the p2m would only be flushed if it was being used as a nested
p2m.  This meant that the p2m was not being flushed at all for altp2m
callers.

Only check np2m_base if p2m_class is set to p2m_nested.

NB that this is not a security issue: The only time this codepath is
called is in cases where either nestedp2m or altp2m is enabled, and
neither of them are in security support.

Reported-by: Matt Leinhos <matt@starlab.io>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Tamas K Lengyel <tamas.lengyel@zentific.com>
---
 xen/arch/x86/mm/p2m.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 6548e9f..0af2ec1 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1627,7 +1627,9 @@ p2m_flush_table(struct p2m_domain *p2m)
     ASSERT(page_list_empty(&p2m->pod.super));
     ASSERT(page_list_empty(&p2m->pod.single));
 
-    if ( p2m->np2m_base == P2M_BASE_EADDR )
+    /* No need to flush if it's already empty */
+    if ( p2m->p2m_class == p2m_nested &&
+         p2m->np2m_base == P2M_BASE_EADDR )
     {
         p2m_unlock(p2m);
         return;
-- 
2.1.4


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

Re: [PATCH v2] xen/p2m: Fix p2m_flush_table for non-nested cases

[Tim Deegan]

At 17:22 +0000 on 08 Feb (1486574546), George Dunlap wrote:
> Commit 71bb7304e7a7a35ea6df4b0cedebc35028e4c159 added flushing of
> nested p2m tables whenever the host p2m table changed.  Unfortunately
> in the process, it added a filter to p2m_flush_table() function so
> that the p2m would only be flushed if it was being used as a nested
> p2m.  This meant that the p2m was not being flushed at all for altp2m
> callers.
> 
> Only check np2m_base if p2m_class is set to p2m_nested.
> 
> NB that this is not a security issue: The only time this codepath is
> called is in cases where either nestedp2m or altp2m is enabled, and
> neither of them are in security support.
> 
> Reported-by: Matt Leinhos <matt@starlab.io>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> CC: Tamas K Lengyel <tamas.lengyel@zentific.com>
> ---
>  xen/arch/x86/mm/p2m.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 6548e9f..0af2ec1 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1627,7 +1627,9 @@ p2m_flush_table(struct p2m_domain *p2m)
>      ASSERT(page_list_empty(&p2m->pod.super));
>      ASSERT(page_list_empty(&p2m->pod.single));
>  
> -    if ( p2m->np2m_base == P2M_BASE_EADDR )
> +    /* No need to flush if it's already empty */
> +    if ( p2m->p2m_class == p2m_nested &&
> +         p2m->np2m_base == P2M_BASE_EADDR )

Looks like p2m_is_nestedp2m(p2m) is the usual idiom.  Either way:

Reviewed-by: Tim Deegan <tim@xen.org>

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

Re: [PATCH v2] xen/p2m: Fix p2m_flush_table for non-nested cases

[George Dunlap]

On 08/02/17 17:29, Tim Deegan wrote:
> At 17:22 +0000 on 08 Feb (1486574546), George Dunlap wrote:
>> Commit 71bb7304e7a7a35ea6df4b0cedebc35028e4c159 added flushing of
>> nested p2m tables whenever the host p2m table changed.  Unfortunately
>> in the process, it added a filter to p2m_flush_table() function so
>> that the p2m would only be flushed if it was being used as a nested
>> p2m.  This meant that the p2m was not being flushed at all for altp2m
>> callers.
>>
>> Only check np2m_base if p2m_class is set to p2m_nested.
>>
>> NB that this is not a security issue: The only time this codepath is
>> called is in cases where either nestedp2m or altp2m is enabled, and
>> neither of them are in security support.
>>
>> Reported-by: Matt Leinhos <matt@starlab.io>
>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>> CC: Tamas K Lengyel <tamas.lengyel@zentific.com>
>> ---
>>  xen/arch/x86/mm/p2m.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index 6548e9f..0af2ec1 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -1627,7 +1627,9 @@ p2m_flush_table(struct p2m_domain *p2m)
>>      ASSERT(page_list_empty(&p2m->pod.super));
>>      ASSERT(page_list_empty(&p2m->pod.single));
>>  
>> -    if ( p2m->np2m_base == P2M_BASE_EADDR )
>> +    /* No need to flush if it's already empty */
>> +    if ( p2m->p2m_class == p2m_nested &&
>> +         p2m->np2m_base == P2M_BASE_EADDR )
> 
> Looks like p2m_is_nestedp2m(p2m) is the usual idiom.  Either way:
> 
> Reviewed-by: Tim Deegan <tim@xen.org>

Right -- I'll change it to the usual idiom and check it in after I've
gotten a test report (or waited a reasonable amount of time).

 -George


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

Re: [PATCH v2] xen/p2m: Fix p2m_flush_table for non-nested cases

[Tamas Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 1955 bytes --]

On Wed, Feb 8, 2017 at 11:12 AM, George Dunlap <george.dunlap@citrix.com>
wrote:

> On 08/02/17 17:29, Tim Deegan wrote:
> > At 17:22 +0000 on 08 Feb (1486574546), George Dunlap wrote:
> >> Commit 71bb7304e7a7a35ea6df4b0cedebc35028e4c159 added flushing of
> >> nested p2m tables whenever the host p2m table changed.  Unfortunately
> >> in the process, it added a filter to p2m_flush_table() function so
> >> that the p2m would only be flushed if it was being used as a nested
> >> p2m.  This meant that the p2m was not being flushed at all for altp2m
> >> callers.
> >>
> >> Only check np2m_base if p2m_class is set to p2m_nested.
> >>
> >> NB that this is not a security issue: The only time this codepath is
> >> called is in cases where either nestedp2m or altp2m is enabled, and
> >> neither of them are in security support.
> >>
> >> Reported-by: Matt Leinhos <matt@starlab.io>
> >> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> >> CC: Tamas K Lengyel <tamas.lengyel@zentific.com>
> >> ---
> >>  xen/arch/x86/mm/p2m.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> >> index 6548e9f..0af2ec1 100644
> >> --- a/xen/arch/x86/mm/p2m.c
> >> +++ b/xen/arch/x86/mm/p2m.c
> >> @@ -1627,7 +1627,9 @@ p2m_flush_table(struct p2m_domain *p2m)
> >>      ASSERT(page_list_empty(&p2m->pod.super));
> >>      ASSERT(page_list_empty(&p2m->pod.single));
> >>
> >> -    if ( p2m->np2m_base == P2M_BASE_EADDR )
> >> +    /* No need to flush if it's already empty */
> >> +    if ( p2m->p2m_class == p2m_nested &&
> >> +         p2m->np2m_base == P2M_BASE_EADDR )
> >
> > Looks like p2m_is_nestedp2m(p2m) is the usual idiom.  Either way:
> >
> > Reviewed-by: Tim Deegan <tim@xen.org>
>
> Right -- I'll change it to the usual idiom and check it in after I've
> gotten a test report (or waited a reasonable amount of time).


Tested-by: Tamas K Lengyel <tamas@tklengyel.com>

[-- Attachment #1.2: Type: text/html, Size: 2976 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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