When a guest maps pages via XENMEM_add_to_physmap to a GFN that already
has an existing mapping, the old page at that GFN was not being removed,
causing a memory leak. This affects all mapping spaces including
XENMAPSPACE_shared_info, XENMAPSPACE_grant_table, and
XENMAPSPACE_gmfn_foreign. The memory would be reclaimed on domain
destruction.
Add logic to remove the previously mapped page before creating the new
mapping, matching the x86 implementation approach.
Additionally, skip removal if the same MFN is being remapped.
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
I'm not sure where to point the Fixes tag to.
---
xen/arch/arm/mm.c | 32 +++++++++++++++++++++++++++++---
1 file changed, 29 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 6df8b616e464..b9f1a493dcd7 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -166,10 +166,11 @@ int xenmem_add_to_physmap_one(
unsigned long idx,
gfn_t gfn)
{
- mfn_t mfn = INVALID_MFN;
+ mfn_t mfn = INVALID_MFN, mfn_old;
int rc;
p2m_type_t t;
struct page_info *page = NULL;
+ struct p2m_domain *p2m = p2m_get_hostp2m(d);
switch ( space )
{
@@ -244,6 +245,33 @@ int xenmem_add_to_physmap_one(
return -ENOSYS;
}
+ /*
+ * Remove previously mapped page if it was present, to avoid leaking
+ * memory.
+ */
+ mfn_old = gfn_to_mfn(d, gfn);
+
+ if ( mfn_valid(mfn_old) )
+ {
+ if ( is_special_page(mfn_to_page(mfn_old)) )
+ {
+ /* Just unmap, don't free */
+ p2m_write_lock(p2m);
+ rc = p2m_set_entry(p2m, gfn, 1, INVALID_MFN,
+ p2m_invalid, p2m->default_access);
+ p2m_write_unlock(p2m);
+ if ( rc )
+ return rc;
+ }
+ else if ( !mfn_eq(mfn, mfn_old) )
+ {
+ /* Normal domain memory is freed, to avoid leaking memory */
+ rc = guest_remove_page(d, gfn_x(gfn));
+ if ( rc )
+ return rc;
+ }
+ }
+
/*
* Map at new location. Here we need to map xenheap RAM page differently
* because we need to store the valid GFN and make sure that nothing was
@@ -255,8 +283,6 @@ int xenmem_add_to_physmap_one(
rc = guest_physmap_add_entry(d, gfn, mfn, 0, t);
else
{
- struct p2m_domain *p2m = p2m_get_hostp2m(d);
-
p2m_write_lock(p2m);
if ( gfn_eq(page_get_xenheap_gfn(mfn_to_page(mfn)), INVALID_GFN) )
{
--
2.43.0On 05.02.2026 13:58, Michal Orzel wrote:
> When a guest maps pages via XENMEM_add_to_physmap to a GFN that already
> has an existing mapping, the old page at that GFN was not being removed,
> causing a memory leak. This affects all mapping spaces including
> XENMAPSPACE_shared_info, XENMAPSPACE_grant_table, and
> XENMAPSPACE_gmfn_foreign. The memory would be reclaimed on domain
> destruction.
>
> Add logic to remove the previously mapped page before creating the new
> mapping, matching the x86 implementation approach.
>
> Additionally, skip removal if the same MFN is being remapped.
>
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
> I'm not sure where to point the Fixes tag to.
> ---
> xen/arch/arm/mm.c | 32 +++++++++++++++++++++++++++++---
> 1 file changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 6df8b616e464..b9f1a493dcd7 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -166,10 +166,11 @@ int xenmem_add_to_physmap_one(
> unsigned long idx,
> gfn_t gfn)
> {
> - mfn_t mfn = INVALID_MFN;
> + mfn_t mfn = INVALID_MFN, mfn_old;
> int rc;
> p2m_type_t t;
> struct page_info *page = NULL;
> + struct p2m_domain *p2m = p2m_get_hostp2m(d);
>
> switch ( space )
> {
> @@ -244,6 +245,33 @@ int xenmem_add_to_physmap_one(
> return -ENOSYS;
> }
>
> + /*
> + * Remove previously mapped page if it was present, to avoid leaking
> + * memory.
> + */
> + mfn_old = gfn_to_mfn(d, gfn);
> +
> + if ( mfn_valid(mfn_old) )
> + {
> + if ( is_special_page(mfn_to_page(mfn_old)) )
> + {
> + /* Just unmap, don't free */
> + p2m_write_lock(p2m);
> + rc = p2m_set_entry(p2m, gfn, 1, INVALID_MFN,
> + p2m_invalid, p2m->default_access);
> + p2m_write_unlock(p2m);
> + if ( rc )
> + return rc;
> + }
> + else if ( !mfn_eq(mfn, mfn_old) )
> + {
> + /* Normal domain memory is freed, to avoid leaking memory */
> + rc = guest_remove_page(d, gfn_x(gfn));
> + if ( rc )
> + return rc;
> + }
> + }
This new code and what follows below it are not inside a single locked region,
hence the cleanup done above may well have been "undone" again by the time the
P2M lock is acquired below. That locking may not be apparent in the x86
variant when merely looking at functions used. There's a large comment,
though, explaining how we actually (ab)use the simplified locking model
(compared to what was once intended, but never carried out).
Further, doesn't P2M entry type influence what needs doing here, including
possibly rejecting the request? x86 refuses to replace p2m_mmio_direct entries
by this means, but seeing that Arm has XENMAPSPACE_dev_mmio, this case may
need handling, but the handling may need to be different from what you do
above. (Just to mention: Presumably on Arm it's no different from x86: An MMIO
page may or may not pass an mfn_valid() check.)
Jan
On 05/02/2026 14:49, Jan Beulich wrote:
> On 05.02.2026 13:58, Michal Orzel wrote:
>> When a guest maps pages via XENMEM_add_to_physmap to a GFN that already
>> has an existing mapping, the old page at that GFN was not being removed,
>> causing a memory leak. This affects all mapping spaces including
>> XENMAPSPACE_shared_info, XENMAPSPACE_grant_table, and
>> XENMAPSPACE_gmfn_foreign. The memory would be reclaimed on domain
>> destruction.
>>
>> Add logic to remove the previously mapped page before creating the new
>> mapping, matching the x86 implementation approach.
>>
>> Additionally, skip removal if the same MFN is being remapped.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> ---
>> I'm not sure where to point the Fixes tag to.
>> ---
>> xen/arch/arm/mm.c | 32 +++++++++++++++++++++++++++++---
>> 1 file changed, 29 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 6df8b616e464..b9f1a493dcd7 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -166,10 +166,11 @@ int xenmem_add_to_physmap_one(
>> unsigned long idx,
>> gfn_t gfn)
>> {
>> - mfn_t mfn = INVALID_MFN;
>> + mfn_t mfn = INVALID_MFN, mfn_old;
>> int rc;
>> p2m_type_t t;
>> struct page_info *page = NULL;
>> + struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>
>> switch ( space )
>> {
>> @@ -244,6 +245,33 @@ int xenmem_add_to_physmap_one(
>> return -ENOSYS;
>> }
>>
>> + /*
>> + * Remove previously mapped page if it was present, to avoid leaking
>> + * memory.
>> + */
>> + mfn_old = gfn_to_mfn(d, gfn);
>> +
>> + if ( mfn_valid(mfn_old) )
>> + {
>> + if ( is_special_page(mfn_to_page(mfn_old)) )
>> + {
>> + /* Just unmap, don't free */
>> + p2m_write_lock(p2m);
>> + rc = p2m_set_entry(p2m, gfn, 1, INVALID_MFN,
>> + p2m_invalid, p2m->default_access);
>> + p2m_write_unlock(p2m);
>> + if ( rc )
>> + return rc;
>> + }
>> + else if ( !mfn_eq(mfn, mfn_old) )
>> + {
>> + /* Normal domain memory is freed, to avoid leaking memory */
>> + rc = guest_remove_page(d, gfn_x(gfn));
>> + if ( rc )
>> + return rc;
>> + }
>> + }
>
> This new code and what follows below it are not inside a single locked region,
> hence the cleanup done above may well have been "undone" again by the time the
> P2M lock is acquired below. That locking may not be apparent in the x86
> variant when merely looking at functions used. There's a large comment,
> though, explaining how we actually (ab)use the simplified locking model
> (compared to what was once intended, but never carried out).
Do you suggest to put the new code and old code in a single locked region?
>
> Further, doesn't P2M entry type influence what needs doing here, including
> possibly rejecting the request? x86 refuses to replace p2m_mmio_direct entries
> by this means, but seeing that Arm has XENMAPSPACE_dev_mmio, this case may
> need handling, but the handling may need to be different from what you do
> above. (Just to mention: Presumably on Arm it's no different from x86: An MMIO
> page may or may not pass an mfn_valid() check.)
I actually had the following in my initial implementation:
p2m_write_lock(p2m);
mfn_old = p2m_get_entry(p2m, gfn, &p2mt_old, NULL, NULL, NULL);
if ( p2mt_old == p2m_mmio_direct )
{
p2m_write_unlock(p2m);
return -EPERM;
}
but realized this is actually a different issue than the one I want to solve and
I don't want to fix two in one go.
~Michal
On 05.02.2026 16:08, Orzel, Michal wrote:
>
>
> On 05/02/2026 14:49, Jan Beulich wrote:
>> On 05.02.2026 13:58, Michal Orzel wrote:
>>> When a guest maps pages via XENMEM_add_to_physmap to a GFN that already
>>> has an existing mapping, the old page at that GFN was not being removed,
>>> causing a memory leak. This affects all mapping spaces including
>>> XENMAPSPACE_shared_info, XENMAPSPACE_grant_table, and
>>> XENMAPSPACE_gmfn_foreign. The memory would be reclaimed on domain
>>> destruction.
>>>
>>> Add logic to remove the previously mapped page before creating the new
>>> mapping, matching the x86 implementation approach.
>>>
>>> Additionally, skip removal if the same MFN is being remapped.
>>>
>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>> ---
>>> I'm not sure where to point the Fixes tag to.
>>> ---
>>> xen/arch/arm/mm.c | 32 +++++++++++++++++++++++++++++---
>>> 1 file changed, 29 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>>> index 6df8b616e464..b9f1a493dcd7 100644
>>> --- a/xen/arch/arm/mm.c
>>> +++ b/xen/arch/arm/mm.c
>>> @@ -166,10 +166,11 @@ int xenmem_add_to_physmap_one(
>>> unsigned long idx,
>>> gfn_t gfn)
>>> {
>>> - mfn_t mfn = INVALID_MFN;
>>> + mfn_t mfn = INVALID_MFN, mfn_old;
>>> int rc;
>>> p2m_type_t t;
>>> struct page_info *page = NULL;
>>> + struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>>
>>> switch ( space )
>>> {
>>> @@ -244,6 +245,33 @@ int xenmem_add_to_physmap_one(
>>> return -ENOSYS;
>>> }
>>>
>>> + /*
>>> + * Remove previously mapped page if it was present, to avoid leaking
>>> + * memory.
>>> + */
>>> + mfn_old = gfn_to_mfn(d, gfn);
>>> +
>>> + if ( mfn_valid(mfn_old) )
>>> + {
>>> + if ( is_special_page(mfn_to_page(mfn_old)) )
>>> + {
>>> + /* Just unmap, don't free */
>>> + p2m_write_lock(p2m);
>>> + rc = p2m_set_entry(p2m, gfn, 1, INVALID_MFN,
>>> + p2m_invalid, p2m->default_access);
>>> + p2m_write_unlock(p2m);
>>> + if ( rc )
>>> + return rc;
>>> + }
>>> + else if ( !mfn_eq(mfn, mfn_old) )
>>> + {
>>> + /* Normal domain memory is freed, to avoid leaking memory */
>>> + rc = guest_remove_page(d, gfn_x(gfn));
>>> + if ( rc )
>>> + return rc;
>>> + }
>>> + }
>>
>> This new code and what follows below it are not inside a single locked region,
>> hence the cleanup done above may well have been "undone" again by the time the
>> P2M lock is acquired below. That locking may not be apparent in the x86
>> variant when merely looking at functions used. There's a large comment,
>> though, explaining how we actually (ab)use the simplified locking model
>> (compared to what was once intended, but never carried out).
> Do you suggest to put the new code and old code in a single locked region?
Yes. Which may be difficult on Arm, where the P2M lock isn't recursive.
>> Further, doesn't P2M entry type influence what needs doing here, including
>> possibly rejecting the request? x86 refuses to replace p2m_mmio_direct entries
>> by this means, but seeing that Arm has XENMAPSPACE_dev_mmio, this case may
>> need handling, but the handling may need to be different from what you do
>> above. (Just to mention: Presumably on Arm it's no different from x86: An MMIO
>> page may or may not pass an mfn_valid() check.)
> I actually had the following in my initial implementation:
> p2m_write_lock(p2m);
> mfn_old = p2m_get_entry(p2m, gfn, &p2mt_old, NULL, NULL, NULL);
> if ( p2mt_old == p2m_mmio_direct )
> {
> p2m_write_unlock(p2m);
> return -EPERM;
> }
> but realized this is actually a different issue than the one I want to solve and
> I don't want to fix two in one go.
Hmm. If you indeed want to separate both (and also not have both in a
series), I'd then suggest to at least mention that this aspect (and
whatever else there may be) is deliberately left out.
Jan
© 2016 - 2026 Red Hat, Inc.