Re: [PATCH v2 02/11] xen/gnttab: Rework resource acquisition

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	"George Dunlap" <George.Dunlap@eu.citrix.com>,
	"Ian Jackson" <iwj@xenproject.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>, "Julien Grall" <julien@xen.org>,
	"Paul Durrant" <paul@xen.org>,
	"Michał Leszczyński" <michal.leszczynski@cert.pl>,
	"Hubert Jasudowicz" <hubert.jasudowicz@cert.pl>,
	"Tamas K Lengyel" <tamas@tklengyel.com>
Subject: Re: [PATCH v2 02/11] xen/gnttab: Rework resource acquisition
Date: Fri, 25 Sep 2020 15:17:55 +0200	[thread overview]
Message-ID: <538236fd-af25-9e35-8f44-3125fe71e4bd@suse.com> (raw)
In-Reply-To: <20200922182444.12350-3-andrew.cooper3@citrix.com>

On 22.09.2020 20:24, Andrew Cooper wrote:
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -4013,6 +4013,81 @@ static int gnttab_get_shared_frame_mfn(struct domain *d,
>      return 0;
>  }
>  
> +int gnttab_acquire_resource(
> +    struct domain *d, unsigned int id, unsigned long frame,
> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
> +{
> +    struct grant_table *gt = d->grant_table;
> +    unsigned int i = nr_frames, tot_frames;
> +    mfn_t tmp;
> +    void **vaddrs = NULL;
> +    int rc;
> +
> +    /* Input sanity. */
> +    if ( !nr_frames )
> +        return -EINVAL;

I continue to object to this becoming an error. It wasn't before,
and it wouldn't be in line with various other operations, not the
least XENMEM_resource_ioreq_server handling, but also various of
the other mem-op functions (just to give concrete examples) or
basically all of the grant-table ones.

And if it really was to be an error, it could be had by folding
into ...

> +    /* Overflow checks */
> +    if ( frame + nr_frames < frame )
> +        return -EINVAL;

this:

    if ( frame + nr_frames <= frame )
        return -EINVAL;

> +    tot_frames = frame + nr_frames;
> +    if ( tot_frames != frame + nr_frames )
> +        return -EINVAL;
> +
> +    /* Grow table if necessary. */
> +    grant_write_lock(gt);
> +    switch ( id )
> +    {
> +    case XENMEM_resource_grant_table_id_shared:
> +        rc = gnttab_get_shared_frame_mfn(d, tot_frames - 1, &tmp);
> +        break;
> +
> +    case XENMEM_resource_grant_table_id_status:
> +        if ( gt->gt_version != 2 )
> +        {
> +    default:
> +            rc = -EINVAL;
> +            break;
> +        }
> +        rc = gnttab_get_status_frame_mfn(d, tot_frames - 1, &tmp);
> +        break;
> +    }
> +
> +    /* Any errors from growing the table? */
> +    if ( rc )
> +        goto out;
> +
> +    switch ( id )
> +    {
> +    case XENMEM_resource_grant_table_id_shared:
> +        vaddrs = gt->shared_raw;
> +        break;
> +
> +    case XENMEM_resource_grant_table_id_status:
> +        /* Check that void ** is a suitable representation for gt->status. */
> +        BUILD_BUG_ON(!__builtin_types_compatible_p(
> +                         typeof(gt->status), grant_status_t **));
> +        vaddrs = (void **)gt->status;
> +        break;
> +    }

Why the 2nd switch()? As long as you don't de-reference vaddrs
prior to checking rc, I don't see anything that could go wrong.
And once both are folded, maybe vaddr's initializer could also
be dropped again?

Jan