Re: [PATCH v14 2/8] i3c: master: Fix IBI request and free cleanup paths

On Tue, Jul 14, 2026 at 10:20:25AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [High] Bypassing the core cleanup paths when `dev->ibi` is cleared by a controller callback causes a permanent leak of the IBI workqueue (including its rescuer kthread) and potentially the `ibi` object itself.
>
> Pre-existing issues:
> - [High] Potential use-after-free of IBI pool during workqueue destruction if IBI disable fails.
> --
>
> --- Patch [2]: [PATCH v14 2/8] i3c: master: Fix IBI request and free cleanup paths ---
> commit e0d1c57d2924f66898a40232f7e52ca0e1be0aea
> Author: Lakshay Piplani <[email protected]>
>
> i3c: master: Fix IBI request and free cleanup paths
>
> This commit attempts to fix a workqueue leak in the IBI request failure path
> by destroying the workqueue before freeing the IBI object. It also adds checks
> to avoid touching dev->ibi after controller callbacks if they have already
> cleared it, aiming to prevent NULL pointer dereferences.
>
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index 3b1c342be2bc5..ba37f71badac8 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
> > @@ -3702,8 +3702,18 @@ int i3c_dev_request_ibi_locked(struct i3c_dev_desc *dev,
> >
> >  	dev->ibi = ibi;
> >  	ret = master->ops->request_ibi(dev, req);
> > -	if (ret) {
> > -		kfree(ibi);
> > +
> > +	/*
> > +	 * The controller callback may have already released and cleared dev->ibi
> > +	 * when the request is forwarded by a virtual controller. Only clean up the
> > +	 * IBI object if the callback left dev->ibi valid.
> > +	 */
> > +	if (ret && dev->ibi) {
> > +		/* Avoid leaking the workqueue allocated for this IBI request. */
> > +		if (dev->ibi->wq)
> > +			destroy_workqueue(dev->ibi->wq);
> > +
> > +		kfree(dev->ibi);
> >  		dev->ibi = NULL;
> >  	}
>
> [Severity: High]
> Does this logic introduce a permanent leak of the IBI workqueue and the local
> ibi object?
>
> Since the core framework dynamically allocated ibi and ibi->wq earlier in
> i3c_dev_request_ibi_locked(), if master->ops->request_ibi() fails and the
> callback cleared dev->ibi, the check if (ret && dev->ibi) will be false.
>
> Because the callback does not own these resources and cannot safely destroy
> the framework's workqueue, the workqueue (including its rescuer kthread) and
> the ibi object will be skipped during cleanup and leak permanently.

look like make sense
	if (ret) {
		if (ibi=>wq)
			destroy_workqueue(...);
		kfree(ibi)
		dev->ibi = NULL;
	}

>
> [ ... ]
>
> > @@ -3743,6 +3753,13 @@ void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
> >
> >  	master->ops->free_ibi(dev);
>
> [Severity: High]
> This isn't a bug introduced by this patch, but can this lead to a
> use-after-free of the IBI pool memory if disabling the IBI fails?
>
> Earlier in i3c_dev_free_ibi_locked(), if the IBI is still enabled, it tries
> to disable it via i3c_dev_disable_ibi_locked(). If that disable fails
> (e.g., due to a hardware timeout), it returns an error without waiting for
> pending IBIs to finish, and we proceed directly to master->ops->free_ibi(dev).
>
> This callback immediately frees the generic IBI pool. Subsequently,
> destroy_workqueue() is called below, which synchronously drains
> any executing IBI handler works. Since these pending work items reside inside
> the newly-freed pool, won't the rescuer thread dereference freed memory as
> the workqueue drains?
>
> > +	/*
> > +	 * The controller callback may have already released dev->ibi, for example
> > +	 * when the request was forwarded by a virtual controller.
> > +	 */
> > +	if (!dev->ibi)
> > +		return;
>
> [Severity: High]
> Similar to the request path above, does this early return bypass the
> destruction of the workqueue and cause a resource leak?
>
> If the controller callback cleared dev->ibi on the free path, we return
> early and skip the destroy_workqueue(dev->ibi->wq) step, leaking the
> workqueue and its rescuer thread.

make sense

Frank
>
> > +
> >  	if (dev->ibi->wq) {
> >  		destroy_workqueue(dev->ibi->wq);
> >  		dev->ibi->wq = NULL;
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c