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 <[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);
>>> +
>>> + 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
