Xen devel - Sep 2009 - [PATCH] x86: add an extra check when validating a huge pv L2 entry

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 )




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

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



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

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


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

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

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

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.



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