| Message ID | 20190423105457.17502-2-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/swiotlb: fix an issue and improve swiotlb-xen | expand |
On 4/23/19 6:54 AM, Juergen Gross wrote: > The condition in xen_swiotlb_free_coherent() for deciding whether to > call xen_destroy_contiguous_region() is wrong: in case the region to > be freed is not contiguous calling xen_destroy_contiguous_region() is > the wrong thing to do: it would result in inconsistent mappings of > multiple PFNs to the same MFN. This will lead to various strange > crashes or data corruption. > > Instead of calling xen_destroy_contiguous_region() in that case a > warning should be issued as that situation should never occur. > > Cc: stable@vger.kernel.org > Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> On 23.04.19 at 12:54, <jgross@suse.com> wrote: > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -360,8 +360,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, > /* Convert the size to actually allocated. */ > size = 1UL << (order + XEN_PAGE_SHIFT); > > - if (((dev_addr + size - 1 <= dma_mask)) || > - range_straddles_page_boundary(phys, size)) > + if ((dev_addr + size - 1 <= dma_mask) && > + !WARN_ON(range_straddles_page_boundary(phys, size))) > xen_destroy_contiguous_region(phys, order); On the allocation side we have if (((dev_addr + size - 1 <= dma_mask)) && !range_straddles_page_boundary(phys, size)) *dma_handle = dev_addr; else { if (xen_create_contiguous_region(phys, order, fls64(dma_mask), dma_handle) != 0) { xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs); return NULL; } } which is (as far as the function call is concerned) if ((dev_addr + size - 1 > dma_mask) || range_straddles_page_boundary(phys, size)) xen_create_contiguous_region(...); So I don't think your transformation is correct. Even worse, both parts of the condition in xen_swiotlb_free_coherent() act on an address that is the _result_ of the prior xen_create_contiguous_region(), i.e. the address should always match _both_ criteria anyway. Whereas what you really want is undo the xen_create_contiguous_region() only when it actually was called. Otherwise you also shatter contiguous allocations that were contiguous already for other reasons (perhaps just luck). Jan
On 25/04/2019 10:53, Jan Beulich wrote: >>>> On 23.04.19 at 12:54, <jgross@suse.com> wrote: >> --- a/drivers/xen/swiotlb-xen.c >> +++ b/drivers/xen/swiotlb-xen.c >> @@ -360,8 +360,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, >> /* Convert the size to actually allocated. */ >> size = 1UL << (order + XEN_PAGE_SHIFT); >> >> - if (((dev_addr + size - 1 <= dma_mask)) || >> - range_straddles_page_boundary(phys, size)) >> + if ((dev_addr + size - 1 <= dma_mask) && >> + !WARN_ON(range_straddles_page_boundary(phys, size))) >> xen_destroy_contiguous_region(phys, order); > > On the allocation side we have > > if (((dev_addr + size - 1 <= dma_mask)) && > !range_straddles_page_boundary(phys, size)) > *dma_handle = dev_addr; > else { > if (xen_create_contiguous_region(phys, order, > fls64(dma_mask), dma_handle) != 0) { > xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs); > return NULL; > } > } > > which is (as far as the function call is concerned) > > if ((dev_addr + size - 1 > dma_mask) || > range_straddles_page_boundary(phys, size)) > xen_create_contiguous_region(...); > > So I don't think your transformation is correct. Even worse, both > parts of the condition in xen_swiotlb_free_coherent() act on an > address that is the _result_ of the prior > xen_create_contiguous_region(), i.e. the address should always > match _both_ criteria anyway. Whereas what you really want is > undo the xen_create_contiguous_region() only when it actually > was called. Otherwise you also shatter contiguous allocations > that were contiguous already for other reasons (perhaps just > luck). Yes, that is what patch 3 does. Juergen
On 25/04/2019 10:53, Jan Beulich wrote: >>>> On 23.04.19 at 12:54, <jgross@suse.com> wrote: >> --- a/drivers/xen/swiotlb-xen.c >> +++ b/drivers/xen/swiotlb-xen.c >> @@ -360,8 +360,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, >> /* Convert the size to actually allocated. */ >> size = 1UL << (order + XEN_PAGE_SHIFT); >> >> - if (((dev_addr + size - 1 <= dma_mask)) || >> - range_straddles_page_boundary(phys, size)) >> + if ((dev_addr + size - 1 <= dma_mask) && >> + !WARN_ON(range_straddles_page_boundary(phys, size))) >> xen_destroy_contiguous_region(phys, order); > > On the allocation side we have > > if (((dev_addr + size - 1 <= dma_mask)) && > !range_straddles_page_boundary(phys, size)) > *dma_handle = dev_addr; > else { > if (xen_create_contiguous_region(phys, order, > fls64(dma_mask), dma_handle) != 0) { > xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs); > return NULL; > } > } > > which is (as far as the function call is concerned) > > if ((dev_addr + size - 1 > dma_mask) || > range_straddles_page_boundary(phys, size)) > xen_create_contiguous_region(...); > > So I don't think your transformation is correct. I know that I modified the condition. Calling xen_destroy_contiguous_region() for a non-contiguous region is destructive, so we have to avoid it in any case. I could be talked into moving both tests into the WARN_ON(), however. Juergen
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 877baf2a94f4..42a3924e6d91 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -360,8 +360,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, /* Convert the size to actually allocated. */ size = 1UL << (order + XEN_PAGE_SHIFT); - if (((dev_addr + size - 1 <= dma_mask)) || - range_straddles_page_boundary(phys, size)) + if ((dev_addr + size - 1 <= dma_mask) && + !WARN_ON(range_straddles_page_boundary(phys, size))) xen_destroy_contiguous_region(phys, order); xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs);
The condition in xen_swiotlb_free_coherent() for deciding whether to call xen_destroy_contiguous_region() is wrong: in case the region to be freed is not contiguous calling xen_destroy_contiguous_region() is the wrong thing to do: it would result in inconsistent mappings of multiple PFNs to the same MFN. This will lead to various strange crashes or data corruption. Instead of calling xen_destroy_contiguous_region() in that case a warning should be issued as that situation should never occur. Cc: stable@vger.kernel.org Signed-off-by: Juergen Gross <jgross@suse.com> --- drivers/xen/swiotlb-xen.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)