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
