Re: [PATCH v2 07/11] xen/memory: Improve compat XENMEM_acquire_resource handling

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/09/2020 10:09, Jan Beulich wrote:
> On 22.09.2020 20:24, Andrew Cooper wrote:
>> @@ -446,6 +430,31 @@ int compat_memory_op(unsigned int cmd, 
>> XEN_GUEST_HANDLE_PARAM(void) compat)
>>  
>>  #undef XLAT_mem_acquire_resource_HNDL_frame_list
>>  
>> +            if ( xen_frame_list && cmp.mar.nr_frames )
>> +            {
>> +                /*
>> +                 * frame_list is an input for translated guests, and an 
>> output
>> +                 * for untranslated guests.  Only copy in for translated 
>> guests.
>> +                 */
>> +                if ( paging_mode_translate(currd) )
>> +                {
>> +                    compat_pfn_t *compat_frame_list = (void 
>> *)xen_frame_list;
>> +
>> +                    if ( !compat_handle_okay(cmp.mar.frame_list,
>> +                                             cmp.mar.nr_frames) ||
>> +                         __copy_from_compat_offset(
>> +                             compat_frame_list, cmp.mar.frame_list,
>> +                             0, cmp.mar.nr_frames) )
>> +                        return -EFAULT;
>> +
>> +                    /*
>> +                     * Iterate backwards over compat_frame_list[] expanding
>> +                     * compat_pfn_t to xen_pfn_t in place.
>> +                     */
>> +                    for ( int x = cmp.mar.nr_frames - 1; x >= 0; --x )
>> +                        xen_frame_list[x] = compat_frame_list[x];
> In addition to what Paul has said, I also don't see why you resort
> to a signed type here. Using the available local variable i ought to
> be quite easy:
>
>                     for ( i = cmp.mar.nr_frames; i--; )
>                         xen_frame_list[i] = compat_frame_list[i];

My goal is to make this code able to be followed, not to obfuscate it
further.  In particular, my version doesn't take several minutes to
figure out why it doesn't die with a fatal #PF.

Also (because I thought it would be full of irony to try, and it turns
out I was right), your version is 9 bytes larger once compiled.  This
has everything to do with the scope of the induction variable.  I'm
surprised that, in your effort to irradiate overly large scopes, you
haven't pushed for this form further.

> As an aside, considering the controversy we're having on patch 2, I
> find it quite interesting how you carefully allow for nr_frames being
> zero throughout your changes here (which, as I think is obvious, I
> agree you want to do).

I thought you of all people would appreciate that there *is* a
separation of responsibilities between this parameter-marshalling one,
and the native one.

Also, this code doesn't livelock in the hypervisor when handed 0.

~Andrew