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

[Julien Grall <julien@xxxxxxx>]

Hi Michal,

For the subject title and part of the code, "memory leak" tends to imply 
the memory is lost forever and therefore can never be recovered. AFAIU, 
in your case, the memory will be freed when the domain is destroyed. I 
would suggest to clarify it so it doesn't sound like a security issue.
What about:

"xen/arm: mm: Release the old page reference in xenmem_add_to_physmap_one()"

On 05/02/2026 12: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.
Can you explain why we skip the removal but not the insertion in this case?

> 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);
I saw Jan mentionned the fact that we have two section with the P2M lock 
taken. But isn't it in fact 3 sections as gfn_to_mfn(d, gfn) also take a 
lock?
I am not against the idea of not solving the locking right now. But I 
think it would at least be good to document it so this doesn't come as a 
surprise.
> +
> +    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;
For here and the second "return" statement below. Above callers may have 
taken a reference on the new page. So shouldn't we drop it like this is 
done at the end of the function?
> +        }
> +        else if ( !mfn_eq(mfn, mfn_old) )
> +        {
> +            /* Normal domain memory is freed, to avoid leaking memory */
Based on the thread with Jan, is this statement actually correct? Could 
we reach this call with an MMIO (or event foreign mapping). In which 
case, I am guessing we could fail? If so, is this the intended behavior 
change?
Cheers,

--
Julien Grall