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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----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).

> +    /* The defined versions are 1 and 2.  Any other value is invalid. */
>      unsigned int          gt_version;
>      /* Resource limits of the domain. */
>      unsigned int          max_grant_frames;
> @@ -222,7 +219,6 @@ nr_maptrack_frames(struct grant_table *t)
>  static grant_entry_header_t *
>  shared_entry_header(struct grant_table *t, grant_ref_t ref)
>  {
> -    ASSERT(t->gt_version != 0);

I think ASSERTs like this are probably still reasonable have. In fact we may 
want more of them now gt_version == 0 really really should be impossible.

  Paul

>      if ( t->gt_version == 1 )
>          return (grant_entry_header_t*)&shared_entry_v1(t, ref);
>      else
> @@ -1302,20 +1298,6 @@ unmap_common(
> 
>      grant_read_lock(rgt);
> 
> -    if ( rgt->gt_version == 0 )
> -    {
> -        /*
> -         * This ought to be impossible, as such a mapping should not have
> -         * been established (see the nr_grant_entries(rgt) bounds check in
> -         * gnttab_map_grant_ref()). Doing this check only in
> -         * gnttab_unmap_common_complete() - as it used to be done - would,
> -         * however, be too late.
> -         */
> -        rc = GNTST_bad_gntref;
> -        flags = 0;
> -        goto unlock_out;
> -    }
> -
>      op->rd = rd;
>      op->ref = map->ref;
> 
> @@ -1932,9 +1914,6 @@ gnttab_setup_table(
>          goto unlock;
>      }
> 
> -    if ( gt->gt_version == 0 )
> -        gt->gt_version = 1;
> -
>      if ( (op.nr_frames > nr_grant_frames(gt) ||
>            ((gt->gt_version > 1) &&
>             (grant_to_status_frames(op.nr_frames) > nr_status_frames(gt)))) &&
> @@ -2991,15 +2970,11 @@
> gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t)
> uop)
> 
>      switch ( gt->gt_version )
>      {
> -    case 0:
> -        if ( op.version == 2 )
> -        {
>      case 1:
> -            /* XXX: We could maybe shrink the active grant table here. */
> -            res = gnttab_populate_status_frames(currd, gt,
> nr_grant_frames(gt));
> -            if ( res < 0)
> -                goto out_unlock;
> -        }
> +        /* XXX: We could maybe shrink the active grant table here. */
> +        res = gnttab_populate_status_frames(currd, gt, nr_grant_frames(gt));
> +        if ( res < 0)
> +            goto out_unlock;
>          break;
>      case 2:
>          for ( i = 0; i < GNTTAB_NR_RESERVED_ENTRIES; i++ )
> @@ -3594,6 +3569,8 @@ grant_table_create(
>      percpu_rwlock_resource_init(&t->lock, grant_rwlock);
>      spin_lock_init(&t->maptrack_lock);
> 
> +    t->gt_version = 1;
> +
>      /* Okay, install the structure. */
>      t->domain = d;
>      d->grant_table = t;
> @@ -3869,9 +3846,6 @@ int gnttab_map_frame(struct domain *d, unsigned
> long idx, gfn_t gfn,
> 
>      grant_write_lock(gt);
> 
> -    if ( gt->gt_version == 0 )
> -        gt->gt_version = 1;
> -
>      if ( gt->gt_version == 2 &&
>           (idx & XENMAPIDX_grant_table_status) )
>      {
> --
> 2.1.4

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