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

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

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