On Thu, Jun 06, 2024 at 10:29:30PM +0530, Ekansh Gupta wrote: > Unsigned PDs are sandboxed DSP processes used to offload computation > workloads to the DSP. Unsigned PD have less privileges in terms of > DSP resource access as compared to Signed PD. > > Unsigned PD requires more initial memory to spawn. Also most of the > memory request are allocated from user space. Current initial memory > size is not sufficient and mapping request for user space allocated > buffer is not supported. This results in failure of unsigned PD offload > request. Add changes to fix initial memory size and user space allocated > buffer mapping support. You can guess my comment here. > > Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP") And here. > Cc: stable <stable@xxxxxxxxxx> > Signed-off-by: Ekansh Gupta <quic_ekangupt@xxxxxxxxxxx> > --- > drivers/misc/fastrpc.c | 180 ++++++++++++++++++++++++++++++----------- > 1 file changed, 133 insertions(+), 47 deletions(-) > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index 32f2e6f625ed..5ffb6098ac38 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -40,7 +40,7 @@ > #define FASTRPC_INIT_HANDLE 1 > #define FASTRPC_DSP_UTILITIES_HANDLE 2 > #define FASTRPC_CTXID_MASK (0xFF0) > -#define INIT_FILELEN_MAX (2 * 1024 * 1024) > +#define INIT_FILELEN_MAX (5 * 1024 * 1024) So, there are two things being fixed here. One is the insufficient memory size, another one is the mmap request. Separate commits, please. > #define INIT_FILE_NAMELEN_MAX (128) > #define FASTRPC_DEVICE_NAME "fastrpc" > > @@ -327,6 +327,7 @@ struct fastrpc_user { > int tgid; > int pd; > bool is_secure_dev; > + bool is_unsigned_pd; > char *servloc_name; > /* Lock for lists */ > spinlock_t lock; > @@ -1488,7 +1489,6 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl, > u32 siglen; > } inbuf; > u32 sc; > - bool unsigned_module = false; > > args = kcalloc(FASTRPC_CREATE_PROCESS_NARGS, sizeof(*args), GFP_KERNEL); > if (!args) > @@ -1500,9 +1500,9 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl, > } > > if (init.attrs & FASTRPC_MODE_UNSIGNED_MODULE) > - unsigned_module = true; > + fl->is_unsigned_pd = true; > > - if (is_session_rejected(fl, unsigned_module)) { > + if (is_session_rejected(fl, fl->is_unsigned_pd)) { > err = -ECONNREFUSED; > goto err; > } > @@ -1985,6 +1985,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 fastrpc_map *map = NULL, *iterm, *m; > struct device *dev = fl->sctx->dev; > int err = 0; > > @@ -2031,34 +2032,75 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp) > } > return err; > } > - dev_err(dev, "buffer not found addr 0x%09lx, len 0x%08llx\n", > + spin_lock(&fl->lock); > + list_for_each_entry_safe(iterm, m, &fl->maps, node) { > + if (iterm->raddr == req.vaddrout) { > + map = iterm; > + break; > + } > + } > + spin_unlock(&fl->lock); > + if (!map) { > + dev_dbg(dev, "buffer not found addr 0x%09llx, len 0x%08llx\n", > req.vaddrout, req.size); > - return -EINVAL; > + return -EINVAL; > + } > + > + err = fastrpc_req_munmap_dsp(fl, map->raddr, map->size); > + if (err) > + dev_dbg(dev, "unmmap\tpt fd = %d, 0x%09llx error\n", map->fd, map->raddr); Which error? The message is useless. > + else > + fastrpc_map_put(map); Should the fl->lock be still held here? Can the map be modified concurrently? > + > + return err; > } > > -static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) > +static int fastrpc_req_map_dsp(struct fastrpc_user *fl, u64 phys, > + u64 size, u32 flag, uintptr_t vaddrin, u64 *raddr) > { > struct fastrpc_invoke_args args[3] = { [0 ... 2] = { 0 } }; > - struct fastrpc_buf *buf = NULL; > struct fastrpc_mmap_req_msg req_msg; > struct fastrpc_mmap_rsp_msg rsp_msg; > struct fastrpc_phy_page pages; > - struct fastrpc_req_mmap req; > - struct device *dev = fl->sctx->dev; > int err; > u32 sc; > > - if (copy_from_user(&req, argp, sizeof(req))) > - return -EFAULT; > + req_msg.pgid = fl->tgid; > + req_msg.flags = flag; > + req_msg.vaddr = vaddrin; > + req_msg.num = sizeof(pages); > > - if (req.flags != ADSP_MMAP_ADD_PAGES && req.flags != ADSP_MMAP_REMOTE_HEAP_ADDR) { > - dev_err(dev, "flag not supported 0x%x\n", req.flags); > + args[0].ptr = (u64) (uintptr_t) &req_msg; > + args[0].length = sizeof(req_msg); > > - return -EINVAL; > + pages.addr = phys; > + pages.size = size; > + > + args[1].ptr = (u64) (uintptr_t) &pages; > + args[1].length = sizeof(pages); > + sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MMAP, 2, 1); > + err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc, > + &args[0]); > + > + if (err) { > + dev_err(fl->sctx->dev, "mmap error (len 0x%08llx)\n", size); > + return err; > } > + *raddr = rsp_msg.vaddr; > + > + return err; > +} > + > +static int fastrpc_req_buf_alloc(struct fastrpc_user *fl, > + struct fastrpc_req_mmap req, char __user *argp) > +{ > + struct device *dev = fl->sctx->dev; > + struct fastrpc_buf *buf = NULL; > + u64 raddr = 0; > + int err; > > if (req.vaddrin) { > - dev_err(dev, "adding user allocated pages is not supported\n"); > + dev_err(dev, "adding user allocated pages is not supported for signed PD\n"); Drop, less chance of users spamming the dmesg. > return -EINVAL; > } > > @@ -2091,36 +2133,16 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) > } > } > > - req_msg.pgid = fl->tgid; > - req_msg.flags = req.flags; > - req_msg.vaddr = req.vaddrin; > - req_msg.num = sizeof(pages); > - > - args[0].ptr = (u64) (uintptr_t) &req_msg; > - args[0].length = sizeof(req_msg); > - > - pages.addr = buf->phys; > - pages.size = buf->size; > - > - args[1].ptr = (u64) (uintptr_t) &pages; > - args[1].length = sizeof(pages); > - > - args[2].ptr = (u64) (uintptr_t) &rsp_msg; > - args[2].length = sizeof(rsp_msg); > - > - sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MMAP, 2, 1); > - err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc, > - &args[0]); > - if (err) { > - dev_err(dev, "mmap error (len 0x%08llx)\n", buf->size); > + err = fastrpc_req_map_dsp(fl, buf->phys, buf->size, buf->flag, > + req.vaddrin, &raddr); > + if (err) > goto err_invoke; > - } > > /* update the buffer to be able to deallocate the memory on the DSP */ > - buf->raddr = (uintptr_t) rsp_msg.vaddr; > + buf->raddr = (uintptr_t) raddr; > > /* let the client know the address to use */ > - req.vaddrout = rsp_msg.vaddr; > + req.vaddrout = raddr; > > spin_lock(&fl->lock); > if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) > @@ -2129,16 +2151,14 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) > list_add_tail(&buf->node, &fl->mmaps); > spin_unlock(&fl->lock); > > + dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n", > + buf->raddr, buf->size); > + > if (copy_to_user((void __user *)argp, &req, sizeof(req))) { > err = -EFAULT; > goto err_copy; > } > - > - dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n", > - buf->raddr, buf->size); > - > return 0; > - > err_copy: > spin_lock(&fl->lock); > list_del(&buf->node); > @@ -2146,11 +2166,77 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) > fastrpc_req_munmap_impl(fl, buf); > buf = NULL; > err_invoke: > - fastrpc_buf_free(buf); > + if (buf) > + fastrpc_buf_free(buf); > + > + return err; > +} > + > +static int fastrpc_req_map_create(struct fastrpc_user *fl, > + struct fastrpc_req_mmap req, char __user *argp) > +{ > + struct fastrpc_map *map = NULL; > + struct device *dev = fl->sctx->dev; > + u64 raddr = 0; > + int err; > + > + if ((req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && fl->is_unsigned_pd) { > + dev_err(dev, "secure memory allocation is not supported in unsigned PD\n"); > + return -EINVAL; > + } > + err = fastrpc_map_create(fl, req.fd, req.size, 0, &map); > + if (err) { > + dev_err(dev, "failed to map buffer, fd = %d\n", req.fd); > + return err; > + } > + > + err = fastrpc_req_map_dsp(fl, map->phys, map->size, req.flags, > + req.vaddrin, &raddr); > + if (err) > + goto err_invoke; > + > + /* update the buffer to be able to deallocate the memory on the DSP */ > + map->raddr = (uintptr_t) raddr; > + > + /* let the client know the address to use */ > + req.vaddrout = raddr; > + dev_dbg(dev, "mmap\t\tpt 0x%09llx OK [len 0x%08llx]\n", > + map->raddr, map->size); > + > + if (copy_to_user((void __user *)argp, &req, sizeof(req))) { > + err = -EFAULT; > + goto err_copy; > + } > + return 0; > +err_copy: > + fastrpc_req_munmap_dsp(fl, map->raddr, map->size); > +err_invoke: > + fastrpc_map_put(map); > > return err; > } > > +static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) > +{ > + struct fastrpc_req_mmap req; > + int err; > + > + if (copy_from_user(&req, argp, sizeof(req))) > + return -EFAULT; > + > + if ((req.flags == ADSP_MMAP_ADD_PAGES || > + req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && !fl->is_unsigned_pd) { > + err = fastrpc_req_buf_alloc(fl, req, argp); > + if (err) > + return err; > + } else { > + err = fastrpc_req_map_create(fl, req, argp); > + if (err) > + return err; > + } > + return 0; > +} > + > static int fastrpc_req_mem_unmap_impl(struct fastrpc_user *fl, struct fastrpc_mem_unmap *req) > { > struct fastrpc_invoke_args args[1] = { [0] = { 0 } }; > -- > 2.43.0 > -- With best wishes Dmitry