* [PATCH] Extra check in grant table code for mapping of shared frame
@ 2012-09-04 15:11 Andres Lagar-Cavilla
0 siblings, 0 replies; 8+ messages in thread
From: Andres Lagar-Cavilla @ 2012-09-04 15:11 UTC (permalink / raw)
To: xen-devel; +Cc: keir, andres, tim, JBeulich
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 )
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] Extra check in grant table code for mapping of shared frame
@ 2012-09-13 15:27 Andres Lagar-Cavilla
2012-09-17 11:00 ` Andres Lagar-Cavilla
2012-09-19 15:35 ` Jan Beulich
0 siblings, 2 replies; 8+ messages in thread
From: Andres Lagar-Cavilla @ 2012-09-13 15:27 UTC (permalink / raw)
To: xen-devel; +Cc: keir, andres, tim, JBeulich
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 )
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Extra check in grant table code for mapping of shared frame
2012-09-13 15:27 Andres Lagar-Cavilla
@ 2012-09-17 11:00 ` Andres Lagar-Cavilla
2012-09-17 11:17 ` Keir Fraser
2012-09-19 15:35 ` Jan Beulich
1 sibling, 1 reply; 8+ messages in thread
From: Andres Lagar-Cavilla @ 2012-09-17 11:00 UTC (permalink / raw)
To: Andres Lagar-Cavilla; +Cc: keir, tim, JBeulich, xen-devel
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 )
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Extra check in grant table code for mapping of shared frame
2012-09-17 11:00 ` Andres Lagar-Cavilla
@ 2012-09-17 11:17 ` Keir Fraser
2012-09-17 16:29 ` Tim Deegan
0 siblings, 1 reply; 8+ messages in thread
From: Keir Fraser @ 2012-09-17 11:17 UTC (permalink / raw)
To: Andres Lagar-Cavilla, Andres Lagar-Cavilla; +Cc: tim, JBeulich, xen-devel
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 )
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Extra check in grant table code for mapping of shared frame
2012-09-17 11:17 ` Keir Fraser
@ 2012-09-17 16:29 ` Tim Deegan
0 siblings, 0 replies; 8+ messages in thread
From: Tim Deegan @ 2012-09-17 16:29 UTC (permalink / raw)
To: Keir Fraser
Cc: Andres Lagar-Cavilla, Andres Lagar-Cavilla, JBeulich, xen-devel
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 )
> >
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Extra check in grant table code for mapping of shared frame
2012-09-13 15:27 Andres Lagar-Cavilla
2012-09-17 11:00 ` Andres Lagar-Cavilla
@ 2012-09-19 15:35 ` Jan Beulich
2012-09-20 15:30 ` Andres Lagar-Cavilla
1 sibling, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2012-09-19 15:35 UTC (permalink / raw)
To: Andres Lagar-Cavilla; +Cc: tim, andres, keir, xen-devel
>>> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Extra check in grant table code for mapping of shared frame
2012-09-19 15:35 ` Jan Beulich
@ 2012-09-20 15:30 ` Andres Lagar-Cavilla
2012-09-20 15:50 ` Jan Beulich
0 siblings, 1 reply; 8+ messages in thread
From: Andres Lagar-Cavilla @ 2012-09-20 15:30 UTC (permalink / raw)
To: Jan Beulich; +Cc: tim, keir, Andres Lagar-Cavilla, xen-devel
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Extra check in grant table code for mapping of shared frame
2012-09-20 15:30 ` Andres Lagar-Cavilla
@ 2012-09-20 15:50 ` Jan Beulich
0 siblings, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2012-09-20 15:50 UTC (permalink / raw)
To: Andres Lagar-Cavilla; +Cc: tim, keir, Andres Lagar-Cavilla, xen-devel
>>> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-09-20 15:50 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-04 15:11 [PATCH] Extra check in grant table code for mapping of shared frame Andres Lagar-Cavilla
-- strict thread matches above, loose matches on Subject: below --
2012-09-13 15:27 Andres Lagar-Cavilla
2012-09-17 11:00 ` Andres Lagar-Cavilla
2012-09-17 11:17 ` Keir Fraser
2012-09-17 16:29 ` Tim Deegan
2012-09-19 15:35 ` Jan Beulich
2012-09-20 15:30 ` Andres Lagar-Cavilla
2012-09-20 15:50 ` Jan Beulich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).