Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [Xen-devel] [PATCH] xen-swiotlb: use actually allocated size on
 check physical contiguous
To:     "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
References: <d578b69a-848b-2c86-175a-1b1933d82a36@oracle.com>
Cc:     Joe Jin <joe.jin@oracle.com>,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        John Sobecki <john.sobecki@oracle.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        stable@vger.kernel.org
From:   Dongli Zhang <dongli.zhang@oracle.com>
Message-ID: <f3befca2-fc65-81b8-c9ed-311de7967958@oracle.com>
Date:   Wed, 5 Sep 2018 07:14:26 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.3.0
MIME-Version: 1.0
In-Reply-To: <d578b69a-848b-2c86-175a-1b1933d82a36@oracle.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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);
>