| Message ID | 20240606165939.12950-10-quic_ekangupt@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add missing features to FastRPC driver | expand |
On Thu, Jun 06, 2024 at 10:29:29PM +0530, Ekansh Gupta wrote: > A static PD daemon process can request for remote heap allocation > which will allocate memory and change the ownership if the VMID > information were defined. This allocations are currently getting > added to fl mmaps list which could get freed when there is any > daemon kill. There is no way to request for freeing of this memory > also. Add changes to maintain the remote heap allocation in the Simpler. 'Maintain foo' > static PD specific structure and add method to free the memory > on user request. Should this be split into two patches? 'foo and bar' suggests that. > > Fixes: 532ad70c6d44 ("misc: fastrpc: Add mmap request assigning for static PD pool") > Cc: stable <stable@kernel.org> > Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> > --- > drivers/misc/fastrpc.c | 129 +++++++++++++++++++++++++++++++---------- > 1 file changed, 98 insertions(+), 31 deletions(-) > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index 68c1595446d5..32f2e6f625ed 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -210,6 +210,7 @@ struct fastrpc_buf { > struct dma_buf *dmabuf; > struct device *dev; > void *virt; > + u32 flag; > u64 phys; > u64 size; > /* Lock for dma buf attachments */ > @@ -1924,29 +1925,54 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp) > return 0; > } > > -static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *buf) > +static int fastrpc_req_munmap_dsp(struct fastrpc_user *fl, uintptr_t raddr, u64 size) > { > struct fastrpc_invoke_args args[1] = { [0] = { 0 } }; > struct fastrpc_munmap_req_msg req_msg; > - struct device *dev = fl->sctx->dev; > int err; > u32 sc; > > req_msg.pgid = fl->tgid; > - req_msg.size = buf->size; > - req_msg.vaddr = buf->raddr; > + req_msg.size = size; > + req_msg.vaddr = raddr; > > args[0].ptr = (u64) (uintptr_t) &req_msg; > args[0].length = sizeof(req_msg); > - > sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MUNMAP, 1, 0); > err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc, > &args[0]); > + > + return err; > +} > + > +static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *buf) > +{ > + struct device *dev = fl->sctx->dev; > + int err; > + > + err = fastrpc_req_munmap_dsp(fl, buf->raddr, buf->size); > if (!err) { > + if (buf->flag == ADSP_MMAP_REMOTE_HEAP_ADDR) { > + if (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(dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n", > + buf->phys, buf->size, err); > + return err; > + } It looks like this is too nested. Consider refactoring this code. For example, extract the function that you have c&p'ed. > + } > + } > dev_dbg(dev, "unmmap\tpt 0x%09lx OK\n", buf->raddr); > - spin_lock(&fl->lock); > - list_del(&buf->node); > - spin_unlock(&fl->lock); > fastrpc_buf_free(buf); > } else { > dev_err(dev, "unmmap\tpt 0x%09lx ERROR\n", buf->raddr); > @@ -1960,6 +1986,7 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp) > struct fastrpc_buf *buf = NULL, *iter, *b; > struct fastrpc_req_munmap req; > struct device *dev = fl->sctx->dev; > + int err = 0; Why =0 is necessary? > > if (copy_from_user(&req, argp, sizeof(req))) > return -EFAULT; > @@ -1968,18 +1995,45 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp) > list_for_each_entry_safe(iter, b, &fl->mmaps, node) { > if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) { > buf = iter; > + list_del(&buf->node); > break; > } > } > 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; > } > > - return fastrpc_req_munmap_impl(fl, buf); > + spin_lock(&fl->lock); > + if (fl->spd) { > + list_for_each_entry_safe(iter, b, &fl->spd->rmaps, node) { > + if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) { > + buf = iter; > + list_del(&buf->node); > + break; > + } > + } > + } > + spin_unlock(&fl->lock); > + if (buf) { > + err = fastrpc_req_munmap_impl(fl, buf); > + if (err) { > + spin_lock(&fl->lock); > + list_add_tail(&buf->node, &fl->spd->rmaps); > + spin_unlock(&fl->lock); > + } > + return err; > + } > + dev_err(dev, "buffer not found addr 0x%09lx, len 0x%08llx\n", > + req.vaddrout, req.size); Can this be triggered by the user? If so, it's dev_dbg() at best. > + return -EINVAL; > } > > static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) > @@ -2008,15 +2062,34 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) > return -EINVAL; > } > > - if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) > + if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) { > + if (!fl->spd || !fl->spd->ispdup) { > + dev_err(dev, "remote heap request supported only for active static PD\n"); > + return -EINVAL; > + } > err = fastrpc_remote_heap_alloc(fl, dev, req.size, &buf); > - else > + } else { > err = fastrpc_buf_alloc(fl, dev, req.size, &buf); > + } > > if (err) { > dev_err(dev, "failed to allocate buffer\n"); > return err; > } > + buf->flag = req.flags; > + > + /* Add memory to static PD pool, protection through hypervisor */ > + if ((req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && fl->cctx->vmcount) { > + u64 src_perms = BIT(QCOM_SCM_VMID_HLOS); > + > + err = qcom_scm_assign_mem(buf->phys, (u64)buf->size, > + &src_perms, fl->cctx->vmperms, fl->cctx->vmcount); > + 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); misaligned > + goto err_invoke; > + } > + } > > req_msg.pgid = fl->tgid; > req_msg.flags = req.flags; > @@ -2049,26 +2122,16 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) > /* let the client know the address to use */ > req.vaddrout = rsp_msg.vaddr; > > - /* Add memory to static PD pool, protection thru hypervisor */ > - if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR && fl->cctx->vmcount) { > - u64 src_perms = BIT(QCOM_SCM_VMID_HLOS); > - > - err = qcom_scm_assign_mem(buf->phys, (u64)buf->size, > - &src_perms, fl->cctx->vmperms, fl->cctx->vmcount); > - if (err) { > - dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d", > - buf->phys, buf->size, err); > - goto err_assign; > - } > - } > - > spin_lock(&fl->lock); > - list_add_tail(&buf->node, &fl->mmaps); > + if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) > + list_add_tail(&buf->node, &fl->spd->rmaps); > + else > + list_add_tail(&buf->node, &fl->mmaps); > spin_unlock(&fl->lock); > > if (copy_to_user((void __user *)argp, &req, sizeof(req))) { > err = -EFAULT; > - goto err_assign; > + goto err_copy; > } > > dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n", > @@ -2076,8 +2139,12 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) > > return 0; > > -err_assign: > +err_copy: > + spin_lock(&fl->lock); > + list_del(&buf->node); > + spin_unlock(&fl->lock); > fastrpc_req_munmap_impl(fl, buf); > + buf = NULL; > err_invoke: > fastrpc_buf_free(buf); > > -- > 2.43.0 >
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index 68c1595446d5..32f2e6f625ed 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -210,6 +210,7 @@ struct fastrpc_buf { struct dma_buf *dmabuf; struct device *dev; void *virt; + u32 flag; u64 phys; u64 size; /* Lock for dma buf attachments */ @@ -1924,29 +1925,54 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp) return 0; } -static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *buf) +static int fastrpc_req_munmap_dsp(struct fastrpc_user *fl, uintptr_t raddr, u64 size) { struct fastrpc_invoke_args args[1] = { [0] = { 0 } }; struct fastrpc_munmap_req_msg req_msg; - struct device *dev = fl->sctx->dev; int err; u32 sc; req_msg.pgid = fl->tgid; - req_msg.size = buf->size; - req_msg.vaddr = buf->raddr; + req_msg.size = size; + req_msg.vaddr = raddr; args[0].ptr = (u64) (uintptr_t) &req_msg; args[0].length = sizeof(req_msg); - sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MUNMAP, 1, 0); err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc, &args[0]); + + return err; +} + +static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *buf) +{ + struct device *dev = fl->sctx->dev; + int err; + + err = fastrpc_req_munmap_dsp(fl, buf->raddr, buf->size); if (!err) { + if (buf->flag == ADSP_MMAP_REMOTE_HEAP_ADDR) { + if (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(dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n", + buf->phys, buf->size, err); + return err; + } + } + } dev_dbg(dev, "unmmap\tpt 0x%09lx OK\n", buf->raddr); - spin_lock(&fl->lock); - list_del(&buf->node); - spin_unlock(&fl->lock); fastrpc_buf_free(buf); } else { dev_err(dev, "unmmap\tpt 0x%09lx ERROR\n", buf->raddr); @@ -1960,6 +1986,7 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp) struct fastrpc_buf *buf = NULL, *iter, *b; struct fastrpc_req_munmap req; struct device *dev = fl->sctx->dev; + int err = 0; if (copy_from_user(&req, argp, sizeof(req))) return -EFAULT; @@ -1968,18 +1995,45 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp) list_for_each_entry_safe(iter, b, &fl->mmaps, node) { if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) { buf = iter; + list_del(&buf->node); break; } } 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; } - return fastrpc_req_munmap_impl(fl, buf); + spin_lock(&fl->lock); + if (fl->spd) { + list_for_each_entry_safe(iter, b, &fl->spd->rmaps, node) { + if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) { + buf = iter; + list_del(&buf->node); + break; + } + } + } + spin_unlock(&fl->lock); + if (buf) { + err = fastrpc_req_munmap_impl(fl, buf); + if (err) { + spin_lock(&fl->lock); + list_add_tail(&buf->node, &fl->spd->rmaps); + spin_unlock(&fl->lock); + } + return err; + } + dev_err(dev, "buffer not found addr 0x%09lx, len 0x%08llx\n", + req.vaddrout, req.size); + return -EINVAL; } static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) @@ -2008,15 +2062,34 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) return -EINVAL; } - if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) + if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) { + if (!fl->spd || !fl->spd->ispdup) { + dev_err(dev, "remote heap request supported only for active static PD\n"); + return -EINVAL; + } err = fastrpc_remote_heap_alloc(fl, dev, req.size, &buf); - else + } else { err = fastrpc_buf_alloc(fl, dev, req.size, &buf); + } if (err) { dev_err(dev, "failed to allocate buffer\n"); return err; } + buf->flag = req.flags; + + /* Add memory to static PD pool, protection through hypervisor */ + if ((req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && fl->cctx->vmcount) { + u64 src_perms = BIT(QCOM_SCM_VMID_HLOS); + + err = qcom_scm_assign_mem(buf->phys, (u64)buf->size, + &src_perms, fl->cctx->vmperms, fl->cctx->vmcount); + 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); + goto err_invoke; + } + } req_msg.pgid = fl->tgid; req_msg.flags = req.flags; @@ -2049,26 +2122,16 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) /* let the client know the address to use */ req.vaddrout = rsp_msg.vaddr; - /* Add memory to static PD pool, protection thru hypervisor */ - if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR && fl->cctx->vmcount) { - u64 src_perms = BIT(QCOM_SCM_VMID_HLOS); - - err = qcom_scm_assign_mem(buf->phys, (u64)buf->size, - &src_perms, fl->cctx->vmperms, fl->cctx->vmcount); - if (err) { - dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d", - buf->phys, buf->size, err); - goto err_assign; - } - } - spin_lock(&fl->lock); - list_add_tail(&buf->node, &fl->mmaps); + if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) + list_add_tail(&buf->node, &fl->spd->rmaps); + else + list_add_tail(&buf->node, &fl->mmaps); spin_unlock(&fl->lock); if (copy_to_user((void __user *)argp, &req, sizeof(req))) { err = -EFAULT; - goto err_assign; + goto err_copy; } dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n", @@ -2076,8 +2139,12 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) return 0; -err_assign: +err_copy: + spin_lock(&fl->lock); + list_del(&buf->node); + spin_unlock(&fl->lock); fastrpc_req_munmap_impl(fl, buf); + buf = NULL; err_invoke: fastrpc_buf_free(buf);
A static PD daemon process can request for remote heap allocation which will allocate memory and change the ownership if the VMID information were defined. This allocations are currently getting added to fl mmaps list which could get freed when there is any daemon kill. There is no way to request for freeing of this memory also. Add changes to maintain the remote heap allocation in the static PD specific structure and add method to free the memory on user request. Fixes: 532ad70c6d44 ("misc: fastrpc: Add mmap request assigning for static PD pool") Cc: stable <stable@kernel.org> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> --- drivers/misc/fastrpc.c | 129 +++++++++++++++++++++++++++++++---------- 1 file changed, 98 insertions(+), 31 deletions(-)