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 <[email protected]>
---
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).
--- 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;
}
