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.
>
> --- 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.
XEN_DMOP_relocate_memory will corrupt itself, given the expectation that
'done' only moves forwards.
~Andrew
