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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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