Ping: [PATCH] Xen/gnttab: improve error handling in gnttab_dma_{alloc,free}_pages()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.02.2026 09:55, Jan Beulich wrote:
> Both decrease-reservation and populate-physmap can succeed partially. For
> the former this may be pretty unlikely, but clearly the latter can hit
> both out-of-memory conditions in the hypervisor or denial because of the
> allocation exceeding the domain's allowance (there's no interaction with
> the balloon driver here either).
> 
> In gnttab_dma_free_pages() we simply can't give back to the system what
> hasn't been re-filled with backing memory. For gnttab_dma_alloc_pages()
> both parts of the overall region need dealing with differently.
> 
> While no present caller of gnttab_dma_free_pages() checks its return
> value, also don't use -EFAULT there: It really is an out-of-memory
> condition. (In gnttab_dma_alloc_pages() -EFAULT doesn't look quite right
> either, but I couldn't think of a clearly better error code there.)
> 
> Fixes: 9bdc7304f536 ("xen/grant-table: Allow allocating buffers suitable for 
> DMA")
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> I'm likely screwing things up, as I can't understand how this is intended
> to work: args->dev_bus_addr shouldn't really be in pseudo-physical address
> space, or else the address isn't suitable for handing to a device in order
> to DMA to/from it. Hence using __phys_to_pfn() on it to then hand the
> result to pfn_to_page() feels bogus. Was all of this perhaps only ever
> intended for (tested with) translated domains? With the uses of
> xenmem_reservation_va_mapping_*() only being masquerade?
> 
> Furthermore the allocated buffer ought to have been contiguous in
> machine / DMA space. Yet the way it's re-populated upon freeing of the
> area doesn't guarantee that at all.
> 
> All of what is done assumes dma_free_{coherent,wc,attr}() is capable of
> freeing piecemeal. I only checked dma_release_from_dev_coherent() to
> fulfill this requirement. If this can't be relied upon, please consider
> this submission as merely a bug report (for someone else to fix).

Even in case this patch is complete rubbish, something wants doing here. May
I therefore ask for feedback (perhaps in the form of a completely different
patch)?

Jan

> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -1095,6 +1095,28 @@ int gnttab_dma_alloc_pages(struct gnttab
>       ret = xenmem_reservation_decrease(args->nr_pages, args->frames);
>       if (ret != args->nr_pages) {
>               pr_debug("Failed to decrease reservation for DMA buffer\n");
> +             if (ret >= 0) {
> +                     /* Free the part where decrease didn't work. */
> +                     size_t done = ret << PAGE_SHIFT;
> +
> +                     xenmem_reservation_va_mapping_update(args->nr_pages - 
> ret,
> +                                                          &args->pages[ret],
> +                                                          
> &args->frames[ret]);
> +
> +                     if (args->coherent)
> +                             dma_free_coherent(args->dev, size - done,
> +                                               args->vaddr + done,
> +                                               args->dev_bus_addr + done);
> +                     else
> +                             dma_free_wc(args->dev, size - done,
> +                                         args->vaddr + done,
> +                                         args->dev_bus_addr + done);
> +
> +                     if (!ret) /* Nothing else to do? */
> +                             return -EFAULT;
> +
> +                     args->nr_pages = ret;
> +             }
>               ret = -EFAULT;
>               goto fail;
>       }
> @@ -1128,7 +1150,12 @@ int gnttab_dma_free_pages(struct gnttab_
>       ret = xenmem_reservation_increase(args->nr_pages, args->frames);
>       if (ret != args->nr_pages) {
>               pr_debug("Failed to increase reservation for DMA buffer\n");
> -             ret = -EFAULT;
> +             /* Can only free what has been re-filled. */
> +             if (!ret)
> +                     return -ENOMEM;
> +             if (ret > 0)
> +                     args->nr_pages = ret;
> +             ret = -ENOMEM;
>       } else {
>               ret = 0;
>       }