thr3ads.net - Xen devel - [PATCH] Extra check in grant table code for mapping of shared frame [Sep 2012]

If this information is useful, please help other people find it:
Share via:

Andres Lagar-Cavilla

2012-Sep-04 15:11 UTC

[PATCH] Extra check in grant table code for mapping of shared frame

xen/common/grant_table.c |  9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)


Small fix, please consider for 4.2. Thanks.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r 3a6050031b9f -r a18d6bd0d127 xen/common/grant_table.c
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -649,9 +649,12 @@ __gnttab_map_grant_ref(
     }
     else if ( owner == rd || owner == dom_cow )
     {
-        if ( gnttab_host_mapping_get_page_type(op, ld, rd) &&
-             !get_page_type(pg, PGT_writable_page) )
-            goto could_not_pin;
+        if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
+        {
+            if ( (owner == dom_cow) ||
+                 !get_page_type(pg, PGT_writable_page) )
+                goto could_not_pin;
+        }
 
         nr_gets++;
         if ( op->flags & GNTMAP_host_map )

Andres Lagar-Cavilla

2012-Sep-13 15:27 UTC

head link

[PATCH] Extra check in grant table code for mapping of shared frame

xen/common/grant_table.c |  9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)


Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r 5ce5b53ea68f -r 40b91bed1275 xen/common/grant_table.c
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -649,9 +649,12 @@ __gnttab_map_grant_ref(
     }
     else if ( owner == rd || owner == dom_cow )
     {
-        if ( gnttab_host_mapping_get_page_type(op, ld, rd) &&
-             !get_page_type(pg, PGT_writable_page) )
-            goto could_not_pin;
+        if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
+        {
+            if ( (owner == dom_cow) ||
+                 !get_page_type(pg, PGT_writable_page) )
+                goto could_not_pin;
+        }
 
         nr_gets++;
         if ( op->flags & GNTMAP_host_map )

Andres Lagar-Cavilla

2012-Sep-17 11:00 UTC

head link

Re: [PATCH] Extra check in grant table code for mapping of shared frame

ping…
Thanks,
Andres
On Sep 13, 2012, at 11:27 AM, Andres Lagar-Cavilla wrote:
> xen/common/grant_table.c |  9 ++++++---
> 1 files changed, 6 insertions(+), 3 deletions(-)
> 
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> 
> diff -r 5ce5b53ea68f -r 40b91bed1275 xen/common/grant_table.c
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -649,9 +649,12 @@ __gnttab_map_grant_ref(
>     }
>     else if ( owner == rd || owner == dom_cow )
>     {
> -        if ( gnttab_host_mapping_get_page_type(op, ld, rd) &&
> -             !get_page_type(pg, PGT_writable_page) )
> -            goto could_not_pin;
> +        if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
> +        {
> +            if ( (owner == dom_cow) ||
> +                 !get_page_type(pg, PGT_writable_page) )
> +                goto could_not_pin;
> +        }
> 
>         nr_gets++;
>         if ( op->flags & GNTMAP_host_map )

Keir Fraser

2012-Sep-17 11:17 UTC

head link

Re: [PATCH] Extra check in grant table code for mapping of shared frame

Probably needs Tim to comment on it. Assuming he''s any wiser about this
code
than the rest of us. ;)

 -- Keir

On 17/09/2012 12:00, "Andres Lagar-Cavilla"
<andreslc@gridcentric.ca> wrote:
> ping
> Thanks,
> Andres
> On Sep 13, 2012, at 11:27 AM, Andres Lagar-Cavilla wrote:
> 
>> xen/common/grant_table.c |  9 ++++++---
>> 1 files changed, 6 insertions(+), 3 deletions(-)
>> 
>> 
>> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>> 
>> diff -r 5ce5b53ea68f -r 40b91bed1275 xen/common/grant_table.c
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -649,9 +649,12 @@ __gnttab_map_grant_ref(
>>     }
>>     else if ( owner == rd || owner == dom_cow )
>>     {
>> -        if ( gnttab_host_mapping_get_page_type(op, ld, rd) &&
>> -             !get_page_type(pg, PGT_writable_page) )
>> -            goto could_not_pin;
>> +        if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
>> +        {
>> +            if ( (owner == dom_cow) ||
>> +                 !get_page_type(pg, PGT_writable_page) )
>> +                goto could_not_pin;
>> +        }
>> 
>>         nr_gets++;
>>         if ( op->flags & GNTMAP_host_map )
>

Tim Deegan

2012-Sep-17 16:29 UTC

head link

Re: [PATCH] Extra check in grant table code for mapping of shared frame

At 12:17 +0100 on 17 Sep (1347884247), Keir Fraser
wrote:> Probably needs Tim to comment on it. Assuming he''s any wiser about
this code
> than the rest of us. ;)
Looks OK to my limited understanding. :)

Tim.
> On 17/09/2012 12:00, "Andres Lagar-Cavilla"
<andreslc@gridcentric.ca> wrote:
> 
> > ping
> > Thanks,
> > Andres
> > On Sep 13, 2012, at 11:27 AM, Andres Lagar-Cavilla wrote:
> > 
> >> xen/common/grant_table.c |  9 ++++++---
> >> 1 files changed, 6 insertions(+), 3 deletions(-)
> >> 
> >> 
> >> Signed-off-by: Andres Lagar-Cavilla
<andres@lagarcavilla.org>
> >> 
> >> diff -r 5ce5b53ea68f -r 40b91bed1275 xen/common/grant_table.c
> >> --- a/xen/common/grant_table.c
> >> +++ b/xen/common/grant_table.c
> >> @@ -649,9 +649,12 @@ __gnttab_map_grant_ref(
> >>     }
> >>     else if ( owner == rd || owner == dom_cow )
> >>     {
> >> -        if ( gnttab_host_mapping_get_page_type(op, ld, rd)
&&
> >> -             !get_page_type(pg, PGT_writable_page) )
> >> -            goto could_not_pin;
> >> +        if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
> >> +        {
> >> +            if ( (owner == dom_cow) ||
> >> +                 !get_page_type(pg, PGT_writable_page) )
> >> +                goto could_not_pin;
> >> +        }
> >> 
> >>         nr_gets++;
> >>         if ( op->flags & GNTMAP_host_map )
> > 
> 
>

Jan Beulich

2012-Sep-19 15:35 UTC

head link

Re: [PATCH] Extra check in grant table code for mapping of shared frame

>>> On 13.09.12 at 17:27, Andres Lagar-Cavilla
<andres@lagarcavilla.org> wrote:
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -649,9 +649,12 @@ __gnttab_map_grant_ref(
>      }
>      else if ( owner == rd || owner == dom_cow )
>      {
> -        if ( gnttab_host_mapping_get_page_type(op, ld, rd) &&
> -             !get_page_type(pg, PGT_writable_page) )
> -            goto could_not_pin;
> +        if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
> +        {
> +            if ( (owner == dom_cow) ||
> +                 !get_page_type(pg, PGT_writable_page) )
> +                goto could_not_pin;
> +        }
>  
>          nr_gets++;
>          if ( op->flags & GNTMAP_host_map )
Isn''t that only half of it, in that the error/unmap paths need to
also consider that get_page_type() wasn''t called? There''s
quite a few calls to gnttab_host_mapping_get_page_type()/
put_page_type() sequences there.

Jan

Andres Lagar-Cavilla

2012-Sep-20 15:30 UTC

head link

Re: [PATCH] Extra check in grant table code for mapping of shared frame

On Sep 19, 2012, at 11:35 AM, Jan Beulich wrote:
>>>> On 13.09.12 at 17:27, Andres Lagar-Cavilla
<andres@lagarcavilla.org> wrote:
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -649,9 +649,12 @@ __gnttab_map_grant_ref(
>>     }
>>     else if ( owner == rd || owner == dom_cow )
>>     {
>> -        if ( gnttab_host_mapping_get_page_type(op, ld, rd) &&
>> -             !get_page_type(pg, PGT_writable_page) )
>> -            goto could_not_pin;
>> +        if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
>> +        {
>> +            if ( (owner == dom_cow) ||
>> +                 !get_page_type(pg, PGT_writable_page) )
>> +                goto could_not_pin;
>> +        }
>> 
>>         nr_gets++;
>>         if ( op->flags & GNTMAP_host_map )
> 
> Isn''t that only half of it, in that the error/unmap paths need to
> also consider that get_page_type() wasn''t called? There''s
> quite a few calls to gnttab_host_mapping_get_page_type()/
> put_page_type() sequences there.
I think this is covered. could_not_pin will cascade into undo_out, and nr_gets
remains at zero at this point. Then:
 undo_out:
    if ( nr_gets > 1 )
    {
       …
    }
    if ( nr_gets > 0 )
    {
        if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
            put_page_type(pg);
            ...

i.e. put_page_type will not be called. This is really tricky code!

Andres> 
> Jan
>

Jan Beulich

2012-Sep-20 15:50 UTC

head link

Re: [PATCH] Extra check in grant table code for mapping of shared frame

>>> On 20.09.12 at 17:30, Andres Lagar-Cavilla
<andreslc@gridcentric.ca> wrote:
> On Sep 19, 2012, at 11:35 AM, Jan Beulich wrote:
> 
>>>>> On 13.09.12 at 17:27, Andres Lagar-Cavilla
<andres@lagarcavilla.org> wrote:
>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -649,9 +649,12 @@ __gnttab_map_grant_ref(
>>>     }
>>>     else if ( owner == rd || owner == dom_cow )
>>>     {
>>> -        if ( gnttab_host_mapping_get_page_type(op, ld, rd)
&&
>>> -             !get_page_type(pg, PGT_writable_page) )
>>> -            goto could_not_pin;
>>> +        if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
>>> +        {
>>> +            if ( (owner == dom_cow) ||
>>> +                 !get_page_type(pg, PGT_writable_page) )
>>> +                goto could_not_pin;
>>> +        }
>>> 
>>>         nr_gets++;
>>>         if ( op->flags & GNTMAP_host_map )
>> 
>> Isn't that only half of it, in that the error/unmap paths need to
>> also consider that get_page_type() wasn't called? There's
>> quite a few calls to gnttab_host_mapping_get_page_type()/
>> put_page_type() sequences there.
> 
> I think this is covered. could_not_pin will cascade into undo_out, and 
> nr_gets remains at zero at this point. Then:
>  undo_out:
>     if ( nr_gets > 1 )
>     {
>        …
>     }
>     if ( nr_gets > 0 )
>     {
>         if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
>             put_page_type(pg);
>             ...
> 
> i.e. put_page_type will not be called. This is really tricky code!
Okay, that path indeed looks safe through this nr_gets use.

Oh, and I see, the other cases are of no concern because
the check you added leads directly to the failure path.

Thanks for clarifying,
Jan

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

Xen devel - Sep 2012 - [PATCH] Extra check in grant table code for mapping of shared frame

[PATCH] Extra check in grant table code for mapping of shared frame

[PATCH] Extra check in grant table code for mapping of shared frame

Re: [PATCH] Extra check in grant table code for mapping of shared frame

Re: [PATCH] Extra check in grant table code for mapping of shared frame

Re: [PATCH] Extra check in grant table code for mapping of shared frame

Re: [PATCH] Extra check in grant table code for mapping of shared frame

Re: [PATCH] Extra check in grant table code for mapping of shared frame

Re: [PATCH] Extra check in grant table code for mapping of shared frame