Extra check in grant table code for mapping of shared frame
PATCH 1/1 Extra check in grant table code for mapping of shared frame
xen/common/grant_table.c | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-)
xen/common/grant_table.c +6 -3
@@ -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 )
ping…
Thanks,
Andres
Quoted 25 lines · Andres Lagar-Cavilla
xen/common/grant_table.c| 9 ++++++---1 files changed, 6 insertions(+), 3 deletions(-)Signed-off-by Andres Lagar-Cavilla <[email protected]>diff -r
5ce5b53ea68f-r40b91bed1275xen/common/grant_table.cxen/common/grant_table.c +6
-3@@ -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 )
Probably needs Tim to comment on it. Assuming he's any wiser about this code than the rest of us. ;)
-- Keir
Quoted 31 lines · Andres Lagar-Cavilla
pingŠ
Thanks,
AndresQuoted 25 lines · Andres Lagar-Cavilla
xen/common/grant_table.c| 9 ++++++---1 files changed, 6 insertions(+), 3 deletions(-)Signed-off-by Andres Lagar-Cavilla <[email protected]>diff -r
5ce5b53ea68f-r40b91bed1275xen/common/grant_table.cxen/common/grant_table.c +6
-3@@ -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 )
Quoted 2 lines · Keir Fraser
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.
Quoted 35 lines · Andres Lagar-Cavilla
Quoted 32 lines
pingŠ
Thanks,
AndresQuoted 25 lines · Andres Lagar-Cavilla
xen/common/grant_table.c| 9 ++++++---1 files changed, 6 insertions(+), 3 deletions(-)Signed-off-by Andres Lagar-Cavilla <[email protected]>diff -r
5ce5b53ea68f-r40b91bed1275xen/common/grant_table.cxen/common/grant_table.c +6
-3@@ -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 )
Quoted 9 lines
Quoted 1 line
Quoted 1 line · Andres Lagar-Cavilla
xen/common/grant_table.c +0
-2@@ -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) )
xen/common/grant_table.c __gnttab_map_grant_ref( · line 660
Quoted 10 lines
xen/common/grant_table.c +6
-1- 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
Quoted 17 lines · Jan Beulich
Quoted 17 lines
Quoted 1 line
Quoted 1 line · Andres Lagar-Cavilla
xen/common/grant_table.c +6
-3@@ -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; + }
Quoted 7 lines
xen/common/grant_table.c __gnttab_map_grant_ref( · line 660
Quoted 2 lines
xen/common/grant_table.c +0
-0nr_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
Quoted 3 lines
Jan
Quoted 41 lines
Quoted 1 line
Quoted 1 line · Andres Lagar-Cavilla
Quoted 17 lines · Jan Beulich
Quoted 17 lines
Quoted 1 line
Quoted 1 line · Andres Lagar-Cavilla
xen/common/grant_table.c +6
-3@@ -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; + }Quoted 7 lines
xen/common/grant_table.c __gnttab_map_grant_ref( · line 660
Quoted 2 lines
xen/common/grant_table.c +0
-0nr_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_pinwill cascade intoundo_out, andnr_getsremains 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); ...
Quoted 1 line
i.e.
put_page_typewill 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
[email protected]
http://lists.xen.org/xen-devel