* [PATCH] xen-swiotlb: use actually allocated size on check physical contiguous
@ 2018-09-04 18:16 Joe Jin
2018-09-04 19:13 ` Greg KH
2018-09-04 23:14 ` [Xen-devel] " Dongli Zhang
0 siblings, 2 replies; 5+ messages in thread
From: Joe Jin @ 2018-09-04 18:16 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, John Sobecki, DONGLI.ZHANG
Cc: xen-devel, linux-kernel, stable
xen_swiotlb_{alloc,free}_coherent() actually allocate/free size by order
but used the required size to check if address is physical contiguous,
if first pages are physical contiguous also passed
range_straddles_page_boundary() check, but others were not it will
lead kernel panic.
Signed-off-by: Joe Jin <joe.jin@oracle.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
drivers/xen/swiotlb-xen.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index a6f9ba85dc4b..aa081f806728 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -303,6 +303,9 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size,
*/
flags &= ~(__GFP_DMA | __GFP_HIGHMEM);
+ /* Convert the size to actually allocated. */
+ size = 1UL << (order + XEN_PAGE_SHIFT);
+
/* On ARM this function returns an ioremap'ped virtual address for
* which virt_to_phys doesn't return the corresponding physical
* address. In fact on ARM virt_to_phys only works for kernel direct
@@ -351,6 +354,9 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
* physical address */
phys = xen_bus_to_phys(dev_addr);
+ /* 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))
xen_destroy_contiguous_region(phys, order);
--
2.15.2 (Apple Git-101.1)
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] xen-swiotlb: use actually allocated size on check physical contiguous 2018-09-04 18:16 [PATCH] xen-swiotlb: use actually allocated size on check physical contiguous Joe Jin @ 2018-09-04 19:13 ` Greg KH 2018-09-04 23:14 ` [Xen-devel] " Dongli Zhang 1 sibling, 0 replies; 5+ messages in thread From: Greg KH @ 2018-09-04 19:13 UTC (permalink / raw) To: Joe Jin Cc: Konrad Rzeszutek Wilk, John Sobecki, DONGLI.ZHANG, xen-devel, linux-kernel, stable On Tue, Sep 04, 2018 at 11:16:58AM -0700, Joe Jin wrote: > xen_swiotlb_{alloc,free}_coherent() actually allocate/free size by order > but used the required size to check if address is physical contiguous, > if first pages are physical contiguous also passed > range_straddles_page_boundary() check, but others were not it will > lead kernel panic. > > Signed-off-by: Joe Jin <joe.jin@oracle.com> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > drivers/xen/swiotlb-xen.c | 6 ++++++ > 1 file changed, 6 insertions(+) > <formletter> This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly. </formletter> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Xen-devel] [PATCH] xen-swiotlb: use actually allocated size on check physical contiguous 2018-09-04 18:16 [PATCH] xen-swiotlb: use actually allocated size on check physical contiguous Joe Jin 2018-09-04 19:13 ` Greg KH @ 2018-09-04 23:14 ` Dongli Zhang 2018-09-04 23:36 ` Andrew Cooper 2018-09-07 22:47 ` Joe Jin 1 sibling, 2 replies; 5+ messages in thread From: Dongli Zhang @ 2018-09-04 23:14 UTC (permalink / raw) To: xen-devel Cc: Joe Jin, Konrad Rzeszutek Wilk, John Sobecki, linux-kernel, stable Below module would help people reproduce the issue to understand the symptom: https://github.com/finallyjustice/patchset/blob/master/xen-swiotlb-panic.c In addition, on the xen hypervisor side, the memory_exchange() in xen hypervisor does not check if the the pfn of input mfn belong to the same extent are continuous in guest domain. As a result, the wrong page is stolen from guest domain. Can we assume it is fine to not check if pfn of mfn are continuous in xen hypervisor? I think it is OK as the guest domain is responsible to maintain its memory pages. xen is happy to serve any operations from guest domain, unless such operation is harmful to xen hypervisor or other domains. In the worst cause, the dom0 (or domU with passthrough) would panic itself. 496 static long memory_exchange(... ... ... 609 /* Steal a chunk's worth of input pages from the domain. */ 610 for ( j = 0; j < (1UL << in_chunk_order); j++ ) 611 { 612 if ( unlikely(__copy_from_guest_offset( 613 &gmfn, exch.in.extent_start, (i<<in_chunk_order)+j, 1)) ) 614 { 615 rc = -EFAULT; 616 goto fail; 617 } 618 -------> not checking if the gfn of gmfn are continuous in below 'for loop' for each extent. 619 for ( k = 0; k < (1UL << exch.in.extent_order); k++ ) 620 { 621 #ifdef CONFIG_X86 622 p2m_type_t p2mt; 623 624 /* Shared pages cannot be exchanged */ 625 mfn = get_gfn_unshare(d, gmfn + k, &p2mt); 626 if ( p2m_is_shared(p2mt) ) 627 { 628 put_gfn(d, gmfn + k); 629 rc = -ENOMEM; 630 goto fail; 631 } 632 #else /* !CONFIG_X86 */ 633 mfn = gfn_to_mfn(d, _gfn(gmfn + k)); 634 #endif 635 if ( unlikely(!mfn_valid(mfn)) ) 636 { 637 put_gfn(d, gmfn + k); 638 rc = -EINVAL; 639 goto fail; 640 } 641 642 page = mfn_to_page(mfn); 643 -----> As a result, the wrong page is stolen. 644 rc = steal_page(d, page, MEMF_no_refcount); 645 if ( unlikely(rc) ) 646 { 647 put_gfn(d, gmfn + k); 648 goto fail; 649 } 650 651 page_list_add(page, &in_chunk_list); 652 put_gfn(d, gmfn + k); 653 } 654 } Dongli Zhang On 09/05/2018 02:16 AM, Joe Jin wrote: > xen_swiotlb_{alloc,free}_coherent() actually allocate/free size by order > but used the required size to check if address is physical contiguous, > if first pages are physical contiguous also passed > range_straddles_page_boundary() check, but others were not it will > lead kernel panic. > > Signed-off-by: Joe Jin <joe.jin@oracle.com> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > drivers/xen/swiotlb-xen.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index a6f9ba85dc4b..aa081f806728 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -303,6 +303,9 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size, > */ > flags &= ~(__GFP_DMA | __GFP_HIGHMEM); > > + /* Convert the size to actually allocated. */ > + size = 1UL << (order + XEN_PAGE_SHIFT); > + > /* On ARM this function returns an ioremap'ped virtual address for > * which virt_to_phys doesn't return the corresponding physical > * address. In fact on ARM virt_to_phys only works for kernel direct > @@ -351,6 +354,9 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, > * physical address */ > phys = xen_bus_to_phys(dev_addr); > > + /* 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)) > xen_destroy_contiguous_region(phys, order); > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Xen-devel] [PATCH] xen-swiotlb: use actually allocated size on check physical contiguous 2018-09-04 23:14 ` [Xen-devel] " Dongli Zhang @ 2018-09-04 23:36 ` Andrew Cooper 2018-09-07 22:47 ` Joe Jin 1 sibling, 0 replies; 5+ messages in thread From: Andrew Cooper @ 2018-09-04 23:36 UTC (permalink / raw) To: Dongli Zhang, xen-devel; +Cc: John Sobecki, Joe Jin, linux-kernel, stable On 05/09/18 00:14, Dongli Zhang wrote: > Below module would help people reproduce the issue to understand the symptom: > > https://github.com/finallyjustice/patchset/blob/master/xen-swiotlb-panic.c > > In addition, on the xen hypervisor side, the memory_exchange() in xen hypervisor > does not check if the the pfn of input mfn belong to the same extent are > continuous in guest domain. As a result, the wrong page is stolen from guest domain. > > Can we assume it is fine to not check if pfn of mfn are continuous in xen > hypervisor? The purpose of the memory_exchange hypercall is to exchange any arbitrary set of guest frames with an equivalently sized set frames with different properties. The practical use is for PV guests to be able to create a DMA buffer which is physically continuous. Xen does not, and indeed should not, care about the properties of the input frame list. ~Andrew ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Xen-devel] [PATCH] xen-swiotlb: use actually allocated size on check physical contiguous 2018-09-04 23:14 ` [Xen-devel] " Dongli Zhang 2018-09-04 23:36 ` Andrew Cooper @ 2018-09-07 22:47 ` Joe Jin 1 sibling, 0 replies; 5+ messages in thread From: Joe Jin @ 2018-09-07 22:47 UTC (permalink / raw) To: Dongli Zhang, xen-devel; +Cc: Konrad Rzeszutek Wilk, John Sobecki, linux-kernel Anybody would like to help review this patch? Thanks, Joe On 09/05/2018 02:16 AM, Joe Jin wrote > xen_swiotlb_{alloc,free}_coherent() actually allocate/free size by order > but used the required size to check if address is physical contiguous, > if first pages are physical contiguous also passed > range_straddles_page_boundary() check, but others were not it will > lead kernel panic. > > Signed-off-by: Joe Jin <joe.jin@oracle.com> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > drivers/xen/swiotlb-xen.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index a6f9ba85dc4b..aa081f806728 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -303,6 +303,9 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size, > */ > flags &= ~(__GFP_DMA | __GFP_HIGHMEM); > > + /* Convert the size to actually allocated. */ > + size = 1UL << (order + XEN_PAGE_SHIFT); > + > /* On ARM this function returns an ioremap'ped virtual address for > * which virt_to_phys doesn't return the corresponding physical > * address. In fact on ARM virt_to_phys only works for kernel direct > @@ -351,6 +354,9 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, > * physical address */ > phys = xen_bus_to_phys(dev_addr); > > + /* 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)) > xen_destroy_contiguous_region(phys, order); > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-09-07 22:47 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-04 18:16 [PATCH] xen-swiotlb: use actually allocated size on check physical contiguous Joe Jin 2018-09-04 19:13 ` Greg KH 2018-09-04 23:14 ` [Xen-devel] " Dongli Zhang 2018-09-04 23:36 ` Andrew Cooper 2018-09-07 22:47 ` Joe Jin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).