Re: [PATCH V5 11/14] i3c: master: Reject IBI requests from non-IBI-capable devices

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

Pre-existing issues:
- [High] Workqueue memory and rescuer thread leak on the error path of i3c_dev_request_ibi_locked.
--

--- Patch [11]: [PATCH V5 11/14] i3c: master: Reject IBI requests from non-IBI-capable devices ---
commit d6c55123f731ff15725d89d2f9b3fef0f879efa9
Author: Adrian Hunter <[email protected]>

i3c: master: Reject IBI requests from non-IBI-capable devices

This patch checks if a device advertises IBI support before attempting to
set up IBI handling. It adds a check for I3C_BCR_IBI_REQ_CAP in
i3c_device_request_ibi() and returns -EOPNOTSUPP if not supported.

> diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
> --- a/drivers/i3c/device.c
> +++ b/drivers/i3c/device.c
[ ... ]
> @@ -204,12 +204,14 @@ int i3c_device_request_ibi(struct i3c_device *dev,
>  		return ret;
>  
>  	i3c_bus_normaluse_lock(dev->bus);
> -	if (dev->desc) {
> +	if (!dev->desc) {
> +		ret = -ENOENT;
> +	} else if (!(dev->desc->info.bcr & I3C_BCR_IBI_REQ_CAP)) {
> +		ret = -EOPNOTSUPP;
> +	} else {
>  		mutex_lock(&dev->desc->ibi_lock);
>  		ret = i3c_dev_request_ibi_locked(dev->desc, req);

[Severity: High]
This is a pre-existing issue, but looking at the error path of
i3c_dev_request_ibi_locked() in drivers/i3c/master.c, does it leak the
workqueue and its rescuer thread if the hardware driver fails the request?

drivers/i3c/master.c:i3c_dev_request_ibi_locked() {
    ...
	ibi->wq = alloc_ordered_workqueue(dev_name(i3cdev_to_dev(dev->dev)),
                                          WQ_MEM_RECLAIM);
	if (!ibi->wq) {
		kfree(ibi);
		return -ENOMEM;
	}
    ...
	ret = master->ops->request_ibi(dev, req);
	if (ret) {
		kfree(ibi);
		dev->ibi = NULL;
	}

	return ret;
}

When a device driver requests an IBI and the bus controller fails the
request, the ibi struct is freed but destroy_workqueue() is never called.
Since the workqueue is created with WQ_MEM_RECLAIM, wouldn't this leak a
kernel task and its associated memory on every failure?

>  		mutex_unlock(&dev->desc->ibi_lock);
> -	} else {
> -		ret = -ENOENT;
>  	}
>  	i3c_bus_normaluse_unlock(dev->bus);

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

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