Re: [PATCH v8 06/16] xen/memory: Fix mapping grant tables with XENMEM_acquire_resource

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>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH v8 06/16] xen/memory: Fix mapping grant tables with XENMEM_acquire_resource
Date: Mon, 1 Feb 2021 14:03:44 +0100	[thread overview]
Message-ID: <28e8d116-9d5f-3c73-b366-63d5b047b085@suse.com> (raw)
In-Reply-To: <53a26fb9-9c43-d1c4-90cd-bb29d57e106b@citrix.com>

On 01.02.2021 12:11, Andrew Cooper wrote:
> On 01/02/2021 10:10, Roger Pau Monné wrote:
>> On Sat, Jan 30, 2021 at 02:58:42AM +0000, Andrew Cooper wrote:
>>> @@ -636,15 +662,45 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
>>>                      compat_frame_list[i] = frame;
>>>                  }
>>>  
>>> -                if ( __copy_to_compat_offset(cmp.mar.frame_list, 0,
>>> +                if ( __copy_to_compat_offset(cmp.mar.frame_list, start_extent,
>>>                                               compat_frame_list,
>>> -                                             cmp.mar.nr_frames) )
>>> +                                             done) )
>>>                      return -EFAULT;
>> Is it fine to return with a possibly pending continuation already
>> encoded here?
>>
>> Other places seem to crash the domain when that happens.
> 
> Hmm.  This is all a total mess.  (Elsewhere the error handling is also
> broken - a caller who receives an error can't figure out how to recover)
> 
> But yes - I think you're right - the only thing we can do here is `goto
> crash;` and woe betide any 32bit kernel which passes a pointer to a
> read-only buffer.

I'd like to ask you to reconsider the "goto crash", both the one
you mention above and the other one already present in the patch.
Wiring all the cases where we mean to crash the guest into a
single domain_crash() invocation has the downside that when
observing such a case one can't remotely know which path has led
there. Therefore I'd like to suggest individual domain_crash()
invocations on every affected path. Elsewhere in the file there
already is such an instance, commented "Cannot cancel the
continuation...".

Jan