Re: [PATCH v2 6/6] gnttab: allow disabling grant table per-domain

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.09.2021 10:21, Roger Pau Monne wrote:
> Allow setting max_grant_version to 0 in order to disable grant table
> usage by a domain. This prevents allocating the grant-table structure
> inside of Xen and requires guards to be added in several functions in
> order to prevent dereferencing the structure.
> 
> Note that a domain without a grant table could still use some of the
> grant related hypercalls, it could for example issue a GNTTABOP_copy
> of a grant reference from a remote domain into a local frame.

I guess I'd consider this wrong - no grant table should imo mean no
grant operations at all. Disabling granting can be done by setting
the frame count to zero, while disabling the mapping of grants can
be done by forcing no maptrack table.

That way the number of places where checks need adding would reduce
quite a bit.

> @@ -1037,6 +1043,14 @@ map_grant_ref(
>      }
>  
>      rgt = rd->grant_table;
> +    if ( !rgt )
> +    {
> +        put_maptrack_handle(lgt, handle);
> +        rcu_unlock_domain(rd);
> +        gdprintk(XENLOG_INFO, "%pd has no grant table\n", rd);
> +        op->status = GNTST_bad_domain;
> +        return;

I would pull this check earlier, to simplify error cleanup. It
could live right after having established rd.

> @@ -1367,6 +1381,13 @@ unmap_common(
>      ld = current->domain;
>      lgt = ld->grant_table;
>  
> +    if ( !lgt )
> +    {
> +        gdprintk(XENLOG_INFO, "%pd has no grant table\n", ld);
> +        op->status = GNTST_bad_domain;
> +        return;
> +    }

While this is necessary, ...

> @@ -1406,6 +1427,13 @@ unmap_common(
>      TRACE_1D(TRC_MEM_PAGE_GRANT_UNMAP, dom);
>  
>      rgt = rd->grant_table;
> +    if ( !rgt )
> +    {
> +        rcu_unlock_domain(rd);
> +        gdprintk(XENLOG_INFO, "%pd has no grant table\n", rd);
> +        op->status = GNTST_bad_domain;
> +        return;
> +    }

.. this looks to simply be a bug check, i.e. may want to be BUG_ON().
There's can't be anything to unmap if the mapping of a grant of that
domain can't have succeeded.

> @@ -1556,6 +1584,12 @@ unmap_common_complete(struct gnttab_unmap_common *op)
>  
>      rcu_lock_domain(rd);
>      rgt = rd->grant_table;
> +    if ( !rgt )
> +    {
> +        rcu_unlock_domain(rd);
> +        op->status = GNTST_bad_domain;
> +        return;
> +    }

Same here, I think.

> @@ -2138,6 +2174,11 @@ gnttab_query_size(
>      }
>  
>      gt = d->grant_table;
> +    if ( !gt )
> +    {
> +        op.status = GNTST_bad_domain;
> +        goto out;
> +    }

I'm not sure here - I could also see this report zero (and success).

> @@ -3270,6 +3327,11 @@ 
> gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_status_frames_t) 
> uop,
>      }
>  
>      gt = d->grant_table;
> +    if ( !gt )
> +    {
> +        op.status = GNTST_bad_domain;
> +        goto out2;
> +    }

While not simplifying error cleanup here, I think this might still
benefit from getting moved ahead of the XSM hook. There's no point
querying XSM in this case.

Jan