Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/5] misc: fastrpc: few fixes
@ 2019-08-23 10:06 Srinivas Kandagatla
  2019-08-23 10:06 ` [PATCH 1/5] misc: fastrpc: Reference count channel context Srinivas Kandagatla
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Srinivas Kandagatla @ 2019-08-23 10:06 UTC (permalink / raw)
  To: gregkh; +Cc: arnd, linux-arm-msm, linux-kernel, Srinivas Kandagatla

Hi Greg,

More testing on fastprc revealed few memory leaks in driver
and few corner cases.
These patches are the fixes for those cases.
One patch from Jorge is to remove unsed definition.


Thanks,
srini

Bjorn Andersson (2):
  misc: fastrpc: Reference count channel context
  misc: fastrpc: Don't reference rpmsg_device after remove

Jorge Ramirez-Ortiz (1):
  misc: fastrpc: remove unused definition

Srinivas Kandagatla (2):
  misc: fastrpc: fix double refcounting on dmabuf
  misc: fastrpc: free dma buf scatter list

 drivers/misc/fastrpc.c | 74 ++++++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 31 deletions(-)

-- 
2.21.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/5] misc: fastrpc: Reference count channel context
  2019-08-23 10:06 [PATCH 0/5] misc: fastrpc: few fixes Srinivas Kandagatla
@ 2019-08-23 10:06 ` Srinivas Kandagatla
  2019-08-28 20:53   ` Greg KH
  2019-08-23 10:06 ` [PATCH 2/5] misc: fastrpc: Don't reference rpmsg_device after remove Srinivas Kandagatla
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Srinivas Kandagatla @ 2019-08-23 10:06 UTC (permalink / raw)
  To: gregkh
  Cc: arnd, linux-arm-msm, linux-kernel, Bjorn Andersson,
	Mayank Chopra, Abhinav Asati, Vamsi Singamsetty,
	Srinivas Kandagatla

From: Bjorn Andersson <bjorn.andersson@linaro.org>

The channel context is referenced from the fastrpc user and might as
user space holds the file descriptor open outlive the fastrpc device,
which is removed when the remote processor is shutting down.

Reference count the channel context in order to retain this object until
all references has been relinquished.

TEST=stop and start remote proc1 using sysfs

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Mayank Chopra <mak.chopra@codeaurora.org>
Signed-off-by: Abhinav Asati <asatiabhi@codeaurora.org>
Signed-off-by: Vamsi Singamsetty <vamssi@codeaurora.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/misc/fastrpc.c | 43 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 38 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index c790585da14c..c019e867e7fa 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -186,6 +186,7 @@ struct fastrpc_channel_ctx {
 	struct idr ctx_idr;
 	struct list_head users;
 	struct miscdevice miscdev;
+	struct kref refcount;
 };
 
 struct fastrpc_user {
@@ -293,6 +294,25 @@ static int fastrpc_buf_alloc(struct fastrpc_user *fl, struct device *dev,
 	return 0;
 }
 
+static void fastrpc_channel_ctx_free(struct kref *ref)
+{
+	struct fastrpc_channel_ctx *cctx;
+
+	cctx = container_of(ref, struct fastrpc_channel_ctx, refcount);
+
+	kfree(cctx);
+}
+
+static void fastrpc_channel_ctx_get(struct fastrpc_channel_ctx *cctx)
+{
+	kref_get(&cctx->refcount);
+}
+
+static void fastrpc_channel_ctx_put(struct fastrpc_channel_ctx *cctx)
+{
+	kref_put(&cctx->refcount, fastrpc_channel_ctx_free);
+}
+
 static void fastrpc_context_free(struct kref *ref)
 {
 	struct fastrpc_invoke_ctx *ctx;
@@ -316,6 +336,8 @@ static void fastrpc_context_free(struct kref *ref)
 	kfree(ctx->maps);
 	kfree(ctx->olaps);
 	kfree(ctx);
+
+	fastrpc_channel_ctx_put(cctx);
 }
 
 static void fastrpc_context_get(struct fastrpc_invoke_ctx *ctx)
@@ -422,6 +444,9 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
 		fastrpc_get_buff_overlaps(ctx);
 	}
 
+	/* Released in fastrpc_context_put() */
+	fastrpc_channel_ctx_get(cctx);
+
 	ctx->sc = sc;
 	ctx->retval = -1;
 	ctx->pid = current->pid;
@@ -451,6 +476,7 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
 	spin_lock(&user->lock);
 	list_del(&ctx->node);
 	spin_unlock(&user->lock);
+	fastrpc_channel_ctx_put(cctx);
 	kfree(ctx->maps);
 	kfree(ctx->olaps);
 	kfree(ctx);
@@ -1123,6 +1149,7 @@ static int fastrpc_device_release(struct inode *inode, struct file *file)
 	}
 
 	fastrpc_session_free(cctx, fl->sctx);
+	fastrpc_channel_ctx_put(cctx);
 
 	mutex_destroy(&fl->mutex);
 	kfree(fl);
@@ -1141,6 +1168,9 @@ static int fastrpc_device_open(struct inode *inode, struct file *filp)
 	if (!fl)
 		return -ENOMEM;
 
+	/* Released in fastrpc_device_release() */
+	fastrpc_channel_ctx_get(cctx);
+
 	filp->private_data = fl;
 	spin_lock_init(&fl->lock);
 	mutex_init(&fl->mutex);
@@ -1398,10 +1428,6 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 	int i, err, domain_id = -1;
 	const char *domain;
 
-	data = devm_kzalloc(rdev, sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
 	err = of_property_read_string(rdev->of_node, "label", &domain);
 	if (err) {
 		dev_info(rdev, "FastRPC Domain not specified in DT\n");
@@ -1420,6 +1446,10 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 		return -EINVAL;
 	}
 
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
 	data->miscdev.minor = MISC_DYNAMIC_MINOR;
 	data->miscdev.name = kasprintf(GFP_KERNEL, "fastrpc-%s",
 				domains[domain_id]);
@@ -1428,6 +1458,8 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 	if (err)
 		return err;
 
+	kref_init(&data->refcount);
+
 	dev_set_drvdata(&rpdev->dev, data);
 	dma_set_mask_and_coherent(rdev, DMA_BIT_MASK(32));
 	INIT_LIST_HEAD(&data->users);
@@ -1462,7 +1494,8 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
 
 	misc_deregister(&cctx->miscdev);
 	of_platform_depopulate(&rpdev->dev);
-	kfree(cctx);
+
+	fastrpc_channel_ctx_put(cctx);
 }
 
 static int fastrpc_rpmsg_callback(struct rpmsg_device *rpdev, void *data,
-- 
2.21.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 2/5] misc: fastrpc: Don't reference rpmsg_device after remove
  2019-08-23 10:06 [PATCH 0/5] misc: fastrpc: few fixes Srinivas Kandagatla
  2019-08-23 10:06 ` [PATCH 1/5] misc: fastrpc: Reference count channel context Srinivas Kandagatla
@ 2019-08-23 10:06 ` Srinivas Kandagatla
  2019-08-28 15:41   ` Stephen Boyd
  2019-08-23 10:06 ` [PATCH 3/5] misc: fastrpc: remove unused definition Srinivas Kandagatla
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Srinivas Kandagatla @ 2019-08-23 10:06 UTC (permalink / raw)
  To: gregkh
  Cc: arnd, linux-arm-msm, linux-kernel, Bjorn Andersson,
	Mayank Chopra, Abhinav Asati, Vamsi Singamsetty,
	Srinivas Kandagatla

From: Bjorn Andersson <bjorn.andersson@linaro.org>

As fastrpc_rpmsg_remove() returns the rpdev of the channel context is no
longer a valid object, so ensure to update the channel context to no
longer reference the old object and guard in the invoke code path
against dereferencing it.

TEST=stop and start remote proc1 using sysfs

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Mayank Chopra <mak.chopra@codeaurora.org>
Signed-off-by: Abhinav Asati <asatiabhi@codeaurora.org>
Signed-off-by: Vamsi Singamsetty <vamssi@codeaurora.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/misc/fastrpc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index c019e867e7fa..59ee6de26229 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -913,6 +913,9 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
 	if (!fl->sctx)
 		return -EINVAL;
 
+	if (!fl->cctx->rpdev)
+		return -EPIPE;
+
 	ctx = fastrpc_context_alloc(fl, kernel, sc, args);
 	if (IS_ERR(ctx))
 		return PTR_ERR(ctx);
@@ -1495,6 +1498,7 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
 	misc_deregister(&cctx->miscdev);
 	of_platform_depopulate(&rpdev->dev);
 
+	cctx->rpdev = NULL;
 	fastrpc_channel_ctx_put(cctx);
 }
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 3/5] misc: fastrpc: remove unused definition
  2019-08-23 10:06 [PATCH 0/5] misc: fastrpc: few fixes Srinivas Kandagatla
  2019-08-23 10:06 ` [PATCH 1/5] misc: fastrpc: Reference count channel context Srinivas Kandagatla
  2019-08-23 10:06 ` [PATCH 2/5] misc: fastrpc: Don't reference rpmsg_device after remove Srinivas Kandagatla
@ 2019-08-23 10:06 ` Srinivas Kandagatla
  2019-08-23 10:06 ` [PATCH 4/5] misc: fastrpc: fix double refcounting on dmabuf Srinivas Kandagatla
  2019-08-23 10:06 ` [PATCH 5/5] misc: fastrpc: free dma buf scatter list Srinivas Kandagatla
  4 siblings, 0 replies; 12+ messages in thread
From: Srinivas Kandagatla @ 2019-08-23 10:06 UTC (permalink / raw)
  To: gregkh
  Cc: arnd, linux-arm-msm, linux-kernel, Jorge Ramirez-Ortiz,
	Abhinav Asati, Vamsi Singamsetty, Srinivas Kandagatla

From: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>

Remove unused INIT_MEMLEN_MAX define.

Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
Signed-off-by: Abhinav Asati <asatiabhi@codeaurora.org>
Signed-off-by: Vamsi Singamsetty <vamssi@codeaurora.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/misc/fastrpc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 59ee6de26229..38829fa74f28 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -33,7 +33,6 @@
 #define FASTRPC_INIT_HANDLE	1
 #define FASTRPC_CTXID_MASK (0xFF0)
 #define INIT_FILELEN_MAX (64 * 1024 * 1024)
-#define INIT_MEMLEN_MAX  (8 * 1024 * 1024)
 #define FASTRPC_DEVICE_NAME	"fastrpc"
 
 /* Retrives number of input buffers from the scalars parameter */
-- 
2.21.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 4/5] misc: fastrpc: fix double refcounting on dmabuf
  2019-08-23 10:06 [PATCH 0/5] misc: fastrpc: few fixes Srinivas Kandagatla
                   ` (2 preceding siblings ...)
  2019-08-23 10:06 ` [PATCH 3/5] misc: fastrpc: remove unused definition Srinivas Kandagatla
@ 2019-08-23 10:06 ` Srinivas Kandagatla
       [not found]   ` <CAC8LzUAnz+RZYh+bBbJbXJYP3QDq4H1847W8rJxj-aF1B1J9QQ@mail.gmail.com>
  2019-08-23 10:06 ` [PATCH 5/5] misc: fastrpc: free dma buf scatter list Srinivas Kandagatla
  4 siblings, 1 reply; 12+ messages in thread
From: Srinivas Kandagatla @ 2019-08-23 10:06 UTC (permalink / raw)
  To: gregkh
  Cc: arnd, linux-arm-msm, linux-kernel, Srinivas Kandagatla,
	Mayank Chopra, Jorge Ramirez-Ortiz

dma buf refcount has to be done by the driver which is going to use the fd.
This driver already does refcount on the dmabuf fd if its actively using it
but also does an additional refcounting via extra ioctl.
This additional refcount can lead to memory leak in cases where the
applications fail to call the ioctl to decrement the refcount.

So remove this extra refcount in the ioctl

More info of dma buf usage at drivers/dma-buf/dma-buf.c

Reported-by: Mayank Chopra <mak.chopra@codeaurora.org>
Reported-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/misc/fastrpc.c | 25 -------------------------
 1 file changed, 25 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 38829fa74f28..eee2bb398947 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1198,26 +1198,6 @@ static int fastrpc_device_open(struct inode *inode, struct file *filp)
 	return 0;
 }
 
-static int fastrpc_dmabuf_free(struct fastrpc_user *fl, char __user *argp)
-{
-	struct dma_buf *buf;
-	int info;
-
-	if (copy_from_user(&info, argp, sizeof(info)))
-		return -EFAULT;
-
-	buf = dma_buf_get(info);
-	if (IS_ERR_OR_NULL(buf))
-		return -EINVAL;
-	/*
-	 * one for the last get and other for the ALLOC_DMA_BUFF ioctl
-	 */
-	dma_buf_put(buf);
-	dma_buf_put(buf);
-
-	return 0;
-}
-
 static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char __user *argp)
 {
 	struct fastrpc_alloc_dma_buf bp;
@@ -1253,8 +1233,6 @@ static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char __user *argp)
 		return -EFAULT;
 	}
 
-	get_dma_buf(buf->dmabuf);
-
 	return 0;
 }
 
@@ -1322,9 +1300,6 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
 	case FASTRPC_IOCTL_INIT_CREATE:
 		err = fastrpc_init_create_process(fl, argp);
 		break;
-	case FASTRPC_IOCTL_FREE_DMA_BUFF:
-		err = fastrpc_dmabuf_free(fl, argp);
-		break;
 	case FASTRPC_IOCTL_ALLOC_DMA_BUFF:
 		err = fastrpc_dmabuf_alloc(fl, argp);
 		break;
-- 
2.21.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 5/5] misc: fastrpc: free dma buf scatter list
  2019-08-23 10:06 [PATCH 0/5] misc: fastrpc: few fixes Srinivas Kandagatla
                   ` (3 preceding siblings ...)
  2019-08-23 10:06 ` [PATCH 4/5] misc: fastrpc: fix double refcounting on dmabuf Srinivas Kandagatla
@ 2019-08-23 10:06 ` Srinivas Kandagatla
  4 siblings, 0 replies; 12+ messages in thread
From: Srinivas Kandagatla @ 2019-08-23 10:06 UTC (permalink / raw)
  To: gregkh
  Cc: arnd, linux-arm-msm, linux-kernel, Srinivas Kandagatla, Mayank Chopra

dma buf scatter list is never freed, free it!

Orignally detected by kmemleak:
  backtrace:
    [<ffffff80088b7658>] kmemleak_alloc+0x50/0x84
    [<ffffff8008373284>] sg_kmalloc+0x38/0x60
    [<ffffff8008373144>] __sg_alloc_table+0x60/0x110
    [<ffffff800837321c>] sg_alloc_table+0x28/0x58
    [<ffffff800837336c>] __sg_alloc_table_from_pages+0xc0/0x1ac
    [<ffffff800837346c>] sg_alloc_table_from_pages+0x14/0x1c
    [<ffffff8008097a3c>] __iommu_get_sgtable+0x5c/0x8c
    [<ffffff800850a1d0>] fastrpc_dma_buf_attach+0x84/0xf8
    [<ffffff80085114bc>] dma_buf_attach+0x70/0xc8
    [<ffffff8008509efc>] fastrpc_map_create+0xf8/0x1e8
    [<ffffff80085086f4>] fastrpc_device_ioctl+0x508/0x900
    [<ffffff80082428c8>] compat_SyS_ioctl+0x128/0x200
    [<ffffff80080832c4>] el0_svc_naked+0x34/0x38
    [<ffffffffffffffff>] 0xffffffffffffffff

Reported-by: Mayank Chopra <mak.chopra@codeaurora.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/misc/fastrpc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index eee2bb398947..47ae84afac2e 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -550,6 +550,7 @@ static void fastrpc_dma_buf_detatch(struct dma_buf *dmabuf,
 	mutex_lock(&buffer->lock);
 	list_del(&a->node);
 	mutex_unlock(&buffer->lock);
+	sg_free_table(&a->sgt);
 	kfree(a);
 }
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/5] misc: fastrpc: fix double refcounting on dmabuf
       [not found]   ` <CAC8LzUAnz+RZYh+bBbJbXJYP3QDq4H1847W8rJxj-aF1B1J9QQ@mail.gmail.com>
@ 2019-08-27 21:45     ` Srinivas Kandagatla
  2019-08-28  7:50       ` Jorge Ramirez
  0 siblings, 1 reply; 12+ messages in thread
From: Srinivas Kandagatla @ 2019-08-27 21:45 UTC (permalink / raw)
  To: Jorge Ramirez-Ortiz
  Cc: Greg KH, arnd, linux-arm-msm, Linux Kernel Mailing List, Mayank Chopra


On 23/08/2019 16:23, Jorge Ramirez-Ortiz wrote:
> can you add me as a co-author to this patch please?

No problem I can do that if you feel so!

> since I spent about a day doing the analysis, sent you  a fix that 
> maintained the API used by the library and explained you how to 
> reproduce the issue I think it is just fair. > the  fact that the api could be be modified and the fix be done a bit
> differently- free dma buf ioctl removed- seems just a minor 
> implementation detail to me.

No, that's not true, this is a clear fastrpc design issue.

Userspace is already doing a refcount via mmap/unmap on that dmabuf fd, 
having an additional api adds another level of refcount which is totally 
redundant and is the root cause for this leak.


--srini

> 
> also you can add my tested-by if you want
> 
> TIA
> 
> On Fri, 23 Aug 2019 at 12:07, Srinivas Kandagatla 
> <srinivas.kandagatla@linaro.org <mailto:srinivas.kandagatla@linaro.org>> 
> wrote:
> 
>     dma buf refcount has to be done by the driver which is going to use
>     the fd.
>     This driver already does refcount on the dmabuf fd if its actively
>     using it
>     but also does an additional refcounting via extra ioctl.
>     This additional refcount can lead to memory leak in cases where the
>     applications fail to call the ioctl to decrement the refcount.
> 
>     So remove this extra refcount in the ioctl
> 
>     More info of dma buf usage at drivers/dma-buf/dma-buf.c
> 
>     Reported-by: Mayank Chopra <mak.chopra@codeaurora.org
>     <mailto:mak.chopra@codeaurora.org>>
>     Reported-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org
>     <mailto:jorge.ramirez-ortiz@linaro.org>>
>     Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org
>     <mailto:srinivas.kandagatla@linaro.org>>
>     ---
>       drivers/misc/fastrpc.c | 25 -------------------------
>       1 file changed, 25 deletions(-)
> 
>     diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>     index 38829fa74f28..eee2bb398947 100644
>     --- a/drivers/misc/fastrpc.c
>     +++ b/drivers/misc/fastrpc.c
>     @@ -1198,26 +1198,6 @@ static int fastrpc_device_open(struct inode
>     *inode, struct file *filp)
>              return 0;
>       }
> 
>     -static int fastrpc_dmabuf_free(struct fastrpc_user *fl, char __user
>     *argp)
>     -{
>     -       struct dma_buf *buf;
>     -       int info;
>     -
>     -       if (copy_from_user(&info, argp, sizeof(info)))
>     -               return -EFAULT;
>     -
>     -       buf = dma_buf_get(info);
>     -       if (IS_ERR_OR_NULL(buf))
>     -               return -EINVAL;
>     -       /*
>     -        * one for the last get and other for the ALLOC_DMA_BUFF ioctl
>     -        */
>     -       dma_buf_put(buf);
>     -       dma_buf_put(buf);
>     -
>     -       return 0;
>     -}
>     -
>       static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char
>     __user *argp)
>       {
>              struct fastrpc_alloc_dma_buf bp;
>     @@ -1253,8 +1233,6 @@ static int fastrpc_dmabuf_alloc(struct
>     fastrpc_user *fl, char __user *argp)
>                      return -EFAULT;
>              }
> 
>     -       get_dma_buf(buf->dmabuf);
>     -
>              return 0;
>       }
> 
>     @@ -1322,9 +1300,6 @@ static long fastrpc_device_ioctl(struct file
>     *file, unsigned int cmd,
>              case FASTRPC_IOCTL_INIT_CREATE:
>                      err = fastrpc_init_create_process(fl, argp);
>                      break;
>     -       case FASTRPC_IOCTL_FREE_DMA_BUFF:
>     -               err = fastrpc_dmabuf_free(fl, argp);
>     -               break;
>              case FASTRPC_IOCTL_ALLOC_DMA_BUFF:
>                      err = fastrpc_dmabuf_alloc(fl, argp);
>                      break;
>     -- 
>     2.21.0
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/5] misc: fastrpc: fix double refcounting on dmabuf
  2019-08-27 21:45     ` Srinivas Kandagatla
@ 2019-08-28  7:50       ` Jorge Ramirez
  2019-08-28  8:48         ` Srinivas Kandagatla
  0 siblings, 1 reply; 12+ messages in thread
From: Jorge Ramirez @ 2019-08-28  7:50 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Greg KH, arnd, linux-arm-msm, Linux Kernel Mailing List, Mayank Chopra

On 8/27/19 23:45, Srinivas Kandagatla wrote:
> 
> On 23/08/2019 16:23, Jorge Ramirez-Ortiz wrote:
>> can you add me as a co-author to this patch please?
> 
> No problem I can do that if you feel so!

yes please. thanks!

> 
>> since I spent about a day doing the analysis, sent you  a fix that
>> maintained the API used by the library and explained you how to
>> reproduce the issue I think it is just fair. > the  fact that the api
>> could be be modified and the fix be done a bit
>> differently- free dma buf ioctl removed- seems just a minor
>> implementation detail to me.
> 
> No, that's not true, this is a clear fastrpc design issue.

IMO the ioctls defines the contract with userspace and the contract
establishes that userspace must call deallocate. the kernel wrongly
implemented to that contract since it doesn't handle the cases where
userspace can't send the release calls which leads to memory leaks. this
is what I meant by and implementation issue.

if we had many fastrpc users, rolling out the design change that you
propose - removing an ioctl- would definitively have an impact. But
since that is not yet the case, there is not doubt that your patch makes
more sense.

but my point was that there is not a huge gap in efforts between doing
one or the other.

> 
> Userspace is already doing a refcount via mmap/unmap on that dmabuf fd,
> having an additional api adds another level of refcount which is totally
> redundant and is the root cause for this leak.

yes it is redundant but is not the root cause for this leak. the root
cause is that the driver doesnt handle the case where userspace didnt or
was not able to call release (and that is no more than adding allocated
buffers to a list and clean on exit)

> 
> 
> --srini


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/5] misc: fastrpc: fix double refcounting on dmabuf
  2019-08-28  7:50       ` Jorge Ramirez
@ 2019-08-28  8:48         ` Srinivas Kandagatla
  2019-08-28 10:35           ` Jorge Ramirez
  0 siblings, 1 reply; 12+ messages in thread
From: Srinivas Kandagatla @ 2019-08-28  8:48 UTC (permalink / raw)
  To: Jorge Ramirez
  Cc: Greg KH, arnd, linux-arm-msm, Linux Kernel Mailing List, Mayank Chopra



On 28/08/2019 08:50, Jorge Ramirez wrote:
> On 8/27/19 23:45, Srinivas Kandagatla wrote:
>> On 23/08/2019 16:23, Jorge Ramirez-Ortiz wrote:
>>> can you add me as a co-author to this patch please?
>> No problem I can do that if you feel so!
> yes please. thanks!
> 
>>> since I spent about a day doing the analysis, sent you  a fix that
>>> maintained the API used by the library and explained you how to
>>> reproduce the issue I think it is just fair. > the  fact that the api
>>> could be be modified and the fix be done a bit
>>> differently- free dma buf ioctl removed- seems just a minor
>>> implementation detail to me.
>> No, that's not true, this is a clear fastrpc design issue.
> IMO the ioctls defines the contract with userspace and the contract
> establishes that userspace must call deallocate. the kernel wrongly
> implemented to that contract since it doesn't handle the cases where
> userspace can't send the release calls which leads to memory leaks. this
> is what I meant by and implementation issue.
> 
> if we had many fastrpc users, rolling out the design change that you
> propose - removing an ioctl- would definitively have an impact. But
> since that is not yet the case, there is not doubt that your patch makes
> more sense.

Exactly before it make a way into other projects!

> 
> but my point was that there is not a huge gap in efforts between doing
> one or the other.

Thats not the point, point is about right fix!

> 
>> Userspace is already doing a refcount via mmap/unmap on that dmabuf fd,
>> having an additional api adds another level of refcount which is totally
>> redundant and is the root cause for this leak.
> yes it is redundant but is not the root cause for this leak. the root
> cause is that the driver doesnt handle the case where userspace didnt or
> was not able to call release (and that is no more than adding allocated
> buffers to a list and clean on exit)

I don't agree with you on that. We should not take an extra refcount in 
first place in driver.

let me explain it one more time!

dmabuf has to be mmaped in userspace app before it is used, and
"Memory mappings that were created in the process shall be unmapped 
before the process is destroyed" so the refcount is taken care by 
mmap/unmap automatically.

--srini


> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/5] misc: fastrpc: fix double refcounting on dmabuf
  2019-08-28  8:48         ` Srinivas Kandagatla
@ 2019-08-28 10:35           ` Jorge Ramirez
  0 siblings, 0 replies; 12+ messages in thread
From: Jorge Ramirez @ 2019-08-28 10:35 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Greg KH, arnd, linux-arm-msm, Linux Kernel Mailing List, Mayank Chopra

On 8/28/19 10:48, Srinivas Kandagatla wrote:
> 
> 
> On 28/08/2019 08:50, Jorge Ramirez wrote:
>> On 8/27/19 23:45, Srinivas Kandagatla wrote:
>>> On 23/08/2019 16:23, Jorge Ramirez-Ortiz wrote:
>>>> can you add me as a co-author to this patch please?
>>> No problem I can do that if you feel so!
>> yes please. thanks!
>>
>>>> since I spent about a day doing the analysis, sent you  a fix that
>>>> maintained the API used by the library and explained you how to
>>>> reproduce the issue I think it is just fair. > the  fact that the api
>>>> could be be modified and the fix be done a bit
>>>> differently- free dma buf ioctl removed- seems just a minor
>>>> implementation detail to me.
>>> No, that's not true, this is a clear fastrpc design issue.
>> IMO the ioctls defines the contract with userspace and the contract
>> establishes that userspace must call deallocate. the kernel wrongly
>> implemented to that contract since it doesn't handle the cases where
>> userspace can't send the release calls which leads to memory leaks. this
>> is what I meant by and implementation issue.
>>
>> if we had many fastrpc users, rolling out the design change that you
>> propose - removing an ioctl- would definitively have an impact. But
>> since that is not yet the case, there is not doubt that your patch makes
>> more sense.
> 
> Exactly before it make a way into other projects!
> 
>>
>> but my point was that there is not a huge gap in efforts between doing
>> one or the other.
> 
> Thats not the point, point is about right fix!

but I disagree with you about what 'right' means.

in this context, for you "right" meant potentially breaking some users
and implement the best possible kernel design.

for me, it meant continue to obey at the agreed ioctl interface to not
disturb the users.

but as I said, since there is not a significant pool of fastrpc users,
breaking backward compatibility with the fastrpc library is not
important hence why I agree that removing the ioctl was the better
choice (on this particular case).

> 
>>
>>> Userspace is already doing a refcount via mmap/unmap on that dmabuf fd,
>>> having an additional api adds another level of refcount which is totally
>>> redundant and is the root cause for this leak.
>> yes it is redundant but is not the root cause for this leak. the root
>> cause is that the driver doesnt handle the case where userspace didnt or
>> was not able to call release (and that is no more than adding allocated
>> buffers to a list and clean on exit)
> 
> I don't agree with you on that. We should not take an extra refcount in
> first place in driver.

of course taking an extra refcount is functionally pointless. But that
was a design decision that imposed something on the user. and the kernel
can certainly work with that 'silly' design decision by tracking the
memory in the driver.

is the right thing to do to keep less than ideal designs in the kernel
to support agreed user interface? IMO it depends on the use case

to my eyes the design was obviously wrong, I never questioned
that...that was very clear when I started tracing the code.

perhaps, rather than work around it, I should have considered that
removing the ioctl wouldnt be a big deal to anyone.

so I would have send two patches instead of the one I sent you

1. fix leak (keep track of allocated dma buffers and make sure
everything is released on exit)

2. remove unnecessary ioctl warning users.


> 
> let me explain it one more time!

cmon, I did understand it before we engaged in this discussion :)



> 
> dmabuf has to be mmaped in userspace app before it is used, and
> "Memory mappings that were created in the process shall be unmapped
> before the process is destroyed" so the refcount is taken care by
> mmap/unmap automatically.


I would like to leave the discussion here if that is ok with you (I
clearly understand your POV but I feel I am not doing a good job at
sharing my thoughts...we can do that offline if you want)


> 
> --srini
> 
> 
>>
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/5] misc: fastrpc: Don't reference rpmsg_device after remove
  2019-08-23 10:06 ` [PATCH 2/5] misc: fastrpc: Don't reference rpmsg_device after remove Srinivas Kandagatla
@ 2019-08-28 15:41   ` Stephen Boyd
  0 siblings, 0 replies; 12+ messages in thread
From: Stephen Boyd @ 2019-08-28 15:41 UTC (permalink / raw)
  To: Srinivas Kandagatla, gregkh
  Cc: arnd, linux-arm-msm, linux-kernel, Bjorn Andersson,
	Mayank Chopra, Abhinav Asati, Vamsi Singamsetty,
	Srinivas Kandagatla

Quoting Srinivas Kandagatla (2019-08-23 03:06:19)
> From: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> As fastrpc_rpmsg_remove() returns the rpdev of the channel context is no
> longer a valid object, so ensure to update the channel context to no
> longer reference the old object and guard in the invoke code path
> against dereferencing it.
> 
> TEST=stop and start remote proc1 using sysfs

Remove this line? Looks like a chromiumism.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/5] misc: fastrpc: Reference count channel context
  2019-08-23 10:06 ` [PATCH 1/5] misc: fastrpc: Reference count channel context Srinivas Kandagatla
@ 2019-08-28 20:53   ` Greg KH
  0 siblings, 0 replies; 12+ messages in thread
From: Greg KH @ 2019-08-28 20:53 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: arnd, linux-arm-msm, linux-kernel, Bjorn Andersson,
	Mayank Chopra, Abhinav Asati, Vamsi Singamsetty

On Fri, Aug 23, 2019 at 11:06:18AM +0100, Srinivas Kandagatla wrote:
> From: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> The channel context is referenced from the fastrpc user and might as
> user space holds the file descriptor open outlive the fastrpc device,
> which is removed when the remote processor is shutting down.
> 
> Reference count the channel context in order to retain this object until
> all references has been relinquished.
> 
> TEST=stop and start remote proc1 using sysfs

What is this for?

Please fix up the series and resend so that I do not have to hand-edit
the changelog text.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, back to index

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-23 10:06 [PATCH 0/5] misc: fastrpc: few fixes Srinivas Kandagatla
2019-08-23 10:06 ` [PATCH 1/5] misc: fastrpc: Reference count channel context Srinivas Kandagatla
2019-08-28 20:53   ` Greg KH
2019-08-23 10:06 ` [PATCH 2/5] misc: fastrpc: Don't reference rpmsg_device after remove Srinivas Kandagatla
2019-08-28 15:41   ` Stephen Boyd
2019-08-23 10:06 ` [PATCH 3/5] misc: fastrpc: remove unused definition Srinivas Kandagatla
2019-08-23 10:06 ` [PATCH 4/5] misc: fastrpc: fix double refcounting on dmabuf Srinivas Kandagatla
     [not found]   ` <CAC8LzUAnz+RZYh+bBbJbXJYP3QDq4H1847W8rJxj-aF1B1J9QQ@mail.gmail.com>
2019-08-27 21:45     ` Srinivas Kandagatla
2019-08-28  7:50       ` Jorge Ramirez
2019-08-28  8:48         ` Srinivas Kandagatla
2019-08-28 10:35           ` Jorge Ramirez
2019-08-23 10:06 ` [PATCH 5/5] misc: fastrpc: free dma buf scatter list Srinivas Kandagatla

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org linux-arm-msm@archiver.kernel.org
	public-inbox-index linux-arm-msm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox