Messages in this thread | | | From | Srinivas Kandagatla <> | Subject | Re: [PATCH 4/5] misc: fastrpc: fix double refcounting on dmabuf | Date | Tue, 27 Aug 2019 22:45:46 +0100 |
| |
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 >
| |