| Message ID | 20250513042825.2147985-6-ekansh.gupta@oss.qualcomm.com |
|---|---|
| State | New |
| Headers | show |
| Series | misc: fastrpc: Add missing bug fixes | expand |
On Mon, May 19, 2025 at 04:28:34PM +0530, Ekansh Gupta wrote: > > > On 5/19/2025 4:22 PM, Dmitry Baryshkov wrote: > > On Tue, May 13, 2025 at 09:58:25AM +0530, Ekansh Gupta wrote: > >> User request for remote heap allocation is supported using ioctl > >> interface but support for unmap is missing. This could result in > >> memory leak issues. Add unmap user request support for remote heap. > > Can this memory be in use by the remote proc? > Remote heap allocation request is only intended for audioPD. Other PDs > running on DSP are not intended to use this request. 'Intended'. That's fine. I asked a different question: _can_ it be in use? What happens if userspace by mistake tries to unmap memory too early? Or if it happens intentionally, at some specific time during work. > > > >> Fixes: 532ad70c6d449 ("misc: fastrpc: Add mmap request assigning for static PD pool") > >> Cc: stable@kernel.org > >> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com> > >> --- > >> drivers/misc/fastrpc.c | 62 ++++++++++++++++++++++++++++++++++-------- > >> 1 file changed, 51 insertions(+), 11 deletions(-) > >> >
On 5/19/2025 7:04 PM, Dmitry Baryshkov wrote: > On Mon, May 19, 2025 at 04:28:34PM +0530, Ekansh Gupta wrote: >> >> On 5/19/2025 4:22 PM, Dmitry Baryshkov wrote: >>> On Tue, May 13, 2025 at 09:58:25AM +0530, Ekansh Gupta wrote: >>>> User request for remote heap allocation is supported using ioctl >>>> interface but support for unmap is missing. This could result in >>>> memory leak issues. Add unmap user request support for remote heap. >>> Can this memory be in use by the remote proc? >> Remote heap allocation request is only intended for audioPD. Other PDs >> running on DSP are not intended to use this request. > 'Intended'. That's fine. I asked a different question: _can_ it be in > use? What happens if userspace by mistake tries to unmap memory too > early? Or if it happens intentionally, at some specific time during > work. If the unmap is restricted to audio daemon, then the unmap will only happen if the remoteproc is no longer using this memory. But without this restriction, yes it possible that some userspace process calls unmap which tries to move the ownership back to HLOS which the remoteproc is still using the memory. This might lead to memory access problems. > >>>> Fixes: 532ad70c6d449 ("misc: fastrpc: Add mmap request assigning for static PD pool") >>>> Cc: stable@kernel.org >>>> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com> >>>> --- >>>> drivers/misc/fastrpc.c | 62 ++++++++++++++++++++++++++++++++++-------- >>>> 1 file changed, 51 insertions(+), 11 deletions(-) >>>>
On Thu, 22 May 2025 at 08:01, Ekansh Gupta <ekansh.gupta@oss.qualcomm.com> wrote: > > > > On 5/19/2025 7:04 PM, Dmitry Baryshkov wrote: > > On Mon, May 19, 2025 at 04:28:34PM +0530, Ekansh Gupta wrote: > >> > >> On 5/19/2025 4:22 PM, Dmitry Baryshkov wrote: > >>> On Tue, May 13, 2025 at 09:58:25AM +0530, Ekansh Gupta wrote: > >>>> User request for remote heap allocation is supported using ioctl > >>>> interface but support for unmap is missing. This could result in > >>>> memory leak issues. Add unmap user request support for remote heap. > >>> Can this memory be in use by the remote proc? > >> Remote heap allocation request is only intended for audioPD. Other PDs > >> running on DSP are not intended to use this request. > > 'Intended'. That's fine. I asked a different question: _can_ it be in > > use? What happens if userspace by mistake tries to unmap memory too > > early? Or if it happens intentionally, at some specific time during > > work. > > If the unmap is restricted to audio daemon, then the unmap will only > happen if the remoteproc is no longer using this memory. > > But without this restriction, yes it possible that some userspace process > calls unmap which tries to move the ownership back to HLOS which the > remoteproc is still using the memory. This might lead to memory access > problems. This needs to be fixed in the driver. We need to track which memory is being used by the remoteproc and unmap it once remoteproc stops using it, without additional userspace intervention.
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index d54368bf8c5c..b64c5b9e07b5 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -197,6 +197,8 @@ struct fastrpc_buf { struct dma_buf *dmabuf; struct device *dev; void *virt; + /* Type of buffer */ + u32 flag; u64 phys; u64 size; /* Lock for dma buf attachments */ @@ -1867,8 +1869,26 @@ static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf * err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc, &args[0]); if (!err) { - dev_dbg(dev, "unmmap\tpt 0x%09lx OK\n", buf->raddr); + if (buf->flag == ADSP_MMAP_REMOTE_HEAP_ADDR && fl->cctx->vmcount) { + u64 src_perms = 0; + struct qcom_scm_vmperm dst_perms; + u32 i; + + for (i = 0; i < fl->cctx->vmcount; i++) + src_perms |= BIT(fl->cctx->vmperms[i].vmid); + + dst_perms.vmid = QCOM_SCM_VMID_HLOS; + dst_perms.perm = QCOM_SCM_PERM_RWX; + err = qcom_scm_assign_mem(buf->phys, (u64)buf->size, + &src_perms, &dst_perms, 1); + if (err) { + dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n", + buf->phys, buf->size, err); + return err; + } + } fastrpc_buf_free(buf); + dev_dbg(dev, "unmmap\tpt 0x%09lx OK\n", buf->raddr); } else { dev_err(dev, "unmmap\tpt 0x%09lx ERROR\n", buf->raddr); } @@ -1882,6 +1902,7 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp) struct fastrpc_req_munmap req; struct device *dev = fl->sctx->dev; int err; + unsigned long flags; if (copy_from_user(&req, argp, sizeof(req))) return -EFAULT; @@ -1896,20 +1917,38 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp) } spin_unlock(&fl->lock); - if (!buf) { - dev_err(dev, "mmap\t\tpt 0x%09llx [len 0x%08llx] not in list\n", - req.vaddrout, req.size); - return -EINVAL; + if (buf) { + err = fastrpc_req_munmap_impl(fl, buf); + if (err) { + spin_lock(&fl->lock); + list_add_tail(&buf->node, &fl->mmaps); + spin_unlock(&fl->lock); + } + return err; } - err = fastrpc_req_munmap_impl(fl, buf); - if (err) { - spin_lock(&fl->lock); - list_add_tail(&buf->node, &fl->mmaps); - spin_unlock(&fl->lock); + spin_lock_irqsave(&fl->cctx->lock, flags); + list_for_each_entry_safe(iter, b, &fl->cctx->rhmaps, node) { + if (iter->raddr == req.vaddrout && iter->size == req.size) { + list_del(&iter->node); + buf = iter; + break; + } } + spin_unlock_irqrestore(&fl->cctx->lock, flags); + if (buf) { + err = fastrpc_req_munmap_impl(fl, buf); + if (err) { + spin_lock_irqsave(&fl->cctx->lock, flags); + list_add_tail(&buf->node, &fl->cctx->rhmaps); + spin_unlock_irqrestore(&fl->cctx->lock, flags); + } + return err; + } + dev_err(dev, "mmap\t\tpt 0x%09llx [len 0x%08llx] not in list\n", + req.vaddrout, req.size); - return err; + return -EINVAL; } static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) @@ -1977,6 +2016,7 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) /* update the buffer to be able to deallocate the memory on the DSP */ buf->raddr = (uintptr_t) rsp_msg.vaddr; + buf->flag = req.flags; /* let the client know the address to use */ req.vaddrout = rsp_msg.vaddr;
User request for remote heap allocation is supported using ioctl interface but support for unmap is missing. This could result in memory leak issues. Add unmap user request support for remote heap. Fixes: 532ad70c6d449 ("misc: fastrpc: Add mmap request assigning for static PD pool") Cc: stable@kernel.org Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com> --- drivers/misc/fastrpc.c | 62 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 51 insertions(+), 11 deletions(-)