On Thu, Sep 19, 2024 at 12:00:33PM GMT, Ekansh Gupta wrote: > > > On 8/30/2024 3:03 PM, Dmitry Baryshkov wrote: > > On Thu, Aug 22, 2024 at 04:29:33PM GMT, Ekansh Gupta wrote: > >> If multiple dma handles are passed with same fd over a remote call > >> the kernel driver takes a reference and expects that put for the > >> map will be called as many times to free the map. > >> But DSP only > >> updates the fd one time in the fd list when the DSP refcount > >> goes to zero > > I'm sorry, I couldn't understand this phrase. Could you plese clarify > > what do you mean here? > DMA handle are buffers passed to DSP which are only unmapped when DSP updated > the buffer fd in fdlist. > fdlist implementation: misc: fastrpc: Add fdlist implementation - kernel/git/next/linux-next.git - The linux-next integration testing tree <https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/misc/fastrpc.c?id=8f6c1d8c4f0cc316b0456788fff8373554d1d99d> > > A remote call payload carries both input/output buffers and dma handles. The lifetime > of input/output buffer is a remote call which means that any buffer allocated or mapped > for a remote call will be freed or unmapped when the remote call is completing. Whereas, > dma handles can get freed over some other remote call whenever the DSP will update > fdlist. So if a remote call passed multiple dma handles with same fd to DSP, on driver, ref > count will be incremented, but DSP can update fdlist only 1 time for the same fd as DSP also > has a ref counting happening for the dma handle and fdlist is updated when the DSP ref > count goes to 0. In this case, the map will not get freed even though it is no longer in use. OK, I started looking at the related code. Pleas fix possible map leak in fastrpc_put_args(), happening if the copy_to_user() fails. Second. Please merge fastrpc_map_lookup() + fastrpc_map_put() invocation into a single call, effectively dropping take_ref argument from fastrpc_map_lookup() (which can now become fastrpc_map_get()). Now back to your patch. Please clarify if my understanding is correct: The driver maps dma bufs and passes them to DSP. Then once DSP firmware finds out that a particular buffer is no longer needed, it returns its fd via the fdlist part of the invoke_buf. As these buffers are returned only once, when they are no longer necessary, the kernel should not take additional references on the long-living dma-bufs. If that's the case, see my comments below. > > > >> and hence kernel make put call only once for the > >> fd. This can cause SMMU fault issue as the same fd can be used > >> in future for some other call. > >> > >> Fixes: 35a82b87135d ("misc: fastrpc: Add dma handle implementation") > >> Cc: stable <stable@xxxxxxxxxx> > >> Signed-off-by: Ekansh Gupta <quic_ekangupt@xxxxxxxxxxx> > >> --- > >> drivers/misc/fastrpc.c | 13 ++++++++----- > >> 1 file changed, 8 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > >> index ebe828770a8d..ad56e918e1f8 100644 > >> --- a/drivers/misc/fastrpc.c > >> +++ b/drivers/misc/fastrpc.c > >> @@ -755,7 +755,7 @@ static const struct dma_buf_ops fastrpc_dma_buf_ops = { > >> > >> static int fastrpc_map_create(struct fastrpc_user *fl, int fd, > >> u64 va, u64 len, u32 attr, > >> - struct fastrpc_map **ppmap) > >> + struct fastrpc_map **ppmap, bool take_ref) > >> { > >> struct fastrpc_session_ctx *sess = fl->sctx; > >> struct fastrpc_map *map = NULL; > >> @@ -763,7 +763,7 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd, > >> struct scatterlist *sgl = NULL; > >> int err = 0, sgl_index = 0; > >> > >> - if (!fastrpc_map_lookup(fl, fd, va, len, ppmap, true)) > >> + if (!fastrpc_map_lookup(fl, fd, va, len, ppmap, take_ref)) > >> return 0; Do not add the take_ref argument to fastrpc_map_create(). Instead extract the rest of the code to the function fastrpc_map_attach() (or something like that). > >> > >> map = kzalloc(sizeof(*map), GFP_KERNEL); > >> @@ -917,14 +917,17 @@ static int fastrpc_create_maps(struct fastrpc_invoke_ctx *ctx) > >> int i, err; > >> > >> for (i = 0; i < ctx->nscalars; ++i) { > >> + bool take_ref = true; > >> > >> if (ctx->args[i].fd == 0 || ctx->args[i].fd == -1 || > >> ctx->args[i].length == 0) > >> continue; > >> > >> + if (i >= ctx->nbufs) > >> + take_ref = false; > > Please clarify too. > nbufs -> total input/output buffers > nscalars -> nbufs + dma handles > So here, avoiding ref increment for dma handles. > > > >> err = fastrpc_map_create(ctx->fl, ctx->args[i].fd, > >> (u64)ctx->args[i].ptr, ctx->args[i].length, > >> - ctx->args[i].attr, &ctx->maps[i]); > >> + ctx->args[i].attr, &ctx->maps[i], take_ref); Call conditionally either fastrpc_map_create() or fastrpc_map_attach(). > >> if (err) { > >> dev_err(dev, "Error Creating map %d\n", err); > >> return -EINVAL; > >> @@ -1417,7 +1420,7 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl, > >> > >> if (init.filelen && init.filefd) { > >> err = fastrpc_map_create(fl, init.filefd, init.file, > >> - init.filelen, 0, &map); > >> + init.filelen, 0, &map, true); > >> if (err) > >> goto err; > >> } > >> @@ -2040,7 +2043,7 @@ static int fastrpc_req_mem_map(struct fastrpc_user *fl, char __user *argp) > >> > >> /* create SMMU mapping */ > >> err = fastrpc_map_create(fl, req.fd, req.vaddrin, req.length, > >> - 0, &map); > >> + 0, &map, true); > >> if (err) { > >> dev_err(dev, "failed to map buffer, fd = %d\n", req.fd); > >> return err; > >> -- > >> 2.34.1 > >> > -- With best wishes Dmitry
