Re: [PATCH] memory: overlapping XENMAPSPACE_gmfn_range requests

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

>
> --- 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