Re: [PATCH v1] i3c: master: allocate IBI workqueue with WQ_HIGHPRI

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Race condition in `i3c_dev_free_ibi_locked()` allows a top-half IRQ handler to cause a use-after-free and NULL pointer dereference.
--

--- Patch [1]: [PATCH v1] i3c: master: allocate IBI workqueue with WQ_HIGHPRI ---
commit 9b5ec34ca4a070587e40145288be3b34ce061d58
Author: Stanley Chu <[email protected]>

i3c: master: allocate IBI workqueue with WQ_HIGHPRI

This commit adds the WQ_HIGHPRI flag to the IBI (In-Band Interrupt) workqueue
allocation in the I3C master driver. This ensures that IBI bottom-half work
is scheduled promptly, avoiding latency issues for sensitive clients that
could occur if processed at normal worker priority.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index afcd7a21a3e66..4fa90013290f1 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -3945,7 +3945,8 @@ int i3c_dev_request_ibi_locked(struct i3c_dev_desc *dev,
>  	if (!ibi)
>  		return -ENOMEM;
>  
> -	ibi->wq = alloc_ordered_workqueue(dev_name(i3cdev_to_dev(dev->dev)), WQ_MEM_RECLAIM);
> +	ibi->wq = alloc_ordered_workqueue(dev_name(i3cdev_to_dev(dev->dev)),
> +					  WQ_MEM_RECLAIM | WQ_HIGHPRI);
[Severity: High]
This isn't a bug introduced by this patch, but is there a race condition in
i3c_dev_free_ibi_locked() when tearing down this workqueue?

When an I3C device driver frees its IBI (e.g. during unbind),
i3c_dev_free_ibi_locked() destroys the workqueue and frees dev->ibi. However,
there is no synchronize_irq() to wait for executing master controller IRQ
handlers to finish.

A concurrently executing IRQ handler can read the device descriptor, be briefly
delayed, and then invoke i3c_master_queue_ibi():

drivers/i3c/master.c:i3c_master_queue_ibi() {
    ...
	atomic_inc(&dev->ibi->pending_ibis);
	queue_work(dev->ibi->wq, &slot->work);
    ...
}

If this happens after i3c_dev_free_ibi_locked() has already destroyed the
workqueue and freed dev->ibi:

drivers/i3c/master.c:i3c_dev_free_ibi_locked() {
    ...
	if (dev->ibi->wq) {
		destroy_workqueue(dev->ibi->wq);
		dev->ibi->wq = NULL;
	}

	kfree(dev->ibi);
	dev->ibi = NULL;
    ...
}

Could this lead to a use-after-free and NULL pointer dereference when accessing
dev->ibi->wq inside queue_work()?

>  	if (!ibi->wq) {
>  		kfree(ibi);
>  		return -ENOMEM;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

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