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

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	"George Dunlap" <George.Dunlap@eu.citrix.com>,
	"Ian Jackson" <iwj@xenproject.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>, "Julien Grall" <julien@xen.org>,
	"Paul Durrant" <paul@xen.org>,
	"Michał Leszczyński" <michal.leszczynski@cert.pl>,
	"Hubert Jasudowicz" <hubert.jasudowicz@cert.pl>,
	"Tamas K Lengyel" <tamas@tklengyel.com>
Subject: Re: [PATCH v2 07/11] xen/memory: Improve compat XENMEM_acquire_resource handling
Date: Mon, 11 Jan 2021 15:25:51 +0100	[thread overview]
Message-ID: <df809e68-4c65-39b6-47d2-9a55ed6389ff@suse.com> (raw)
In-Reply-To: <50773bc6-40a5-7c76-bd30-62bc69772503@citrix.com>

On 08.01.2021 19:57, Andrew Cooper wrote:
> 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.

Let me reply in reverse order: When asking for scope reduction, I
don't think I would typically ask to introduce multiple identical
variables across many smaller scopes. So in general I view helper
variables like induction ones okay to have wide scope, as long as
they're used only for similar purposes (e.g. not again after their
loops).

Additionally it wasn't clear to me whether this originally C++
style of declaring induction variables was something we actually
consider acceptable. Personally I've avoided using such constructs
so far, for this reason. And again personally I'd be happy to see
us formally allow for their use.

Finally, the main aspect of my previous reply was left unaddressed:
I'm not so much concerned about the extra variable, but about it
being signed when (this being used as an array index) one can do
without.

>> 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.

Sure. But the two would better agree in their interpretation of
what a count of zero means.

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

Would you mind explaining where there's a livelock? If indeed
code structure results in such, special casing count-is-zero
early on (and returning success without having done anything) is
an easy solution. Nevertheless I'd generally prefer to achieve
such behavior without additional code, e.g. by loops "naturally"
degenerating to no-ops in such a case.

Jan