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 <[email protected]>
>> Signed-off-by: Jan Beulich <[email protected]>
>> ---
>> 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
