All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] arm: p2m.c bug-fix: hypervisor hang on __p2m_get_mem_access
@ 2016-01-26 11:46 Corneliu ZUZU
  2016-01-26 16:14 ` Ian Campbell
  2016-02-03 11:37 ` [PATCH v2] " Corneliu ZUZU
  0 siblings, 2 replies; 6+ messages in thread
From: Corneliu ZUZU @ 2016-01-26 11:46 UTC (permalink / raw)
  To: xen-devel; +Cc: Stefano Stabellini, Ian Campbell

When __p2m_get_mem_access gets called, the p2m lock is already taken
by either get_page_from_gva or p2m_get_mem_access.

Possible code paths:
1)	-> get_page_from_gva
		-> p2m_mem_access_check_and_get_page
			-> __p2m_get_mem_access
2)	-> p2m_get_mem_access
		-> __p2m_get_mem_access

In both cases if __p2m_get_mem_access subsequently gets to
call p2m_lookup (happens if !radix_tree_lookup(...)), a hypervisor
hang will occur, since p2m_lookup also spin-locks on the p2m lock.

This bug-fix simply replaces the p2m_lookup call from __p2m_get_mem_access
with a call to __p2m_lookup.

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
 xen/arch/arm/p2m.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 2190908..a9157e5 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -490,7 +490,7 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
          * No setting was found in the Radix tree. Check if the
          * entry exists in the page-tables.
          */
-        paddr_t maddr = p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
+        paddr_t maddr = __p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
         if ( INVALID_PADDR == maddr )
             return -ESRCH;
 
-- 
2.5.0

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] arm: p2m.c bug-fix: hypervisor hang on __p2m_get_mem_access
  2016-01-26 11:46 [PATCH] arm: p2m.c bug-fix: hypervisor hang on __p2m_get_mem_access Corneliu ZUZU
@ 2016-01-26 16:14 ` Ian Campbell
       [not found]   ` <56A89338.8040409@bitdefender.com>
  2016-02-03 11:37 ` [PATCH v2] " Corneliu ZUZU
  1 sibling, 1 reply; 6+ messages in thread
From: Ian Campbell @ 2016-01-26 16:14 UTC (permalink / raw)
  To: Corneliu ZUZU, xen-devel; +Cc: Stefano Stabellini

On Tue, 2016-01-26 at 13:46 +0200, Corneliu ZUZU wrote:
> When __p2m_get_mem_access gets called, the p2m lock is already taken
> by either get_page_from_gva or p2m_get_mem_access.
> 
> Possible code paths:
> 1)	-> get_page_from_gva
> 		-> p2m_mem_access_check_and_get_page
> 			-> __p2m_get_mem_access
> 2)	-> p2m_get_mem_access
> 		-> __p2m_get_mem_access

What about:
	-> p2m_mem_access_check
		-> p2m_get_mem_access
I can't see the lock being taken in that paths.

As well as fixing that I think it would be wise to add an assert that the
lock is held before the call to p2m_lookup which you are changing into the
unlocked variant.

> 
> In both cases if __p2m_get_mem_access subsequently gets to
> call p2m_lookup (happens if !radix_tree_lookup(...)), a hypervisor
> hang will occur, since p2m_lookup also spin-locks on the p2m lock.
> 
> This bug-fix simply replaces the p2m_lookup call from
> __p2m_get_mem_access
> with a call to __p2m_lookup.
> 
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> ---
>  xen/arch/arm/p2m.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 2190908..a9157e5 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -490,7 +490,7 @@ static int __p2m_get_mem_access(struct domain *d,
> gfn_t gfn,
>           * No setting was found in the Radix tree. Check if the
>           * entry exists in the page-tables.
>           */
> -        paddr_t maddr = p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
> +        paddr_t maddr = __p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
>          if ( INVALID_PADDR == maddr )
>              return -ESRCH;
>  

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] arm: p2m.c bug-fix: hypervisor hang on __p2m_get_mem_access
       [not found]   ` <56A89338.8040409@bitdefender.com>
@ 2016-01-27 11:42     ` Ian Campbell
  0 siblings, 0 replies; 6+ messages in thread
From: Ian Campbell @ 2016-01-27 11:42 UTC (permalink / raw)
  To: CORNELIU ZUZU, xen-devel

(we went offlist by mistake without noticing, resending my last reply which
I think has sufficient context/quoting to make sense)

On Wed, 2016-01-27 at 11:51 +0200, CORNELIU ZUZU wrote:
> On 1/26/2016 6:14 PM, Ian Campbell wrote:
> > On Tue, 2016-01-26 at 13:46 +0200, Corneliu ZUZU wrote:
> > > When __p2m_get_mem_access gets called, the p2m lock is already taken
> > > by either get_page_from_gva or p2m_get_mem_access.
> > > 
> > > Possible code paths:
> > > 1)	-> get_page_from_gva
> > > 		-> p2m_mem_access_check_and_get_page
> > > 			-> __p2m_get_mem_access
> > > 2)	-> p2m_get_mem_access
> > > 		-> __p2m_get_mem_access
> > What about:
> > 	-> p2m_mem_access_check
> > 		-> p2m_get_mem_access
> > I can't see the lock being taken in that paths.
> 
> Yes, that's code path #2 I've previously mentioned, the lock is first 
> taken in p2m_get_mem_access (i.e. the line 'spin_lock(&p2m->lock)'),
> before __p2m_get_mem_access is called.

Oh yes, not sure how I got myself confused there.

> I'm looking @ the master branch of git://xenbits.xenproject.org/xen.git 
> (although we've hit this bug on the 4.6 stable repo).
> 
> > 
> > As well as fixing that I think it would be wise to add an assert that
> > the
> > lock is held before the call to p2m_lookup which you are changing into
> > the
> > unlocked variant.
> 
> Good idea, didn't know one could do that.
> Concretely, I suppose this would be done by
> 
> ASSERT(spin_is_locked(&p2m->lock));
> 
> ?

Right.

> 
> One question though. It seems that everytime __p2m_get_mem_access is 
> called, the lock is taken *before the call*, not within the
> function itself.

Right. It's a common (but by no mean universal) convention within Xen that
a __foo function is a version of foo which expects the relevant lock(s) to
already be held.

> Since __p2m_get_mem_access also accesses other fields of the p2m pointer 
> (e.g. 'p2m->mem_access_enabled'),
> including performing a radix tree lookup before calling p2m_lookup, 
> doesn't this mean that
> the p2m lock is also needed for these accesses (especially for the radix 
> tree lookup)? And if so,
> shouldn't I position the assertion line @ the *beginning* of 
> __p2m_get_mem_access, e.g. just before 'if ( !p2m->mem_access_enabled )'?

Yes, since p2m_get_mem_access requires the lock to be taken on entry it
makes sense to put the assert as near the top of the function as possible. 

This is true whether the initial bit of the function does anything which
requires the lock.

Ian.

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2] arm: p2m.c bug-fix: hypervisor hang on __p2m_get_mem_access
  2016-01-26 11:46 [PATCH] arm: p2m.c bug-fix: hypervisor hang on __p2m_get_mem_access Corneliu ZUZU
  2016-01-26 16:14 ` Ian Campbell
@ 2016-02-03 11:37 ` Corneliu ZUZU
  2016-02-03 11:52   ` Ian Campbell
  1 sibling, 1 reply; 6+ messages in thread
From: Corneliu ZUZU @ 2016-02-03 11:37 UTC (permalink / raw)
  To: xen-devel; +Cc: Stefano Stabellini, Ian Campbell

When __p2m_get_mem_access gets called, the p2m lock is already taken
by either get_page_from_gva or p2m_get_mem_access.
Possible code paths:
1)	-> get_page_from_gva
		-> p2m_mem_access_check_and_get_page
			-> __p2m_get_mem_access
2)	-> p2m_get_mem_access
		-> __p2m_get_mem_access

In both cases if __p2m_get_mem_access subsequently gets to
call p2m_lookup (happens if !radix_tree_lookup(...)), a hypervisor
hang will occur, since p2m_lookup also spin-locks on the p2m lock.

This bug-fix simply replaces the p2m_lookup call from __p2m_get_mem_access
with a call to __p2m_lookup and also adds an ASSERT to ensure that the p2m lock
is already taken upon __p2m_get_mem_access entry.

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>

---
Changed since v1:
  * added p2m-lock ASSERT
---
 xen/arch/arm/p2m.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 2190908..e8e6db4 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -468,6 +468,8 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
 #undef ACCESS
     };
 
+    ASSERT(spin_is_locked(&p2m->lock));
+
     /* If no setting was ever set, just return rwx. */
     if ( !p2m->mem_access_enabled )
     {
@@ -490,7 +492,7 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
          * No setting was found in the Radix tree. Check if the
          * entry exists in the page-tables.
          */
-        paddr_t maddr = p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
+        paddr_t maddr = __p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
         if ( INVALID_PADDR == maddr )
             return -ESRCH;
 
-- 
2.5.0

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] arm: p2m.c bug-fix: hypervisor hang on __p2m_get_mem_access
  2016-02-03 11:37 ` [PATCH v2] " Corneliu ZUZU
@ 2016-02-03 11:52   ` Ian Campbell
  2016-02-03 11:56     ` Corneliu ZUZU
  0 siblings, 1 reply; 6+ messages in thread
From: Ian Campbell @ 2016-02-03 11:52 UTC (permalink / raw)
  To: Corneliu ZUZU, xen-devel; +Cc: Stefano Stabellini

On Wed, 2016-02-03 at 13:37 +0200, Corneliu ZUZU wrote:

I just now applied a previous v2 which was already in my queue. Was this
just an accidental resend of v2 or is there some important change and this
is really a v3?

> When __p2m_get_mem_access gets called, the p2m lock is already taken
> by either get_page_from_gva or p2m_get_mem_access.
> Possible code paths:
> 1)	-> get_page_from_gva
> 		-> p2m_mem_access_check_and_get_page
> 			-> __p2m_get_mem_access
> 2)	-> p2m_get_mem_access
> 		-> __p2m_get_mem_access
> 
> In both cases if __p2m_get_mem_access subsequently gets to
> call p2m_lookup (happens if !radix_tree_lookup(...)), a hypervisor
> hang will occur, since p2m_lookup also spin-locks on the p2m lock.
> 
> This bug-fix simply replaces the p2m_lookup call from
> __p2m_get_mem_access
> with a call to __p2m_lookup and also adds an ASSERT to ensure that the
> p2m lock
> is already taken upon __p2m_get_mem_access entry.
> 
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> 
> ---
> Changed since v1:
>   * added p2m-lock ASSERT
> ---
>  xen/arch/arm/p2m.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 2190908..e8e6db4 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -468,6 +468,8 @@ static int __p2m_get_mem_access(struct domain *d,
> gfn_t gfn,
>  #undef ACCESS
>      };
>  
> +    ASSERT(spin_is_locked(&p2m->lock));
> +
>      /* If no setting was ever set, just return rwx. */
>      if ( !p2m->mem_access_enabled )
>      {
> @@ -490,7 +492,7 @@ static int __p2m_get_mem_access(struct domain *d,
> gfn_t gfn,
>           * No setting was found in the Radix tree. Check if the
>           * entry exists in the page-tables.
>           */
> -        paddr_t maddr = p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
> +        paddr_t maddr = __p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
>          if ( INVALID_PADDR == maddr )
>              return -ESRCH;
>  

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] arm: p2m.c bug-fix: hypervisor hang on __p2m_get_mem_access
  2016-02-03 11:52   ` Ian Campbell
@ 2016-02-03 11:56     ` Corneliu ZUZU
  0 siblings, 0 replies; 6+ messages in thread
From: Corneliu ZUZU @ 2016-02-03 11:56 UTC (permalink / raw)
  To: xen-devel

On 2/3/2016 1:52 PM, Ian Campbell wrote:
> On Wed, 2016-02-03 at 13:37 +0200, Corneliu ZUZU wrote:
>
> I just now applied a previous v2 which was already in my queue. Was this
> just an accidental resend of v2 or is there some important change and this
> is really a v3?
>
>> When __p2m_get_mem_access gets called, the p2m lock is already taken
>> by either get_page_from_gva or p2m_get_mem_access.
>> Possible code paths:
>> 1)	-> get_page_from_gva
>> 		-> p2m_mem_access_check_and_get_page
>> 			-> __p2m_get_mem_access
>> 2)	-> p2m_get_mem_access
>> 		-> __p2m_get_mem_access
>>
>> In both cases if __p2m_get_mem_access subsequently gets to
>> call p2m_lookup (happens if !radix_tree_lookup(...)), a hypervisor
>> hang will occur, since p2m_lookup also spin-locks on the p2m lock.
>>
>> This bug-fix simply replaces the p2m_lookup call from
>> __p2m_get_mem_access
>> with a call to __p2m_lookup and also adds an ASSERT to ensure that the
>> p2m lock
>> is already taken upon __p2m_get_mem_access entry.
>>
>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
>>
>> ---
>> Changed since v1:
>>    * added p2m-lock ASSERT
>> ---
>>   xen/arch/arm/p2m.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 2190908..e8e6db4 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -468,6 +468,8 @@ static int __p2m_get_mem_access(struct domain *d,
>> gfn_t gfn,
>>   #undef ACCESS
>>       };
>>   
>> +    ASSERT(spin_is_locked(&p2m->lock));
>> +
>>       /* If no setting was ever set, just return rwx. */
>>       if ( !p2m->mem_access_enabled )
>>       {
>> @@ -490,7 +492,7 @@ static int __p2m_get_mem_access(struct domain *d,
>> gfn_t gfn,
>>            * No setting was found in the Radix tree. Check if the
>>            * entry exists in the page-tables.
>>            */
>> -        paddr_t maddr = p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
>> +        paddr_t maddr = __p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
>>           if ( INVALID_PADDR == maddr )
>>               return -ESRCH;
>>   
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
No, sorry, this is just a duplicate of the 1st v2, I thought the first 
one was not sent properly (after waiting a few days and noticing
I was no longer finding it on the web).
Ignore this one. And thanks.

Corneliu.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-02-03 11:56 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-26 11:46 [PATCH] arm: p2m.c bug-fix: hypervisor hang on __p2m_get_mem_access Corneliu ZUZU
2016-01-26 16:14 ` Ian Campbell
     [not found]   ` <56A89338.8040409@bitdefender.com>
2016-01-27 11:42     ` Ian Campbell
2016-02-03 11:37 ` [PATCH v2] " Corneliu ZUZU
2016-02-03 11:52   ` Ian Campbell
2016-02-03 11:56     ` Corneliu ZUZU

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.