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

Re: [PATCH v2] x86/p2m: fix xenmem_add_to_physmap_one double page removal

  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 15 Sep 2021 11:49:22 +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=a2AgMpcRbS959oxBQSIAFBZVmflgjKd8eH6KLnV2Gxg=; b=FZecZNIZNrsBOn9rZn0Kuy6AGqxXen16n9Qth8JEaWvCAkNsQ8vNpWbdzkqnPxTOz4r9d+vmILTfsYYNAmANWX7Qzp0AJ2bfaJIcx6y3SFxRXijIiAGluQ+v9DkFAHSiPc8LIx8bbB4bFhkA2lK72ZRIBpVVM0Bplz/RYeR1jioBbAhyHBTPX8uoERQhaAt/aAsp746RUQ3L3qdcA6mBerl+S4m+mrpstcDQT/F8DqgVFeqZ8lZZk8PMSuFyse9vxd48nfzwgTIXeQz1Qp75GcETv5kiWndRaoMYi1/zfyR+UCHpsZbMtI6hfeHfJQYFrgg8f6N6I7o9EqPsvXAnAA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=PUXmSFaHrVpiQa7NM4thzhP7jpU3cWYR8y+0CRYFfY0BM5mIfD3dUnGWbJXCPMsN9VKzMj1Dyfu9tgSSYJYysZ2zHiOhPHDutGtaDCKw2M14oUqa1Fa/ZD6tktU81pcwyAheNElpK6Lf1dUCnmrTI2aRW0SESMdPS3aaLJNTGCl58Cf4ojx8wL5X5EJBPQbJ2KBzBlf5xpP5R48u7TIaXpsQbFxz4fyuJ1lD7/CSIuhTHlgZICi/huqejX5B1Rf0blo9X7bEW33adbdyc5SGpFpjPfarh0mMqUANZ+zs2TS6XX+lZsWnTen6pwDFezt9e6gH7sw5NfnBn7GS7Niv7A==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Wed, 15 Sep 2021 09:49:39 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 15.09.2021 10:44, Roger Pau Monné wrote:
> On Wed, Sep 15, 2021 at 10:16:41AM +0200, Jan Beulich wrote:
>> On 15.09.2021 10:03, Roger Pau Monne wrote:
>>> If the new gfn matches the previous one (ie: gpfn == old_gpfn)
>>> xenmem_add_to_physmap_one will issue a duplicated call to
>>> guest_physmap_remove_page with the same guest frame number, because
>>> the get_gpfn_from_mfn call has been moved by commit f8582da041 to be
>>> performed before the original page is removed. This leads to the
>>> second guest_physmap_remove_page failing, which was not the case
>>> before commit f8582da041.
>>> Add a shortcut to skip modifying the p2m if the mapping is already as
>>> requested.
>> I've meanwhile had further thoughts here - not sure in how far they
>> affect what to do (including whether to go back to v1, with the
>> description's 1st paragraph adjusted as per above):
>>> --- a/xen/arch/x86/mm/p2m.c
>>> +++ b/xen/arch/x86/mm/p2m.c
>>> @@ -2799,6 +2799,13 @@ int xenmem_add_to_physmap_one(
>>>          goto put_all;
>>>      }
>>> +    if ( gfn_eq(_gfn(old_gpfn), gpfn) )
>>> +    {
>>> +        /* Nothing to do, mapping is already as requested. */
>>> +        ASSERT(mfn_eq(prev_mfn, mfn));
>>> +        goto put_all;
>>> +    }
>> The mapping may not be "already as requested" because of p2m type or
>> p2m access. Thoughts? (At the very least the new check would likely
>> want to move after the p2m_mmio_direct one.)
> Is the type really relevant for the caller? If the mapping is already
> setup as requested, I don't think it makes sense to return -EPERM if
> the type is mmio. I'm also unsure whether we can get into that state
> in the first place.

mmio perhaps indeed can't occur (because of the earlier
get_page_from_mfn()), but in general the type might very well be
relevant: The seemingly benign change might e.g. change logdirty
to ram_rw. Whether that's for good or bad is a different matter.

> I'm unsure regarding the access, I would say changing the access type
> should be done by other means rather that replying on
> xenmem_add_to_physmap.

It _should_, yes, but there might be callers relying on it
changing as a side effect here. (There might also be callers
abusing the call for this purpose.)

> v1 was indeed more similar to the previous behavior IMO, so it's
> likely a safer approach.

Okay, I'll commit v1 with the slightly adjusted description then.




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