Re: [PATCH 3/5] xen/memory: Fix compat XENMEM_acquire_resource for size requests

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.07.2020 21:12, Andrew Cooper wrote:
> On 29/07/2020 21:09, Jan Beulich wrote:
>> On 28.07.2020 13:37, Andrew Cooper wrote:
>>> Copy the nr_frames from the correct structure, so the caller doesn't
>>> unconditionally receive 0.
>>
>> Well, no - it does get copied from the correct structure. It's just
>> that the field doesn't get set properly up front.
> 
> You appear to be objecting to my use of the term "correct".
> 
> There are two structures.  One contains the correct value, and one
> contains the wrong value, which happens to always be 0.
> 
> I stand by sentence as currently written.

At the risk of splitting hair, what you copy from is a field holding
the correct value, but not the correct field. This only works
correctly because of the way __copy_field_{from,to}_guest() happen
to be implemented; there are possible alternative implementations
where this would break, despite ...

>> Otherwise you'll
>> (a) build in an unchecked assumption that the native and compat
>> fields match in type
> 
> Did you actually check?  Because I did before embarking on this course
> of action.
> 
> In file included from /local/xen.git/xen/include/xen/guest_access.h:10:0,
>                  from compat/memory.c:5:
> /local/xen.git/xen/include/asm/guest_access.h:152:28: error: comparison
> of distinct pointer types lacks a cast [-Werror]
>      (void)(&(hnd).p->field == _s);                      \
>                             ^
> compat/memory.c:628:22: note: in expansion of macro ‘__copy_field_to_guest’
>                  if ( __copy_field_to_guest(
>                       ^~~~~~~~~~~~~~~~~~~~~
> 
> This is what the compiler thinks of the code, when nr_frames is changed
> from uint32_t to unsigned long.

... this type safety check (which, I admit, I didn't consider when
writing my reply). I continue to think that handle and struct should
match up not just for {,__}copy_{from,to}_guest() but also for
{,__}copy_field_{from,to}_guest().

>> and (b) set a bad example for people looking
>> here
> 
> This entire function is a massive set of bad examples; the worst IMO
> being the fact that there isn't a single useful comment anywhere in it
> concerning how the higher level loop structure works.
> 
> I'm constantly annoyed that I need to reverse engineer it from scratch
> every time I look at it, despite having a better-than-most understanding
> of what it is trying to achieve, and how it is supposed to work.
> 
> I realise this is noones fault in particular, but it is not
> fair/reasonable to claim that this change is the thing setting a bad
> example in this file.

I'd be happy to see "bad examples" be corrected. As stated at various
occasions, at the time I first implemented the compat layer this seemed
like the most reasonable approach to me. If you see room for
improvement, then I'm all for it.

>> and then cloning this code in perhaps a case where (a) is not
>> even true. If you agree, the alternative change of setting
>> cmp.mar.nr_frames from nat.mar->nr_frames before the call is
> 
> Is there more to this sentence?

I guess I can't figure what you mean here.

> While this example could be implemented (at even higher overhead) by
> copying nat back to cmp before passing it back to the guest, the same is
> not true for the changes required to fix batching (which is another
> series the same size as this).

I'll see when you post this, but I think we will want the principle
outlined above to continue to hold.

Jan