[PATCH] x86: add an extra check when validating a huge pv L2 entry

[PATCH] x86: add an extra check when validating a huge pv L2 entry

[Jan Beulich]

While get_page_and_type_from_pagenr() (through get_page_from_pagenr())
does the needed mfn_valid() check, get_data_page() doesn't and, it
being passed a struct page_info pointer, really expects it's caller(s)
to do.

Signed-off-by: Jan Beulich <jbeulich@novell.com>

--- 2009-08-18.orig/xen/arch/x86/mm.c	2009-09-09 13:32:06.000000000 +0200
+++ 2009-08-18/xen/arch/x86/mm.c	2009-09-09 13:35:02.000000000 +0200
@@ -868,7 +868,10 @@ get_page_from_l2e(
         int writeable = !!(l2e_get_flags(l2e) & _PAGE_RW);
   
         do {
-            rc = get_data_page(mfn_to_page(m), d, writeable);
+            if ( mfn_valid(m) )
+                rc = get_data_page(mfn_to_page(m), d, writeable);
+            else
+                rc = -EINVAL;
             if ( unlikely(!rc) )
             {
                 while ( m-- > mfn )

Re: [PATCH] x86: add an extra check when validating a huge pv L2 entry

[Keir Fraser]

On 09/09/2009 16:19, "Jan Beulich" <JBeulich@novell.com> wrote:

> -            rc = get_data_page(mfn_to_page(m), d, writeable);
> +            if ( mfn_valid(m) )
> +                rc = get_data_page(mfn_to_page(m), d, writeable);
> +            else
> +                rc = -EINVAL;

'else rc = 0' would be more like it, eh?

 -- Keir

Re: [PATCH] x86: add an extra check when validating a huge pv L2 entry

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 09.09.09 17:35 >>>
>On 09/09/2009 16:19, "Jan Beulich" <JBeulich@novell.com> wrote:
>
>> -            rc = get_data_page(mfn_to_page(m), d, writeable);
>> +            if ( mfn_valid(m) )
>> +                rc = get_data_page(mfn_to_page(m), d, writeable);
>> +            else
>> +                rc = -EINVAL;
>
>'else rc = 0' would be more like it, eh?

Oh, yes - I got confused (again) by the inconsistencies in what return
values mean for the various functions.

Jan

Re: [PATCH] x86: add an extra check when validating a huge pv L2 entry

[Michael J Coss]

Jan Beulich wrote:
>>>> Keir Fraser <keir.fraser@eu.citrix.com> 09.09.09 17:35 >>>
>>>>         
>> On 09/09/2009 16:19, "Jan Beulich" <JBeulich@novell.com> wrote:
>>
>>     
>>> -            rc = get_data_page(mfn_to_page(m), d, writeable);
>>> +            if ( mfn_valid(m) )
>>> +                rc = get_data_page(mfn_to_page(m), d, writeable);
>>> +            else
>>> +                rc = -EINVAL;
>>>       
>> 'else rc = 0' would be more like it, eh?
>>     
>
> Oh, yes - I got confused (again) by the inconsistencies in what return
> values mean for the various functions.
>
> Jan
>   
I don't think that that's correct.  The else clause is for the 
mfn_valid(m) failure case, and it seems appropriate to set the rc to 
some non-zero value.

---Michael J Coss

Re: [PATCH] x86: add an extra check when validating a huge pv L2 entry

[Keir Fraser]

On 09/09/2009 17:15, "Michael J Coss" <mjcoss@alcatel-lucent.com> wrote:

>> Oh, yes - I got confused (again) by the inconsistencies in what return
>> values mean for the various functions.
>> 
>> Jan
>>   
> I don't think that that's correct.  The else clause is for the
> mfn_valid(m) failure case, and it seems appropriate to set the rc to
> some non-zero value.

You're confused - look at the surrounding context of the patch. Anyway, I
ultimately checked in soemthing even clearer, as c/s 20189.

 K.