On 21/02/2026 12:57, Julien Grall wrote:
> 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()"
Reads ok.
>
> 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?
Hmm, I must have missed that the type is always fixed. In that case, we can skip
the insertion/removal.
>
>>
>> Signed-off-by: Michal Orzel <[email protected]>
>> ---
>> 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.
I'll prepare a v2 with a comment and better explanation.
>
>> +
>> + 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?
Yes, we should.
>
>> + }
>> + 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?
I wanted to leave MMIO case for a separate change but I can try to fit it all
together.
Let's discuss on v2. I'll try to prepare it taking all the remarks into account.
~Michal
