Re: [PATCH] memory: overlapping XENMAPSPACE_gmfn_range requests

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.12.2025 13:46, Andrew Cooper wrote:
> On 15/12/2025 11:22 am, Jan Beulich wrote:
>> Overlapping requests may need processing backwards, or else the intended
>> effect wouldn't be achieved (and instead some pages would be moved more
>> than once).
>>
>> Also covers XEN_DMOP_relocate_memory, where the potential issue was first
>> noticed.
>>
>> Fixes: a04811a315e0 ("mm: New XENMEM space, XENMAPSPACE_gmfn_range")
>> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> Of course an alternative would be to simply reject overlapping requests.
>> Then we should reject all overlaps though, I think. But since the code
>> change didn't end up overly intrusive, I thought I would go the "fix it"
>> route first.
>>
>> In-place moves (->idx == ->gpfn) are effectively no-ops, but we don't look
>> to short-circuit them for XENMAPSPACE_gmfn, so they're not short-circuited
>> here either.
> 
> Maybe we should short-circuit them.  I can't think of anything good that
> will come of having redundant TLB/IOTLB flushing.  At the best it's a
> waste of time, and at the worst it covers up bugs.

I can do so (in a prereq change). In fact I first had the short-circuiting,
but then remembered that in (somewhat) similar situations elsewhere you
didn't like me doing such.

>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -849,7 +849,7 @@ int xenmem_add_to_physmap(struct domain
>>                            unsigned int start)
>>  {
>>      unsigned int done = 0;
>> -    long rc = 0;
>> +    long rc = 0, adjust = 1;
>>      union add_to_physmap_extra extra = {};
>>      struct page_info *pages[16];
>>  
>> @@ -884,8 +884,25 @@ int xenmem_add_to_physmap(struct domain
>>          return -EOVERFLOW;
>>      }
>>  
>> -    xatp->idx += start;
>> -    xatp->gpfn += start;
>> +    /*
>> +     * Overlapping ranges need processing backwards when destination is 
>> above
>> +     * source.
>> +     */
>> +    if ( xatp->gpfn > xatp->idx &&
>> +         unlikely(xatp->gpfn < xatp->idx + xatp->size) )
>> +    {
>> +        adjust = -1;
>> +
>> +        /* Both fields store "next item to process". */
>> +        xatp->idx += xatp->size - start - 1;
>> +        xatp->gpfn += xatp->size - start - 1;
>> +    }
>> +    else
>> +    {
>> +        xatp->idx += start;
>> +        xatp->gpfn += start;
>> +    }
> 
> These fields get written back during continuations.

I double-checked yet again, but no, I don't think so.

> XEN_DMOP_relocate_memory will corrupt itself, given the expectation that
> 'done' only moves forwards.

This, otoh, I really need to fix.

Jan