Re: [PATCH] xen/arm: Fix memory leak in xenmem_add_to_physmap_one

["Orzel, Michal" <michal.orzel@xxxxxxx>]

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@xxxxxxx>
>> ---
>> 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