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

[Juergen Gross <jgross@xxxxxxxx>]

On 16.02.26 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?
I finally forced myself to look into this maze.

gnttab_dma_alloc_pages() will only be called via
gntdev_ioctl_dmabuf_exp_from_refs(), which has:

        if (xen_pv_domain()) {
                pr_debug("Cannot provide dma-buf in a PV domain\n");
                return -EINVAL;
        }

at its top.

I'll setup a patch making this clear.

Are you fine with me taking your patch and cleaning it up regarding the
use of xenmem_reservation_va_mapping_update()?

> 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).
Something I'll look into at some time...


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature