[PATCH v1 3/3] misc: fastrpc: Skip reference for DMA handles

[Dmitry Baryshkov]

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 at kernel.org>
> >> Signed-off-by: Ekansh Gupta <quic_ekangupt at quicinc.com>
> >> ---
> >>  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