[PATCH 0/2] Misc fixes for Xen grant DMA-mapping layer

[PATCH 0/2] Misc fixes for Xen grant DMA-mapping layer

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Hello all.

These are several fixes I collected when playing with virtio-net device
using Xen grant mappings.

Tested with both virtio-blk and virtio-net devices.

Oleksandr Tyshchenko (2):
  xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page()
  xen/virtio: Fix potential deadlock when accessing
    xen_grant_dma_devices

 drivers/xen/grant-dma-ops.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH 1/2] xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page()

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Take page offset into the account when calculating the number of pages
to be granted.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory access under Xen")
---
 drivers/xen/grant-dma-ops.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
index 8973fc1e9ccc..1998d0e8ce82 100644
--- a/drivers/xen/grant-dma-ops.c
+++ b/drivers/xen/grant-dma-ops.c
@@ -153,7 +153,7 @@ static dma_addr_t xen_grant_dma_map_page(struct device *dev, struct page *page,
 					 unsigned long attrs)
 {
 	struct xen_grant_dma_data *data;
-	unsigned int i, n_pages = PFN_UP(size);
+	unsigned int i, n_pages = PFN_UP(offset + size);
 	grant_ref_t grant;
 	dma_addr_t dma_handle;
 
@@ -185,7 +185,8 @@ static void xen_grant_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
 				     unsigned long attrs)
 {
 	struct xen_grant_dma_data *data;
-	unsigned int i, n_pages = PFN_UP(size);
+	unsigned long offset = dma_handle & (PAGE_SIZE - 1);
+	unsigned int i, n_pages = PFN_UP(offset + size);
 	grant_ref_t grant;
 
 	if (WARN_ON(dir == DMA_NONE))
-- 
2.25.1

Re: [PATCH 1/2] xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page()

[Stefano Stabellini]

On Wed, 5 Oct 2022, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Take page offset into the account when calculating the number of pages
> to be granted.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory access under Xen")

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  drivers/xen/grant-dma-ops.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> index 8973fc1e9ccc..1998d0e8ce82 100644
> --- a/drivers/xen/grant-dma-ops.c
> +++ b/drivers/xen/grant-dma-ops.c
> @@ -153,7 +153,7 @@ static dma_addr_t xen_grant_dma_map_page(struct device *dev, struct page *page,
>  					 unsigned long attrs)
>  {
>  	struct xen_grant_dma_data *data;
> -	unsigned int i, n_pages = PFN_UP(size);
> +	unsigned int i, n_pages = PFN_UP(offset + size);
>  	grant_ref_t grant;
>  	dma_addr_t dma_handle;
>  
> @@ -185,7 +185,8 @@ static void xen_grant_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
>  				     unsigned long attrs)
>  {
>  	struct xen_grant_dma_data *data;
> -	unsigned int i, n_pages = PFN_UP(size);
> +	unsigned long offset = dma_handle & (PAGE_SIZE - 1);
> +	unsigned int i, n_pages = PFN_UP(offset + size);
>  	grant_ref_t grant;
>  
>  	if (WARN_ON(dir == DMA_NONE))
> -- 
> 2.25.1
> 

Re: [PATCH 1/2] xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page()

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 437 bytes --]

On 05.10.22 19:48, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Take page offset into the account when calculating the number of pages
> to be granted.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory access under Xen")

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 1/2] xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page()

[Xenia Ragiadakou]

On 10/5/22 20:48, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Take page offset into the account when calculating the number of pages
> to be granted.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory access under Xen")
> ---
>   drivers/xen/grant-dma-ops.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> index 8973fc1e9ccc..1998d0e8ce82 100644
> --- a/drivers/xen/grant-dma-ops.c
> +++ b/drivers/xen/grant-dma-ops.c
> @@ -153,7 +153,7 @@ static dma_addr_t xen_grant_dma_map_page(struct device *dev, struct page *page,
>   					 unsigned long attrs)
>   {
>   	struct xen_grant_dma_data *data;
> -	unsigned int i, n_pages = PFN_UP(size);
> +	unsigned int i, n_pages = PFN_UP(offset + size);

Here, why do we use PFN_UP and not XEN_PFN_UP?
Also, since the variable 'n_pages' seems to refer to the number of 
grants (unless I got it all entirely wrong ...), wouldn't it be more 
suitable to call explicitly gnttab_count_grant()?
If the above questions have been already answered in the past, please 
ignore.

>   	grant_ref_t grant;
>   	dma_addr_t dma_handle;
>   
> @@ -185,7 +185,8 @@ static void xen_grant_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
>   				     unsigned long attrs)
>   {
>   	struct xen_grant_dma_data *data;
> -	unsigned int i, n_pages = PFN_UP(size);
> +	unsigned long offset = dma_handle & (PAGE_SIZE - 1);
> +	unsigned int i, n_pages = PFN_UP(offset + size);
>   	grant_ref_t grant;
>   
>   	if (WARN_ON(dir == DMA_NONE))

-- 
Xenia

Re: [PATCH 1/2] xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page()

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1482 bytes --]

On 06.10.22 09:35, Xenia Ragiadakou wrote:
> 
> On 10/5/22 20:48, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Take page offset into the account when calculating the number of pages
>> to be granted.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory access 
>> under Xen")
>> ---
>>   drivers/xen/grant-dma-ops.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
>> index 8973fc1e9ccc..1998d0e8ce82 100644
>> --- a/drivers/xen/grant-dma-ops.c
>> +++ b/drivers/xen/grant-dma-ops.c
>> @@ -153,7 +153,7 @@ static dma_addr_t xen_grant_dma_map_page(struct device 
>> *dev, struct page *page,
>>                        unsigned long attrs)
>>   {
>>       struct xen_grant_dma_data *data;
>> -    unsigned int i, n_pages = PFN_UP(size);
>> +    unsigned int i, n_pages = PFN_UP(offset + size);
> 
> Here, why do we use PFN_UP and not XEN_PFN_UP?
> Also, since the variable 'n_pages' seems to refer to the number of grants 
> (unless I got it all entirely wrong ...), wouldn't it be more suitable to call 
> explicitly gnttab_count_grant()?

Good point.

I think this will need another patch for switching grant-dma-ops.c to
use XEN_PAGE_SIZE and XEN_PAGE_SHIFT.


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 1/2] xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page()

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2665 bytes --]

On 06.10.22 12:14, Oleksandr Tyshchenko wrote:
> 
> 
> On Thu, Oct 6, 2022 at 11:05 AM Juergen Gross <jgross@suse.com 
> <mailto:jgross@suse.com>> wrote:
> 
>     On 06.10.22 09:35, Xenia Ragiadakou wrote:
> 
> 
> 
> Hello Xenia, Juergen
> 
> [sorry for the possible format issues]
> 
>      >
>      > On 10/5/22 20:48, Oleksandr Tyshchenko wrote:
>      >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com
>     <mailto:oleksandr_tyshchenko@epam.com>>
>      >>
>      >> Take page offset into the account when calculating the number of pages
>      >> to be granted.
>      >>
>      >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com
>     <mailto:oleksandr_tyshchenko@epam.com>>
>      >> Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory
>     access
>      >> under Xen")
>      >> ---
>      >>   drivers/xen/grant-dma-ops.c | 5 +++--
>      >>   1 file changed, 3 insertions(+), 2 deletions(-)
>      >>
>      >> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
>      >> index 8973fc1e9ccc..1998d0e8ce82 100644
>      >> --- a/drivers/xen/grant-dma-ops.c
>      >> +++ b/drivers/xen/grant-dma-ops.c
>      >> @@ -153,7 +153,7 @@ static dma_addr_t xen_grant_dma_map_page(struct device
>      >> *dev, struct page *page,
>      >>                        unsigned long attrs)
>      >>   {
>      >>       struct xen_grant_dma_data *data;
>      >> -    unsigned int i, n_pages = PFN_UP(size);
>      >> +    unsigned int i, n_pages = PFN_UP(offset + size);
>      >
>      > Here, why do we use PFN_UP and not XEN_PFN_UP?
>      > Also, since the variable 'n_pages' seems to refer to the number of grants
>      > (unless I got it all entirely wrong ...), wouldn't it be more suitable to
>     call
>      > explicitly gnttab_count_grant()?
> 
>     Good point.
> 
> 
> +1
> 
> 
>     I think this will need another patch for switching grant-dma-ops.c to
>     use XEN_PAGE_SIZE and XEN_PAGE_SHIFT.
> 
> 
> +1
> 
> I can create a separate patch for converting on top of this series, it would be 
> nice to clarify one point.
> 
> So I will convert PAGE_SIZE/PAGE_SHIFT to XEN_PAGE_SIZE/XEN_PAGE_SHIFT 
> respectively (where appropriate).

Yes, that would be the idea.

> Should the PFN_UP be converted to XEN_PFN_UP *or* use 
> gnttab_count_grant() explicitly? Personally I would prefer the former, but would 
> also be ok with the latter.

I agree XEN_PFN_UP would be better, especially as XEN_PAGE_SIZE/XEN_PAGE_SHIFT
will be used in the same functions.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH 2/2] xen/virtio: Fix potential deadlock when accessing xen_grant_dma_devices

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

As find_xen_grant_dma_data() is called from both interrupt and process
contexts, the access to xen_grant_dma_devices XArray must be protected
by xa_lock_irqsave to avoid deadlock scenario.
As XArray API doesn't provide xa_store_irqsave helper, call lockless
__xa_store directly and guard it externally.

Also move the storage of the XArray's entry to a separate helper.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory access under Xen")
---
 drivers/xen/grant-dma-ops.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
index 1998d0e8ce82..c66f56d24013 100644
--- a/drivers/xen/grant-dma-ops.c
+++ b/drivers/xen/grant-dma-ops.c
@@ -25,7 +25,7 @@ struct xen_grant_dma_data {
 	bool broken;
 };
 
-static DEFINE_XARRAY(xen_grant_dma_devices);
+static DEFINE_XARRAY_FLAGS(xen_grant_dma_devices, XA_FLAGS_LOCK_IRQ);
 
 #define XEN_GRANT_DMA_ADDR_OFF	(1ULL << 63)
 
@@ -42,14 +42,29 @@ static inline grant_ref_t dma_to_grant(dma_addr_t dma)
 static struct xen_grant_dma_data *find_xen_grant_dma_data(struct device *dev)
 {
 	struct xen_grant_dma_data *data;
+	unsigned long flags;
 
-	xa_lock(&xen_grant_dma_devices);
+	xa_lock_irqsave(&xen_grant_dma_devices, flags);
 	data = xa_load(&xen_grant_dma_devices, (unsigned long)dev);
-	xa_unlock(&xen_grant_dma_devices);
+	xa_unlock_irqrestore(&xen_grant_dma_devices, flags);
 
 	return data;
 }
 
+static int store_xen_grant_dma_data(struct device *dev,
+				    struct xen_grant_dma_data *data)
+{
+	unsigned long flags;
+	int ret;
+
+	xa_lock_irqsave(&xen_grant_dma_devices, flags);
+	ret = xa_err(__xa_store(&xen_grant_dma_devices, (unsigned long)dev, data,
+			GFP_ATOMIC));
+	xa_unlock_irqrestore(&xen_grant_dma_devices, flags);
+
+	return ret;
+}
+
 /*
  * DMA ops for Xen frontends (e.g. virtio).
  *
@@ -338,8 +353,7 @@ void xen_grant_setup_dma_ops(struct device *dev)
 	 */
 	data->backend_domid = iommu_spec.args[0];
 
-	if (xa_err(xa_store(&xen_grant_dma_devices, (unsigned long)dev, data,
-			GFP_KERNEL))) {
+	if (store_xen_grant_dma_data(dev, data)) {
 		dev_err(dev, "Cannot store Xen grant DMA data\n");
 		goto err;
 	}
-- 
2.25.1

Re: [PATCH 2/2] xen/virtio: Fix potential deadlock when accessing xen_grant_dma_devices

[Stefano Stabellini]

On Wed, 5 Oct 2022, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> As find_xen_grant_dma_data() is called from both interrupt and process
> contexts, the access to xen_grant_dma_devices XArray must be protected
> by xa_lock_irqsave to avoid deadlock scenario.
> As XArray API doesn't provide xa_store_irqsave helper, call lockless
> __xa_store directly and guard it externally.
> 
> Also move the storage of the XArray's entry to a separate helper.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory access under Xen")

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  drivers/xen/grant-dma-ops.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> index 1998d0e8ce82..c66f56d24013 100644
> --- a/drivers/xen/grant-dma-ops.c
> +++ b/drivers/xen/grant-dma-ops.c
> @@ -25,7 +25,7 @@ struct xen_grant_dma_data {
>  	bool broken;
>  };
>  
> -static DEFINE_XARRAY(xen_grant_dma_devices);
> +static DEFINE_XARRAY_FLAGS(xen_grant_dma_devices, XA_FLAGS_LOCK_IRQ);
>  
>  #define XEN_GRANT_DMA_ADDR_OFF	(1ULL << 63)
>  
> @@ -42,14 +42,29 @@ static inline grant_ref_t dma_to_grant(dma_addr_t dma)
>  static struct xen_grant_dma_data *find_xen_grant_dma_data(struct device *dev)
>  {
>  	struct xen_grant_dma_data *data;
> +	unsigned long flags;
>  
> -	xa_lock(&xen_grant_dma_devices);
> +	xa_lock_irqsave(&xen_grant_dma_devices, flags);
>  	data = xa_load(&xen_grant_dma_devices, (unsigned long)dev);
> -	xa_unlock(&xen_grant_dma_devices);
> +	xa_unlock_irqrestore(&xen_grant_dma_devices, flags);
>  
>  	return data;
>  }
>  
> +static int store_xen_grant_dma_data(struct device *dev,
> +				    struct xen_grant_dma_data *data)
> +{
> +	unsigned long flags;
> +	int ret;
> +
> +	xa_lock_irqsave(&xen_grant_dma_devices, flags);
> +	ret = xa_err(__xa_store(&xen_grant_dma_devices, (unsigned long)dev, data,
> +			GFP_ATOMIC));
> +	xa_unlock_irqrestore(&xen_grant_dma_devices, flags);
> +
> +	return ret;
> +}
> +
>  /*
>   * DMA ops for Xen frontends (e.g. virtio).
>   *
> @@ -338,8 +353,7 @@ void xen_grant_setup_dma_ops(struct device *dev)
>  	 */
>  	data->backend_domid = iommu_spec.args[0];
>  
> -	if (xa_err(xa_store(&xen_grant_dma_devices, (unsigned long)dev, data,
> -			GFP_KERNEL))) {
> +	if (store_xen_grant_dma_data(dev, data)) {
>  		dev_err(dev, "Cannot store Xen grant DMA data\n");
>  		goto err;
>  	}
> -- 
> 2.25.1
> 

Re: [PATCH 2/2] xen/virtio: Fix potential deadlock when accessing xen_grant_dma_devices

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 734 bytes --]

On 05.10.22 19:48, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> As find_xen_grant_dma_data() is called from both interrupt and process
> contexts, the access to xen_grant_dma_devices XArray must be protected
> by xa_lock_irqsave to avoid deadlock scenario.
> As XArray API doesn't provide xa_store_irqsave helper, call lockless
> __xa_store directly and guard it externally.
> 
> Also move the storage of the XArray's entry to a separate helper.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory access under Xen")

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 0/2] Misc fixes for Xen grant DMA-mapping layer

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 680 bytes --]

On 05.10.22 19:48, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Hello all.
> 
> These are several fixes I collected when playing with virtio-net device
> using Xen grant mappings.
> 
> Tested with both virtio-blk and virtio-net devices.
> 
> Oleksandr Tyshchenko (2):
>    xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page()
>    xen/virtio: Fix potential deadlock when accessing
>      xen_grant_dma_devices
> 
>   drivers/xen/grant-dma-ops.c | 29 ++++++++++++++++++++++-------
>   1 file changed, 22 insertions(+), 7 deletions(-)
> 

Series pushed to xen/tip.git for-linus-6.1


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]