| Message ID | 20210915080342.21346-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] x86/p2m: fix xenmem_add_to_physmap_one double page removal | expand |
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.) I've also meanwhile realized that it was a different form of short circuiting that I had been thinking about before: XENMAPSPACE_gmfn's idx == gpfn. Jan
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. 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. v1 was indeed more similar to the previous behavior IMO, so it's likely a safer approach. Thanks, Roger.
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. Jan
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 674a6f4fe9..bcdc5c7014 100644 --- 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; + } + /* Remove previously mapped page if it was present. */ if ( p2mt == p2m_mmio_direct ) rc = -EPERM;
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. Fixes: f8582da041 ('x86/mm: pull a sanity check earlier in xenmem_add_to_physmap_one()') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - Short circuit and skip modifying the p2m. --- xen/arch/x86/mm/p2m.c | 7 +++++++ 1 file changed, 7 insertions(+)