On Thu, Feb 26, 2026 at 08:41:21PM +0530, Anandu Krishnan E wrote: > Add reference counting using kref to the fastrpc_user structure to > prevent use-after-free issues when contexts are freed from workqueue > after device release.
Please follow https://docs.kernel.org/process/submitting-patches.html#describe-your-changes and start your commit message by clearly establishing the problem, once that's done you can describe the technical solution. > > The issue occurs when fastrpc_device_release() frees the user structure > while invoke contexts are still pending in the workqueue. When the > workqueue later calls fastrpc_context_free(), it attempts to access > buf->fl->cctx in fastrpc_buf_free(), leading to a use-after-free: But why does it do that? The reason why we need buf->fl->cctx in this context is because we need to mask out the DMA address from the buf->dma_addr (remove the SID bits). If we instead split "dma_addr" into two separate members of struct fastrpc_buf, one dma_addr_t dma_addr and one u64 iova_with_sid (?), then it would be clear throughout the driver which address space we're talking about (is it the SID-adjusted iova space or the dma_addr_t in the particular DMA context). In addition to making this aspect of the driver easier to follow, you no longer need to call fastrpc_ipa_to_dma_addr() in fastrpc_buf_free() (or anywhere else for that matter). You can just pass buf->dma_addr directly to dma_free_coherent(). You're isolating the fact that the firmware needs to see "SID | dma_addr" to just two places in the driver. > > pc : fastrpc_buf_free+0x38/0x80 [fastrpc] > lr : fastrpc_context_free+0xa8/0x1b0 [fastrpc] > ... > fastrpc_context_free+0xa8/0x1b0 [fastrpc] > fastrpc_context_put_wq+0x78/0xa0 [fastrpc] > process_one_work+0x180/0x450 > worker_thread+0x26c/0x388 > > Implement proper reference counting to fix this: > - Initialize kref in fastrpc_device_open() > - Take a reference in fastrpc_context_alloc() for each context > - Release the reference in fastrpc_context_free() when context is freed > - Release the initial reference in fastrpc_device_release() There's no reason to include a checklist of pseudo-code in the commit message, describe why and the overall design if this isn't obvious. > > This ensures the user structure remains valid as long as there are > contexts holding references to it, preventing the race condition. > The life cycles at play in this driver is already very hard to reason about. > Fixes: 6cffd79504ce ("misc: fastrpc: Add support for dmabuf exporter") > Cc: [email protected] > Signed-off-by: Anandu Krishnan E <[email protected]> > --- > drivers/misc/fastrpc.c | 35 +++++++++++++++++++++++++++++++---- > 1 file changed, 31 insertions(+), 4 deletions(-) > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index 47356a5d5804..3ababcf327d7 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -310,6 +310,8 @@ struct fastrpc_user { > spinlock_t lock; > /* lock for allocations */ > struct mutex mutex; > + /* Reference count */ > + struct kref refcount; > }; > > /* Extract SMMU PA from consolidated IOVA */ > @@ -497,15 +499,36 @@ static void fastrpc_channel_ctx_put(struct > fastrpc_channel_ctx *cctx) > kref_put(&cctx->refcount, fastrpc_channel_ctx_free); > } > > +static void fastrpc_user_free(struct kref *ref) > +{ > + struct fastrpc_user *fl = container_of(ref, struct fastrpc_user, > refcount); Unrelated question, what does the "fl" abbreviation actually mean? I presume 'f' is for "fastrpc", but what is 'l'? Regards, Bjorn > + > + fastrpc_channel_ctx_put(fl->cctx); > + mutex_destroy(&fl->mutex); > + kfree(fl); > +} > + > +static void fastrpc_user_get(struct fastrpc_user *fl) > +{ > + kref_get(&fl->refcount); > +} > + > +static void fastrpc_user_put(struct fastrpc_user *fl) > +{ > + kref_put(&fl->refcount, fastrpc_user_free); > +} > + > static void fastrpc_context_free(struct kref *ref) > { > struct fastrpc_invoke_ctx *ctx; > struct fastrpc_channel_ctx *cctx; > + struct fastrpc_user *fl; > unsigned long flags; > int i; > > ctx = container_of(ref, struct fastrpc_invoke_ctx, refcount); > cctx = ctx->cctx; > + fl = ctx->fl; > > for (i = 0; i < ctx->nbufs; i++) > fastrpc_map_put(ctx->maps[i]); > @@ -521,6 +544,8 @@ static void fastrpc_context_free(struct kref *ref) > kfree(ctx->olaps); > kfree(ctx); > > + /* Release the reference taken in fastrpc_context_alloc() */ > + fastrpc_user_put(fl); > fastrpc_channel_ctx_put(cctx); > } > > @@ -628,6 +653,8 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc( > > /* Released in fastrpc_context_put() */ > fastrpc_channel_ctx_get(cctx); > + /* Take a reference to user, released in fastrpc_context_free() */ > + fastrpc_user_get(user); > > ctx->sc = sc; > ctx->retval = -1; > @@ -658,6 +685,7 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc( > spin_lock(&user->lock); > list_del(&ctx->node); > spin_unlock(&user->lock); > + fastrpc_user_put(user); > fastrpc_channel_ctx_put(cctx); > kfree(ctx->maps); > kfree(ctx->olaps); > @@ -1606,11 +1634,9 @@ static int fastrpc_device_release(struct inode *inode, > struct file *file) > } > > fastrpc_session_free(cctx, fl->sctx); > - fastrpc_channel_ctx_put(cctx); > - > - mutex_destroy(&fl->mutex); > - kfree(fl); > file->private_data = NULL; > + /* Release the reference taken in fastrpc_device_open */ > + fastrpc_user_put(fl); > > return 0; > } > @@ -1654,6 +1680,7 @@ static int fastrpc_device_open(struct inode *inode, > struct file *filp) > spin_lock_irqsave(&cctx->lock, flags); > list_add_tail(&fl->user, &cctx->users); > spin_unlock_irqrestore(&cctx->lock, flags); > + kref_init(&fl->refcount); > > return 0; > } > -- > 2.34.1 > >
