Re: [PATCH v6 01/11] memory: batch processing in acquire_resource()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.07.2020 21:39, Michał Leszczyński wrote:
> From: Michal Leszczynski <michal.leszczynski@xxxxxxx>
> 
> Allow to acquire large resources by allowing acquire_resource()
> to process items in batches, using hypercall continuation.
> 
> Be aware that this modifies the behavior of acquire_resource
> call with frame_list=NULL. While previously it would return
> the size of internal array (32), with this patch it returns
> the maximal quantity of frames that could be requested at once,
> i.e. UINT_MAX >> MEMOP_EXTENT_SHIFT.

This isn't really a behavioral change, and hence I'd prefer this
to be re-worded: It was and is the upper bound on request sizes
that gets reported here. It's just that this upper bound now
changes.

> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1046,10 +1046,12 @@ static int acquire_grant_table(struct domain *d, 
> unsigned int id,
>  }
>  
>  static int acquire_resource(
> -    XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
> +    XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg,
> +    unsigned long *start_extent)
>  {
>      struct domain *d, *currd = current->domain;
>      xen_mem_acquire_resource_t xmar;
> +    uint32_t total_frames;

Please don't use fixed width types when plain C types will do
(unsigned int here).

> @@ -1069,7 +1071,7 @@ static int acquire_resource(
>          if ( xmar.nr_frames )
>              return -EINVAL;
>  
> -        xmar.nr_frames = ARRAY_SIZE(mfn_list);
> +        xmar.nr_frames = UINT_MAX >> MEMOP_EXTENT_SHIFT;
>  
>          if ( __copy_field_to_guest(arg, &xmar, nr_frames) )
>              return -EFAULT;
> @@ -1077,8 +1079,28 @@ static int acquire_resource(
>          return 0;
>      }
>  
> +    total_frames = xmar.nr_frames;
> +
> +    /* Is the size too large for us to encode a continuation? */
> +    if ( unlikely(xmar.nr_frames > (UINT_MAX >> MEMOP_EXTENT_SHIFT)) )
> +        return -EINVAL;
> +
> +    if ( *start_extent )
> +    {
> +        /*
> +         * Check whether start_extent is in bounds, as this
> +         * value if visible to the calling domain.
> +         */
> +        if ( *start_extent > xmar.nr_frames )
> +            return -EINVAL;
> +
> +        xmar.frame += *start_extent;
> +        xmar.nr_frames -= *start_extent;
> +        guest_handle_add_offset(xmar.frame_list, *start_extent);
> +    }

May I ask that you drop the if() around this block of code?

Also, looking at this, I wonder whether it's a good idea to use the
"start extent" model here anyway: You could as well update the
struct (saying that it may be clobbered in the public header) and
copy the whole thing back to the original guest struct. This would
then remove the pretty arbitrary "UINT_MAX >> MEMOP_EXTENT_SHIFT"
limit you currently need to enforce. The main question is whether
we'd consider such an adjustment to an existing interface
acceptable; there's an at least theoretical risk that it may break
existing callers. Then again no existing caller can sensibly have
specified a count above 32, and when the copying back would be
limited to just the continuation case, no such caller would be
affected in any way afaict.

Jan