[PATCH] xen: avoid race in p2m handling

[PATCH] xen: avoid race in p2m handling

[Juergen Gross]

When a new p2m leaf is allocated this leaf is linked into the p2m tree
via cmpxchg. Unfortunately the compare value for checking the success
of the update is read after checking for the need of a new leaf. It is
possible that a new leaf has been linked into the tree concurrently
in between. This could lead to a leaked memory page and to the loss of
some p2m entries.

Avoid the race by using the read compare value for checking the need
of a new p2m leaf.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/xen/p2m.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
index d1b3da2..8c1a278 100644
--- a/arch/x86/xen/p2m.c
+++ b/arch/x86/xen/p2m.c
@@ -532,6 +532,7 @@ static bool alloc_p2m(unsigned long pfn)
 	unsigned topidx, mididx;
 	unsigned long ***top_p, **mid;
 	unsigned long *top_mfn_p, *mid_mfn;
+	unsigned long *p2m_orig;
 
 	topidx = p2m_top_index(pfn);
 	mididx = p2m_mid_index(pfn);
@@ -579,11 +580,10 @@ static bool alloc_p2m(unsigned long pfn)
 		}
 	}
 
-	if (p2m_top[topidx][mididx] == p2m_identity ||
-	    p2m_top[topidx][mididx] == p2m_missing) {
+	p2m_orig = p2m_top[topidx][mididx];
+	if (p2m_orig == p2m_identity || p2m_orig == p2m_missing) {
 		/* p2m leaf page is missing */
 		unsigned long *p2m;
-		unsigned long *p2m_orig = p2m_top[topidx][mididx];
 
 		p2m = alloc_p2m_page();
 		if (!p2m)
-- 
1.8.4.5

Re: [PATCH] xen: avoid race in p2m handling

[David Vrabel]

On 16/10/14 07:13, Juergen Gross wrote:
> When a new p2m leaf is allocated this leaf is linked into the p2m tree
> via cmpxchg. Unfortunately the compare value for checking the success
> of the update is read after checking for the need of a new leaf. It is
> possible that a new leaf has been linked into the tree concurrently
> in between. This could lead to a leaked memory page and to the loss of
> some p2m entries.
> 
> Avoid the race by using the read compare value for checking the need
> of a new p2m leaf.
[...]
> @@ -579,11 +580,10 @@ static bool alloc_p2m(unsigned long pfn)
>  		}
>  	}
>  
> -	if (p2m_top[topidx][mididx] == p2m_identity ||
> -	    p2m_top[topidx][mididx] == p2m_missing) {
> +	p2m_orig = p2m_top[topidx][mididx];

Do you need to use ACCESS_ONCE() here?

David

Re: [PATCH] xen: avoid race in p2m handling

[Juergen Gross]

On 10/16/2014 05:50 PM, David Vrabel wrote:
> On 16/10/14 07:13, Juergen Gross wrote:
>> When a new p2m leaf is allocated this leaf is linked into the p2m tree
>> via cmpxchg. Unfortunately the compare value for checking the success
>> of the update is read after checking for the need of a new leaf. It is
>> possible that a new leaf has been linked into the tree concurrently
>> in between. This could lead to a leaked memory page and to the loss of
>> some p2m entries.
>>
>> Avoid the race by using the read compare value for checking the need
>> of a new p2m leaf.
> [...]
>> @@ -579,11 +580,10 @@ static bool alloc_p2m(unsigned long pfn)
>>   		}
>>   	}
>>
>> -	if (p2m_top[topidx][mididx] == p2m_identity ||
>> -	    p2m_top[topidx][mididx] == p2m_missing) {
>> +	p2m_orig = p2m_top[topidx][mididx];
>
> Do you need to use ACCESS_ONCE() here?

Yes, you are probably right. Should I send a new patch or do you want
to modify it?

Juergen

Re: [Xen-devel] [PATCH] xen: avoid race in p2m handling

[David Vrabel]

On 17/10/14 05:23, Juergen Gross wrote:
> On 10/16/2014 05:50 PM, David Vrabel wrote:
>> On 16/10/14 07:13, Juergen Gross wrote:
>>> When a new p2m leaf is allocated this leaf is linked into the p2m tree
>>> via cmpxchg. Unfortunately the compare value for checking the success
>>> of the update is read after checking for the need of a new leaf. It is
>>> possible that a new leaf has been linked into the tree concurrently
>>> in between. This could lead to a leaked memory page and to the loss of
>>> some p2m entries.
>>>
>>> Avoid the race by using the read compare value for checking the need
>>> of a new p2m leaf.
>> [...]
>>> @@ -579,11 +580,10 @@ static bool alloc_p2m(unsigned long pfn)
>>>           }
>>>       }
>>>
>>> -    if (p2m_top[topidx][mididx] == p2m_identity ||
>>> -        p2m_top[topidx][mididx] == p2m_missing) {
>>> +    p2m_orig = p2m_top[topidx][mididx];
>>
>> Do you need to use ACCESS_ONCE() here?
> 
> Yes, you are probably right. Should I send a new patch or do you want
> to modify it?

Can you go through and see if there are any other places in the p2m code
that also need ACCESS_ONCE()? And then repost, thanks!

David