[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Ping: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()

  • To: Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Tue, 28 Sep 2021 15:36:38 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=5CZOjMZQ06ut6Cvo6a2Rn8E6/d3bWrOzmvQeZtCrL2A=; b=dznxUe6kpqU8M3WLOGHENsL/BrEuY5nn3KC+oSiXsH31jo2Ih7VSA4MylrdLLsATTIxMXvhdWgKV63bdesWTh/HRdBqPNkZHdiV0Koe7XaSHtX6fVvISROicJA0JGbRm6vHgnyd7qRa6jjEh1oaMxdFs/npXJx2w+8JeQRgBXj/ORwnJR86RSUnRxsqciZBWPy3GGxQGVh7joDl1BBZ1dinXFOdcGS/ZTeEEIoRQKylsy6tOWM73wuEHKs8YNwEN4/rYT085KxMWdpghAshKjO5ipRY+cs3tunl98sVa5K5GaJd4B+qMCD2UdfTkH16+qXHeiSfoUBKBkx5BRyEzjQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=lR5USHTjBOkoKJe8Wqz69QBzxyg+T0lKhaQCLQ/rsHK8AJyMLVIKSnDq4wbJ+/m2oWbk9kGvrxr2cwMBUYrtZFsFs2HF1E8qmXw/K6bgT7XA1QOND2SPlNhHJuzFHEMBmSL715iFt8RMnvtGpOsMZliufmNfcldsmplZyy+RYvZApVlVMDIZlr2FrmgWal6RrLiKNOvyZ7wq49SeoKBGOzcRvxSVNAPmIe1KhYxzymizhmSmR6r6YdXL+2H+uHZhoOePyN6z1MXYtGTXa75ltFTCZF1xGoEhWk81FoxcWQ+0Fs4CX/yWoIN4QKz8l3OKgw3tDScIZgTQ53f697pMEw==
  • Authentication-results: citrix.com; dkim=none (message not signed) header.d=none;citrix.com; dmarc=none action=none header.from=suse.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Tue, 28 Sep 2021 13:37:05 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 22.09.2021 12:00, Roger Pau Monné wrote:
> On Wed, Sep 22, 2021 at 11:42:30AM +0200, Jan Beulich wrote:
>> On 22.09.2021 11:26, Roger Pau Monné wrote:
>>> On Tue, Sep 21, 2021 at 12:12:05PM +0200, Jan Beulich wrote:
>>>> On 21.09.2021 10:32, Roger Pau Monné wrote:
>>>>> On Mon, Sep 20, 2021 at 05:27:17PM +0200, Jan Beulich wrote:
>>>>>> On 20.09.2021 12:20, Roger Pau Monné wrote:
>>>>>>> On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote:
>>>>>>>> --- a/xen/include/asm-arm/grant_table.h
>>>>>>>> +++ b/xen/include/asm-arm/grant_table.h
>>>>>>>> +        if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) ||        
>>>>>>>>    \
>>>>>>> I'm slightly confused by this checks, don't you need to check for
>>>>>>> gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call
>>>>>>> guest_physmap_remove_page?
>>>>>> Why? It's ogfn which gets passed to the function. And it indeed is the
>>>>>> prior GFN's mapping that we want to remove here.
>>>>>>> Or assuming that ogfn is not invalid can be used to imply a removal?
>>>>>> That implication can be (and on x86 is) used for the incoming argument,
>>>>>> i.e. "gfn". I don't think "ogfn" can serve this purpose.
>>>>> I guess I'm confused due to the ogfn checks done on the Arm side that
>>>>> are not performed on x86. So on Arm you always need to explicitly
>>>>> unhook the previous GFN before attempting to setup a new mapping,
>>>>> while on x86 you only need to do this when it's a removal in order to
>>>>> clear the entry?
>>>> The difference isn't with guest_physmap_add_entry() (both x86 and
>>>> Arm only insert a new mapping there), but with
>>>> xenmem_add_to_physmap_one(): Arm's variant doesn't care about prior
>>>> mappings. And gnttab_map_frame() gets called only from there. This
>>>> is effectively what the first paragraph of the description is about.
>>> OK, sorry, it wasn't clear to me from the description. Could you
>>> explicitly mention in the description that the removal is moved into
>>> gnttab_set_frame_gfn on Arm in order to cope with the fact that
>>> xenmem_add_to_physmap_one doesn't perform it.
>> Well, it's not really "in order to cope" - that's true for the placement
>> prior to this change as well, so not a justification for the change.
>> Nevertheless I've tried to make this more clear by changing the 1st
>> paragraph to:
>> "Without holding appropriate locks, attempting to remove a prior mapping
>>  of the underlying page is pointless, as the same (or another) mapping
>>  could be re-established by a parallel request on another vCPU. Move the
>>  code to Arm's gnttab_set_frame_gfn(); it cannot be dropped there since
>>  xenmem_add_to_physmap_one() doesn't call it either (unlike on x86). Of
>>  course this new placement doesn't improve things in any way as far as
>>  the security of grant status frame mappings goes (see XSA-379). Proper
>>  locking would be needed here to allow status frames to be mapped
>>  securely."
> Thanks, this is indeed much clearer IMO:
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Any chance of an Arm ack (or otherwise) here?

Thanks, Jan

> Albeit I still think we need to fix Arm side to do the removal in
> xenmem_add_to_physmap_one (or the x86 side to not do it).
> Thanks, Roger.



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.