Advanced
Mailing List Archive

Mailing List Archive

  1. Home >
  2. Xen>
  3. Devel
[PATCH] Extra check in grant table code for mapping of shared frame
andres at lagarcavilla
Sep 4, 2012, 8:11 AM
Post #1 of 8 (330 views)
Permalink
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 )

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[PATCH] Extra check in grant table code for mapping of shared frame [ In reply to ]
andres at lagarcavilla
Sep 13, 2012, 8:22 AM
Post #2 of 8 (310 views)
Permalink
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 )

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH] Extra check in grant table code for mapping of shared frame [ In reply to ]
andreslc at gridcentric
Sep 17, 2012, 4:00 AM
Post #3 of 8 (308 views)
Permalink
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 )


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH] Extra check in grant table code for mapping of shared frame [ In reply to ]
keir at xen
Sep 17, 2012, 4:17 AM
Post #4 of 8 (308 views)
Permalink
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 )
>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH] Extra check in grant table code for mapping of shared frame [ In reply to ]
tim at xen
Sep 17, 2012, 9:29 AM
Post #5 of 8 (309 views)
Permalink
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 )
> >
>
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH] Extra check in grant table code for mapping of shared frame [ In reply to ]
JBeulich at suse
Sep 19, 2012, 8:35 AM
Post #6 of 8 (311 views)
Permalink
>>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH] Extra check in grant table code for mapping of shared frame [ In reply to ]
andreslc at gridcentric
Sep 20, 2012, 8:30 AM
Post #7 of 8 (307 views)
Permalink
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
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH] Extra check in grant table code for mapping of shared frame [ In reply to ]
JBeulich at suse
Sep 20, 2012, 8:50 AM
Post #8 of 8 (307 views)
Permalink
>>> 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

©2024 GT.net. A Gossamer Threads company.
5th Floor, 455 Granville St., Vancouver, BC V6C 1T1, Canada | Legal