[Xen-devel][PATCH 1/2] xen/gntdev: Do not destroy context while dma-bufs are in use

[Xen-devel][PATCH 1/2] xen/gntdev: Do not destroy context while dma-bufs are in use

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

If there are exported DMA buffers which are still in use and
grant device is closed by either normal user-space close or by
a signal this leads to the grant device context to be destroyed,
thus making it not possible to correctly destroy those exported
buffers when they are returned back to gntdev and makes the module
crash:

[  339.617540] [<ffff00000854c0d8>] dmabuf_exp_ops_release+0x40/0xa8
[  339.617560] [<ffff00000867a6e8>] dma_buf_release+0x60/0x190
[  339.617577] [<ffff0000082211f0>] __fput+0x88/0x1d0
[  339.617589] [<ffff000008221394>] ____fput+0xc/0x18
[  339.617607] [<ffff0000080ed4e4>] task_work_run+0x9c/0xc0
[  339.617622] [<ffff000008089714>] do_notify_resume+0xfc/0x108

Fix this by referencing gntdev on each DMA buffer export and
unreferencing on buffer release.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
 drivers/xen/gntdev-dmabuf.c | 12 +++++++++++-
 drivers/xen/gntdev-dmabuf.h |  2 +-
 drivers/xen/gntdev.c        |  2 +-
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
index cba6b586bfbd..d97fcfc5e558 100644
--- a/drivers/xen/gntdev-dmabuf.c
+++ b/drivers/xen/gntdev-dmabuf.c
@@ -80,6 +80,12 @@ struct gntdev_dmabuf_priv {
 	struct list_head imp_list;
 	/* This is the lock which protects dma_buf_xxx lists. */
 	struct mutex lock;
+	/*
+	 * We reference this file while exporting dma-bufs, so
+	 * the grant device context is not destroyed while there are
+	 * external users alive.
+	 */
+	struct file *filp;
 };
 
 /* DMA buffer export support. */
@@ -311,6 +317,7 @@ static void dmabuf_exp_release(struct kref *kref)
 
 	dmabuf_exp_wait_obj_signal(gntdev_dmabuf->priv, gntdev_dmabuf);
 	list_del(&gntdev_dmabuf->next);
+	fput(gntdev_dmabuf->priv->filp);
 	kfree(gntdev_dmabuf);
 }
 
@@ -423,6 +430,7 @@ static int dmabuf_exp_from_pages(struct gntdev_dmabuf_export_args *args)
 	mutex_lock(&args->dmabuf_priv->lock);
 	list_add(&gntdev_dmabuf->next, &args->dmabuf_priv->exp_list);
 	mutex_unlock(&args->dmabuf_priv->lock);
+	get_file(gntdev_dmabuf->priv->filp);
 	return 0;
 
 fail:
@@ -834,7 +842,7 @@ long gntdev_ioctl_dmabuf_imp_release(struct gntdev_priv *priv,
 	return dmabuf_imp_release(priv->dmabuf_priv, op.fd);
 }
 
-struct gntdev_dmabuf_priv *gntdev_dmabuf_init(void)
+struct gntdev_dmabuf_priv *gntdev_dmabuf_init(struct file *filp)
 {
 	struct gntdev_dmabuf_priv *priv;
 
@@ -847,6 +855,8 @@ struct gntdev_dmabuf_priv *gntdev_dmabuf_init(void)
 	INIT_LIST_HEAD(&priv->exp_wait_list);
 	INIT_LIST_HEAD(&priv->imp_list);
 
+	priv->filp = filp;
+
 	return priv;
 }
 
diff --git a/drivers/xen/gntdev-dmabuf.h b/drivers/xen/gntdev-dmabuf.h
index 7220a53d0fc5..3d9b9cf9d5a1 100644
--- a/drivers/xen/gntdev-dmabuf.h
+++ b/drivers/xen/gntdev-dmabuf.h
@@ -14,7 +14,7 @@
 struct gntdev_dmabuf_priv;
 struct gntdev_priv;
 
-struct gntdev_dmabuf_priv *gntdev_dmabuf_init(void);
+struct gntdev_dmabuf_priv *gntdev_dmabuf_init(struct file *filp);
 
 void gntdev_dmabuf_fini(struct gntdev_dmabuf_priv *priv);
 
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index b0b02a501167..9d8e02cfd480 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -600,7 +600,7 @@ static int gntdev_open(struct inode *inode, struct file *flip)
 	mutex_init(&priv->lock);
 
 #ifdef CONFIG_XEN_GNTDEV_DMABUF
-	priv->dmabuf_priv = gntdev_dmabuf_init();
+	priv->dmabuf_priv = gntdev_dmabuf_init(flip);
 	if (IS_ERR(priv->dmabuf_priv)) {
 		ret = PTR_ERR(priv->dmabuf_priv);
 		kfree(priv);
-- 
2.20.1

[Xen-devel][PATCH 2/2] xen/gntdev: Check and release imported dma-bufs on close

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Check if there are any imported dma-bufs left not released by
user-space when grant device's release callback is called and
free those if this is the case. This can happen if user-space
leaks the buffers because of a bug or application has been
terminated for any reason.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
 drivers/xen/gntdev-dmabuf.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
index d97fcfc5e558..2c4f324f8626 100644
--- a/drivers/xen/gntdev-dmabuf.c
+++ b/drivers/xen/gntdev-dmabuf.c
@@ -745,6 +745,14 @@ static int dmabuf_imp_release(struct gntdev_dmabuf_priv *priv, u32 fd)
 	return 0;
 }
 
+static void dmabuf_imp_release_all(struct gntdev_dmabuf_priv *priv)
+{
+	struct gntdev_dmabuf *q, *gntdev_dmabuf;
+
+	list_for_each_entry_safe(gntdev_dmabuf, q, &priv->imp_list, next)
+		dmabuf_imp_release(priv, gntdev_dmabuf->fd);
+}
+
 /* DMA buffer IOCTL support. */
 
 long gntdev_ioctl_dmabuf_exp_from_refs(struct gntdev_priv *priv, int use_ptemod,
@@ -862,5 +870,6 @@ struct gntdev_dmabuf_priv *gntdev_dmabuf_init(struct file *filp)
 
 void gntdev_dmabuf_fini(struct gntdev_dmabuf_priv *priv)
 {
+	dmabuf_imp_release_all(priv);
 	kfree(priv);
 }
-- 
2.20.1

Re: [Xen-devel][PATCH 2/2] xen/gntdev: Check and release imported dma-bufs on close

[Boris Ostrovsky]

On 2/14/19 9:23 AM, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> Check if there are any imported dma-bufs left not released by
> user-space when grant device's release callback is called and
> free those if this is the case. This can happen if user-space
> leaks the buffers because of a bug or application has been
> terminated for any reason.
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Reviewed-by: Boris Ostrovsky@oracle.com>

Re: [Xen-devel][PATCH 2/2] xen/gntdev: Check and release imported dma-bufs on close

[Juergen Gross]

On 14/02/2019 15:23, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> Check if there are any imported dma-bufs left not released by
> user-space when grant device's release callback is called and
> free those if this is the case. This can happen if user-space
> leaks the buffers because of a bug or application has been
> terminated for any reason.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Applied-to: xen/tip.git for-linus-5.1


Juergen

Re: [Xen-devel][PATCH 1/2] xen/gntdev: Do not destroy context while dma-bufs are in use

[Boris Ostrovsky]

On 2/14/19 9:23 AM, Oleksandr Andrushchenko wrote:
>  
>  /* DMA buffer export support. */
> @@ -311,6 +317,7 @@ static void dmabuf_exp_release(struct kref *kref)
>  
>  	dmabuf_exp_wait_obj_signal(gntdev_dmabuf->priv, gntdev_dmabuf);
>  	list_del(&gntdev_dmabuf->next);
> +	fput(gntdev_dmabuf->priv->filp);
>  	kfree(gntdev_dmabuf);
>  }
>  
> @@ -423,6 +430,7 @@ static int dmabuf_exp_from_pages(struct gntdev_dmabuf_export_args *args)
>  	mutex_lock(&args->dmabuf_priv->lock);
>  	list_add(&gntdev_dmabuf->next, &args->dmabuf_priv->exp_list);
>  	mutex_unlock(&args->dmabuf_priv->lock);
> +	get_file(gntdev_dmabuf->priv->filp);

Not fget()?

-boris

Re: [Xen-devel][PATCH 1/2] xen/gntdev: Do not destroy context while dma-bufs are in use

[Oleksandr Andrushchenko]

On 2/15/19 5:03 PM, Boris Ostrovsky wrote:
> On 2/14/19 9:23 AM, Oleksandr Andrushchenko wrote:
>>   
>>   /* DMA buffer export support. */
>> @@ -311,6 +317,7 @@ static void dmabuf_exp_release(struct kref *kref)
>>   
>>   	dmabuf_exp_wait_obj_signal(gntdev_dmabuf->priv, gntdev_dmabuf);
>>   	list_del(&gntdev_dmabuf->next);
>> +	fput(gntdev_dmabuf->priv->filp);
>>   	kfree(gntdev_dmabuf);
>>   }
>>   
>> @@ -423,6 +430,7 @@ static int dmabuf_exp_from_pages(struct gntdev_dmabuf_export_args *args)
>>   	mutex_lock(&args->dmabuf_priv->lock);
>>   	list_add(&gntdev_dmabuf->next, &args->dmabuf_priv->exp_list);
>>   	mutex_unlock(&args->dmabuf_priv->lock);
>> +	get_file(gntdev_dmabuf->priv->filp);
> Not fget()?
fget wants file descriptor [1] and returns struct file *,
but we already have struct file*, so I use get_file [2]
which does what I need - increments the reference counter
on the file
>
> -boris
>
>
[1] 
https://elixir.bootlin.com/linux/v5.0-rc6/source/include/linux/file.h#L46
[2] https://elixir.bootlin.com/linux/v5.0-rc6/source/include/linux/fs.h#L949

Re: [Xen-devel][PATCH 1/2] xen/gntdev: Do not destroy context while dma-bufs are in use

[Boris Ostrovsky]

On 2/15/19 10:07 AM, Oleksandr Andrushchenko wrote:
> On 2/15/19 5:03 PM, Boris Ostrovsky wrote:
>> On 2/14/19 9:23 AM, Oleksandr Andrushchenko wrote:
>>>     /* DMA buffer export support. */
>>> @@ -311,6 +317,7 @@ static void dmabuf_exp_release(struct kref *kref)
>>>         dmabuf_exp_wait_obj_signal(gntdev_dmabuf->priv, gntdev_dmabuf);
>>>       list_del(&gntdev_dmabuf->next);
>>> +    fput(gntdev_dmabuf->priv->filp);
>>>       kfree(gntdev_dmabuf);
>>>   }
>>>   @@ -423,6 +430,7 @@ static int dmabuf_exp_from_pages(struct
>>> gntdev_dmabuf_export_args *args)
>>>       mutex_lock(&args->dmabuf_priv->lock);
>>>       list_add(&gntdev_dmabuf->next, &args->dmabuf_priv->exp_list);
>>>       mutex_unlock(&args->dmabuf_priv->lock);
>>> +    get_file(gntdev_dmabuf->priv->filp);
>> Not fget()?
> fget wants file descriptor [1] and returns struct file *,
> but we already have struct file*, so I use get_file [2]
> which does what I need - increments the reference counter
> on the file


Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Re: [Xen-devel][PATCH 1/2] xen/gntdev: Do not destroy context while dma-bufs are in use

[Oleksandr Andrushchenko]

On 2/15/19 5:28 PM, Boris Ostrovsky wrote:
> On 2/15/19 10:07 AM, Oleksandr Andrushchenko wrote:
>> On 2/15/19 5:03 PM, Boris Ostrovsky wrote:
>>> On 2/14/19 9:23 AM, Oleksandr Andrushchenko wrote:
>>>>      /* DMA buffer export support. */
>>>> @@ -311,6 +317,7 @@ static void dmabuf_exp_release(struct kref *kref)
>>>>          dmabuf_exp_wait_obj_signal(gntdev_dmabuf->priv, gntdev_dmabuf);
>>>>        list_del(&gntdev_dmabuf->next);
>>>> +    fput(gntdev_dmabuf->priv->filp);
>>>>        kfree(gntdev_dmabuf);
>>>>    }
>>>>    @@ -423,6 +430,7 @@ static int dmabuf_exp_from_pages(struct
>>>> gntdev_dmabuf_export_args *args)
>>>>        mutex_lock(&args->dmabuf_priv->lock);
>>>>        list_add(&gntdev_dmabuf->next, &args->dmabuf_priv->exp_list);
>>>>        mutex_unlock(&args->dmabuf_priv->lock);
>>>> +    get_file(gntdev_dmabuf->priv->filp);
>>> Not fget()?
>> fget wants file descriptor [1] and returns struct file *,
>> but we already have struct file*, so I use get_file [2]
>> which does what I need - increments the reference counter
>> on the file
>
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Thank you,
any chance we can get this for 5.1?

Re: [Xen-devel][PATCH 1/2] xen/gntdev: Do not destroy context while dma-bufs are in use

[Juergen Gross]

On 15/02/2019 16:35, Oleksandr Andrushchenko wrote:
> On 2/15/19 5:28 PM, Boris Ostrovsky wrote:
>> On 2/15/19 10:07 AM, Oleksandr Andrushchenko wrote:
>>> On 2/15/19 5:03 PM, Boris Ostrovsky wrote:
>>>> On 2/14/19 9:23 AM, Oleksandr Andrushchenko wrote:
>>>>>      /* DMA buffer export support. */
>>>>> @@ -311,6 +317,7 @@ static void dmabuf_exp_release(struct kref *kref)
>>>>>          dmabuf_exp_wait_obj_signal(gntdev_dmabuf->priv,
>>>>> gntdev_dmabuf);
>>>>>        list_del(&gntdev_dmabuf->next);
>>>>> +    fput(gntdev_dmabuf->priv->filp);
>>>>>        kfree(gntdev_dmabuf);
>>>>>    }
>>>>>    @@ -423,6 +430,7 @@ static int dmabuf_exp_from_pages(struct
>>>>> gntdev_dmabuf_export_args *args)
>>>>>        mutex_lock(&args->dmabuf_priv->lock);
>>>>>        list_add(&gntdev_dmabuf->next, &args->dmabuf_priv->exp_list);
>>>>>        mutex_unlock(&args->dmabuf_priv->lock);
>>>>> +    get_file(gntdev_dmabuf->priv->filp);
>>>> Not fget()?
>>> fget wants file descriptor [1] and returns struct file *,
>>> but we already have struct file*, so I use get_file [2]
>>> which does what I need - increments the reference counter
>>> on the file
>>
>> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Thank you,
> any chance we can get this for 5.1?
> 

Yes.


Juergen

Re: [Xen-devel][PATCH 1/2] xen/gntdev: Do not destroy context while dma-bufs are in use

[Juergen Gross]

On 14/02/2019 15:23, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> If there are exported DMA buffers which are still in use and
> grant device is closed by either normal user-space close or by
> a signal this leads to the grant device context to be destroyed,
> thus making it not possible to correctly destroy those exported
> buffers when they are returned back to gntdev and makes the module
> crash:
> 
> [  339.617540] [<ffff00000854c0d8>] dmabuf_exp_ops_release+0x40/0xa8
> [  339.617560] [<ffff00000867a6e8>] dma_buf_release+0x60/0x190
> [  339.617577] [<ffff0000082211f0>] __fput+0x88/0x1d0
> [  339.617589] [<ffff000008221394>] ____fput+0xc/0x18
> [  339.617607] [<ffff0000080ed4e4>] task_work_run+0x9c/0xc0
> [  339.617622] [<ffff000008089714>] do_notify_resume+0xfc/0x108
> 
> Fix this by referencing gntdev on each DMA buffer export and
> unreferencing on buffer release.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Applied to xen/tip.git for-linus-5.1


Juergen