lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 23 Apr 2019 10:05:54 -0700 (PDT)
From:   Stefano Stabellini <sstabellini@...nel.org>
To:     Juergen Gross <jgross@...e.com>
cc:     xen-devel@...ts.xenproject.org, linux-kernel@...r.kernel.org,
        iommu@...ts.linux-foundation.org, konrad.wilk@...cle.com,
        boris.ostrovsky@...cle.com, sstabellini@...nel.org
Subject: Re: [PATCH 3/3] xen/swiotlb: remember having called
 xen_create_contiguous_region()

On Tue, 23 Apr 2019, Juergen Gross wrote:
> Instead of always calling xen_destroy_contiguous_region() in case the
> memory is DMA-able for the used device, do so only in case it has been
> made DMA-able via xen_create_contiguous_region() before.
> 
> This will avoid a lot of xen_destroy_contiguous_region() calls for
> 64-bit capable devices.
> 
> As the memory in question is owned by swiotlb-xen the PG_owner_priv_1
> flag of the first allocated page can be used for remembering.

Although the patch looks OK, this sentence puzzles me. Why do you say
that the memory in question is owned by swiotlb-xen? Because it was
returned by xen_alloc_coherent_pages? Both the x86 and the Arm
implementation return fresh new memory, hence, it should be safe to set
the PageOwnerPriv1 flag?

My concern with this approach is with the semantics of PG_owner_priv_1.
Is a page marked with PG_owner_priv_1 only supposed to be used by the
owner?


> Signed-off-by: Juergen Gross <jgross@...e.com>
> ---
>  drivers/xen/swiotlb-xen.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 43b6e65ae256..a72f181d8e20 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -321,6 +321,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size,
>  			xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs);
>  			return NULL;
>  		}
> +		SetPageOwnerPriv1(virt_to_page(ret));
>  	}
>  	memset(ret, 0, size);
>  	return ret;
> @@ -344,9 +345,11 @@ 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) &&
> -	    !WARN_ON(range_straddles_page_boundary(phys, size)))
> -		xen_destroy_contiguous_region(phys, order);
> +	if (PageOwnerPriv1(virt_to_page(vaddr))) {
> +		if (!WARN_ON(range_straddles_page_boundary(phys, size)))
> +			xen_destroy_contiguous_region(phys, order);
> +		ClearPageOwnerPriv1(virt_to_page(vaddr));
> +	}
>  
>  	xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs);
>  }

Powered by blists - more mailing lists