Re: [Xen-devel] [PATCH] common/gnttab: Explicitly default to gnttab v1 during domain creation

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 09 August 2018 12:14
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Xen-devel <xen-
> devel@xxxxxxxxxxxxx>
> Cc: Jan Beulich <JBeulich@xxxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Julien Grall <julien.grall@xxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>; George
> Dunlap <George.Dunlap@xxxxxxxxxx>
> Subject: Re: [PATCH] common/gnttab: Explicitly default to gnttab v1 during
> domain creation
> 
> On 09/08/18 11:41, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> >> Sent: 09 August 2018 11:32
> >> To: Xen-devel <xen-devel@xxxxxxxxxxxxx>
> >> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Jan Beulich
> >> <JBeulich@xxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Stefano
> >> Stabellini <sstabellini@xxxxxxxxxx>; Julien Grall <julien.grall@xxxxxxx>;
> Wei
> >> Liu <wei.liu2@xxxxxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>;
> >> George Dunlap <George.Dunlap@xxxxxxxxxx>
> >> Subject: [PATCH] common/gnttab: Explicitly default to gnttab v1 during
> >> domain creation
> >>
> >> For reasons which appear to be exclusively down to poor review of the
> grant
> >> table v2 code, a grant table's version field was wasn't initialised during
> >> creation.
> >>
> >> A number of problems (including XSAs) have occurred in the past trying
> >> trying
> >> to use a grant table which hasn't been properly set up, and various areas
> of
> >> the code cope with v0 by defaulting to v1.
> >>
> >> In particular, the toolstack using GNTTABOP_setup_table to be able to fill
> in
> >> the store/console grants has a side effect of switching to v1.
> >>
> >> In hindsight however, this "fixup if we see 0" is a very poor, with a
> >> substantial degree of risk.  Explicitly default to grant table v1 during
> >> domain create, and let the rest of the code work safely in the knowledge
> >> that
> >> the version is sensibly set.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >> ---
> >> CC: Jan Beulich <JBeulich@xxxxxxxx>
> >> CC: Paul Durrant <paul.durrant@xxxxxxxxxx>
> >> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> >> CC: Julien Grall <julien.grall@xxxxxxx>
> >> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> >> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >> CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> >>
> >> This probably wants backporting, so I chose not to integrate it into my
> series
> >> which rearranges large chunks of DOMCTL_createdomain
> >> ---
> >>  xen/common/grant_table.c | 40 +++++++---------------------------------
> >>  1 file changed, 7 insertions(+), 33 deletions(-)
> >>
> >> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> >> index d9ec711..8bae656 100644
> >> --- a/xen/common/grant_table.c
> >> +++ b/xen/common/grant_table.c
> >> @@ -49,10 +49,7 @@ struct grant_table {
> >>      percpu_rwlock_t       lock;
> >>      /* Lock protecting the maptrack limit */
> >>      spinlock_t            maptrack_lock;
> >> -    /*
> >> -     * The defined versions are 1 and 2.  Set to 0 if we don't know
> >> -     * what version to use yet.
> >> -     */
> > Given that only the guest can set the version by hypercall, it might be
> worth comment somewhere saying that grant table always start in v1 format
> and then get morphed if the guest sets v2. I guess it might be inferred that a
> v1 table is immutable (for those who don't look too hard).
> 
> Really?  I don't think that's a reasonable inference to make, given
> GNTTABOP_set_version as a public interface.
> 

If you think it is sufficiently evident then fine, but the comment you're 
removing implies that 0 -> 1 and 0 -> 2 are valid transitions but maybe 1 -> 2 
is not (otherwise why would you have 0, right?)

  Paul

> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel