[PATCH V2] xen-swiotlb: use actually allocated size on check physical continuous

[PATCH V2] xen-swiotlb: use actually allocated size on check physical continuous

[Joe Jin]

xen_swiotlb_{alloc,free}_coherent() allocate/free memory by order,
but passed required size to range_straddles_page_boundary(),
when first pages are physical continuous,
range_straddles_page_boundary() returned true, then did not
exchanged memory with Xen, later on free memory, it tried to
exchanged non-contiguous memory with Xen, then kernel panic.

Signed-off-by: Joe Jin <joe.jin@oracle.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Christoph Helwig <hch@lst.de>
Cc: Dongli Zhang <dongli.zhang@oracle.com>
Cc: John Sobecki <john.sobecki@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)

Re: [PATCH V2] xen-swiotlb: use actually allocated size on check physical continuous

[Konrad Rzeszutek Wilk]

On Tue, Oct 16, 2018 at 03:21:16PM -0700, Joe Jin wrote:
> xen_swiotlb_{alloc,free}_coherent() allocate/free memory by order,
> but passed required size to range_straddles_page_boundary(),
> when first pages are physical continuous,
> range_straddles_page_boundary() returned true, then did not
> exchanged memory with Xen, later on free memory, it tried to
> exchanged non-contiguous memory with Xen, then kernel panic.

I have a hard time understanding the commit message.

I think you mean to say:

xen_swiotlb_{alloc,free}_coherent() allocate/free memory based on the
order of the pages and not size argument (bytes). This is inconsistent with
range_straddles_page_boundary and memset which use the 'size' value,
which may lead to not exchanging memory with Xen (range_straddles_page_boundary()
returned true). And then the call to xen_swiotlb_free_coherent() would
actually try to exchange the memory with Xen, leading to the kernel
hitting an BUG (as the hypercall returned an error).

This patch fixes it by making the 'size' variable be of the same size
as the amount of memory allocated.

I checked it as such..
> 
> Signed-off-by: Joe Jin <joe.jin@oracle.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Christoph Helwig <hch@lst.de>
> Cc: Dongli Zhang <dongli.zhang@oracle.com>
> Cc: John Sobecki <john.sobecki@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)
> 
> 

Re: [PATCH V2] xen-swiotlb: use actually allocated size on check physical continuous

[Joe Jin]

On 10/18/18 4:52 PM, Konrad Rzeszutek Wilk wrote:
> On Tue, Oct 16, 2018 at 03:21:16PM -0700, Joe Jin wrote:
>> xen_swiotlb_{alloc,free}_coherent() allocate/free memory by order,
>> but passed required size to range_straddles_page_boundary(),
>> when first pages are physical continuous,
>> range_straddles_page_boundary() returned true, then did not
>> exchanged memory with Xen, later on free memory, it tried to
>> exchanged non-contiguous memory with Xen, then kernel panic.
> 
> I have a hard time understanding the commit message.
> 
> I think you mean to say:
> 
> xen_swiotlb_{alloc,free}_coherent() allocate/free memory based on the
> order of the pages and not size argument (bytes). This is inconsistent with
> range_straddles_page_boundary and memset which use the 'size' value,
> which may lead to not exchanging memory with Xen (range_straddles_page_boundary()
> returned true). And then the call to xen_swiotlb_free_coherent() would
> actually try to exchange the memory with Xen, leading to the kernel
> hitting an BUG (as the hypercall returned an error).

Sorry for the confusing, yes you right.

BTW: panic triggered by xen_remap_exchanged_ptes()->xen_mc_issue().

Panic calltrace as below:

 [ 1129.150648] WARNING: CPU: 1 PID: 21001 at arch/x86/xen/multicalls.c:129
xen_mc_flush+0x1ce/0x1e0()
[ 1129.150653] Modules linked in: ocfs2 mptctl mptbase xen_netback
xen_blkback xen_gntalloc xen_gntdev xen_evtchn xenfs xen_privcmd ocfs2_dlmfs
ocfs2_stack_o2cb ocfs2_dlm ocfs2_nodemanager ocfs2_stackglue configfs bnx2fc
fcoe libfcoe libfc sunrpc 8021q mrp garp bridge stp llc bonding
dm_round_robin dm_multipath iTCO_wdt iTCO_vendor_support pcspkr shpchp
sb_edac edac_core i2c_i801 i2c_core cdc_ether usbnet mii lpc_ich mfd_core
ioatdma sg ipmi_devintf ipmi_si ipmi_msghandler ext4 jbd2 mbcache2 sd_mod
usb_storage ahci libahci megaraid_sas ixgbe dca ptp pps_core vxlan udp_tunnel
ip6_udp_tunnel qla2xxx scsi_transport_fc crc32c_intel be2iscsi bnx2i cnic uio
cxgb4i cxgb4 cxgb3i libcxgbi ipv6 cxgb3 mdio libiscsi_tcp qla4xxx
iscsi_boot_sysfs libiscsi scsi_transport_iscsi wmi dm_mirror dm_region_hash
dm_log dm_mod
[ 1129.150719]
[ 1129.150722] CPU: 1 PID: 21001 Comm: blkid Tainted: G        W        
4.1.12-124.16.3.el6uek.x86_64 #2
[ 1129.150723] Hardware name: Oracle Corporation ORACLE SERVER
X5-2/ASM,MOTHERBOARD,1U, BIOS 30130400 02/08/2018
[ 1129.150727]  0000000000000000 ffff8803ec7c3c78 ffffffff816e52a3
0000000000000000
[ 1129.150732]  ffffffff819bc5c3 ffff8803ec7c3cb8 ffffffff8108481a
ffff8803ec7c3d18
[ 1129.150735]  ffff8804a6a4d2c0 0000000000000003 ffff8804994df800
0000000000000001
[ 1129.150740] Call Trace:
[ 1129.150746]  [<ffffffff816e52a3>] dump_stack+0x63/0x81
[ 1129.150753]  [<ffffffff8108481a>] warn_slowpath_common+0x8a/0xc0
[ 1129.150756]  [<ffffffff8108490a>] warn_slowpath_null+0x1a/0x20
[ 1129.150759]  [<ffffffff8100559e>] xen_mc_flush+0x1ce/0x1e0
[ 1129.150764]  [<ffffffff81007af9>] __xen_pgd_unpin+0x109/0x250
[ 1129.150767]  [<ffffffff81007db7>] xen_exit_mmap+0x177/0x2a0
[ 1129.150770]  [<ffffffff811c3628>] exit_mmap+0x48/0x160
[ 1129.150776]  [<ffffffff8112c68c>] ? __audit_free+0x1cc/0x220
[ 1129.150784]  [<ffffffff811e725e>] ? kfree+0x16e/0x180
[ 1129.150787]  [<ffffffff81081cf3>] mmput+0x63/0x100
[ 1129.150790]  [<ffffffff81087263>] do_exit+0x343/0xa90
[ 1129.150793]  [<ffffffff8112c794>] ? __audit_syscall_entry+0xb4/0x110
[ 1129.150796]  [<ffffffff81025a2c>] ? do_audit_syscall_entry+0x6c/0x70
[ 1129.150799]  [<ffffffff81087a45>] do_group_exit+0x45/0xb0
[ 1129.150805]  [<ffffffff81087ac4>] SyS_exit_group+0x14/0x20
[ 1129.150807]  [<ffffffff816edc5c>] system_call_fastpath+0x18/0xd6
[ 1129.150810] ---[ end trace 7aec0081e2a07193 ]---

Do you think the patch fine or no? if yes I'll reorg patch description
and resend it.

Thanks,
Joe

> 
> This patch fixes it by making the 'size' variable be of the same size
> as the amount of memory allocated.
> 
> I checked it as such..
>>
>> Signed-off-by: Joe Jin <joe.jin@oracle.com>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Cc: Christoph Helwig <hch@lst.de>
>> Cc: Dongli Zhang <dongli.zhang@oracle.com>
>> Cc: John Sobecki <john.sobecki@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)
>>
>>


-- 
Oracle <http://www.oracle.com>
Joe Jin | Software Development Director 
ORACLE | Linux and Virtualization
500 Oracle Parkway Redwood City, CA US 94065

Re: [PATCH V2] xen-swiotlb: use actually allocated size on check physical continuous

[Konrad Rzeszutek Wilk]

On Thu, Oct 18, 2018 at 07:16:04PM -0700, Joe Jin wrote:
> On 10/18/18 4:52 PM, Konrad Rzeszutek Wilk wrote:
> > On Tue, Oct 16, 2018 at 03:21:16PM -0700, Joe Jin wrote:
> >> xen_swiotlb_{alloc,free}_coherent() allocate/free memory by order,
> >> but passed required size to range_straddles_page_boundary(),
> >> when first pages are physical continuous,
> >> range_straddles_page_boundary() returned true, then did not
> >> exchanged memory with Xen, later on free memory, it tried to
> >> exchanged non-contiguous memory with Xen, then kernel panic.
> > 
> > I have a hard time understanding the commit message.
> > 
> > I think you mean to say:
> > 
> > xen_swiotlb_{alloc,free}_coherent() allocate/free memory based on the
> > order of the pages and not size argument (bytes). This is inconsistent with
> > range_straddles_page_boundary and memset which use the 'size' value,
> > which may lead to not exchanging memory with Xen (range_straddles_page_boundary()
> > returned true). And then the call to xen_swiotlb_free_coherent() would
> > actually try to exchange the memory with Xen, leading to the kernel
> > hitting an BUG (as the hypercall returned an error).
> 
> Sorry for the confusing, yes you right.
> 
> BTW: panic triggered by xen_remap_exchanged_ptes()->xen_mc_issue().
> 
> Panic calltrace as below:
> 
>  [ 1129.150648] WARNING: CPU: 1 PID: 21001 at arch/x86/xen/multicalls.c:129
> xen_mc_flush+0x1ce/0x1e0()
> [ 1129.150653] Modules linked in: ocfs2 mptctl mptbase xen_netback
> xen_blkback xen_gntalloc xen_gntdev xen_evtchn xenfs xen_privcmd ocfs2_dlmfs
> ocfs2_stack_o2cb ocfs2_dlm ocfs2_nodemanager ocfs2_stackglue configfs bnx2fc
> fcoe libfcoe libfc sunrpc 8021q mrp garp bridge stp llc bonding
> dm_round_robin dm_multipath iTCO_wdt iTCO_vendor_support pcspkr shpchp
> sb_edac edac_core i2c_i801 i2c_core cdc_ether usbnet mii lpc_ich mfd_core
> ioatdma sg ipmi_devintf ipmi_si ipmi_msghandler ext4 jbd2 mbcache2 sd_mod
> usb_storage ahci libahci megaraid_sas ixgbe dca ptp pps_core vxlan udp_tunnel
> ip6_udp_tunnel qla2xxx scsi_transport_fc crc32c_intel be2iscsi bnx2i cnic uio
> cxgb4i cxgb4 cxgb3i libcxgbi ipv6 cxgb3 mdio libiscsi_tcp qla4xxx
> iscsi_boot_sysfs libiscsi scsi_transport_iscsi wmi dm_mirror dm_region_hash
> dm_log dm_mod
> [ 1129.150719]
> [ 1129.150722] CPU: 1 PID: 21001 Comm: blkid Tainted: G        W        
> 4.1.12-124.16.3.el6uek.x86_64 #2
> [ 1129.150723] Hardware name: Oracle Corporation ORACLE SERVER
> X5-2/ASM,MOTHERBOARD,1U, BIOS 30130400 02/08/2018
> [ 1129.150727]  0000000000000000 ffff8803ec7c3c78 ffffffff816e52a3
> 0000000000000000
> [ 1129.150732]  ffffffff819bc5c3 ffff8803ec7c3cb8 ffffffff8108481a
> ffff8803ec7c3d18
> [ 1129.150735]  ffff8804a6a4d2c0 0000000000000003 ffff8804994df800
> 0000000000000001
> [ 1129.150740] Call Trace:
> [ 1129.150746]  [<ffffffff816e52a3>] dump_stack+0x63/0x81
> [ 1129.150753]  [<ffffffff8108481a>] warn_slowpath_common+0x8a/0xc0
> [ 1129.150756]  [<ffffffff8108490a>] warn_slowpath_null+0x1a/0x20
> [ 1129.150759]  [<ffffffff8100559e>] xen_mc_flush+0x1ce/0x1e0
> [ 1129.150764]  [<ffffffff81007af9>] __xen_pgd_unpin+0x109/0x250
> [ 1129.150767]  [<ffffffff81007db7>] xen_exit_mmap+0x177/0x2a0
> [ 1129.150770]  [<ffffffff811c3628>] exit_mmap+0x48/0x160
> [ 1129.150776]  [<ffffffff8112c68c>] ? __audit_free+0x1cc/0x220
> [ 1129.150784]  [<ffffffff811e725e>] ? kfree+0x16e/0x180
> [ 1129.150787]  [<ffffffff81081cf3>] mmput+0x63/0x100
> [ 1129.150790]  [<ffffffff81087263>] do_exit+0x343/0xa90
> [ 1129.150793]  [<ffffffff8112c794>] ? __audit_syscall_entry+0xb4/0x110
> [ 1129.150796]  [<ffffffff81025a2c>] ? do_audit_syscall_entry+0x6c/0x70
> [ 1129.150799]  [<ffffffff81087a45>] do_group_exit+0x45/0xb0
> [ 1129.150805]  [<ffffffff81087ac4>] SyS_exit_group+0x14/0x20
> [ 1129.150807]  [<ffffffff816edc5c>] system_call_fastpath+0x18/0xd6
> [ 1129.150810] ---[ end trace 7aec0081e2a07193 ]---
> 
> Do you think the patch fine or no? if yes I'll reorg patch description
> and resend it.

I will fold in the stack trace in later tonight. Thanks!
> 
> Thanks,
> Joe
> 
> > 
> > This patch fixes it by making the 'size' variable be of the same size
> > as the amount of memory allocated.
> > 
> > I checked it as such..
> >>
> >> Signed-off-by: Joe Jin <joe.jin@oracle.com>
> >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> >> Cc: Christoph Helwig <hch@lst.de>
> >> Cc: Dongli Zhang <dongli.zhang@oracle.com>
> >> Cc: John Sobecki <john.sobecki@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)
> >>
> >>
> 
> 
> -- 
> Oracle <http://www.oracle.com>
> Joe Jin | Software Development Director 
> ORACLE | Linux and Virtualization
> 500 Oracle Parkway Redwood City, CA US 94065

Re: [PATCH V2] xen-swiotlb: use actually allocated size on check physical continuous

[Joe Jin]

On 10/19/18 6:43 AM, Konrad Rzeszutek Wilk wrote:
> I will fold in the stack trace in later tonight. Thanks!

Thank you very much!