Re: [PATCH v3 7/7] xen/memory: Fix mapping grant tables with XENMEM_acquire_resource

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/01/2021 16:12, Jan Beulich wrote:
> On 12.01.2021 20:48, Andrew Cooper wrote:
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -4628,7 +4628,6 @@ int arch_acquire_resource(struct domain *d, unsigned 
>> int type,
>>          if ( id != (unsigned int)ioservid )
>>              break;
>>  
>> -        rc = 0;
>>          for ( i = 0; i < nr_frames; i++ )
>>          {
>>              mfn_t mfn;
> How "good" are our chances that older gcc won't recognize that
> without this initialization ...
>
>> @@ -4639,6 +4638,9 @@ int arch_acquire_resource(struct domain *d, unsigned 
>> int type,
>>  
>>              mfn_list[i] = mfn_x(mfn);
>>          }
>> +        if ( i == nr_frames )
>> +            /* Success.  Passed nr_frames back to the caller. */
>> +            rc = nr_frames;
> ... rc will nevertheless not remain uninitialized when nr_frames
> is zero?

I don't anticipate this function getting complicated enough for us to
need to rely on tricks like that to spot bugs.

AFAICT, it would take a rather larger diffstat to make it "uninitialised
clean" again.

>> @@ -432,6 +419,28 @@ int compat_memory_op(unsigned int cmd, 
>> XEN_GUEST_HANDLE_PARAM(void) compat)
>>  
>>              if ( xen_frame_list && cmp.mar.nr_frames )
>>              {
>> +                unsigned int xlat_max_frames =
>> +                    (COMPAT_ARG_XLAT_SIZE - sizeof(*nat.mar)) /
>> +                    sizeof(*xen_frame_list);
>> +
>> +                if ( start_extent >= nat.mar->nr_frames )
> Comparing with the enclosing if() I find it slightly confusing
> that different instances of nr_frames get used. Perhaps best to
> adjust the earlier patch to also use nat.mar->nr_frames? Or is
> this perhaps on purpose?

Good point - this is before the shuffle to avoid double accounting, and
the code gen will be better by using cmp consistently.

~Andrew