Re: [PATCH v15 2/8] i3c: master: Add controller-only device operation helpers

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

New issues:
- [Low] Incomplete refactoring: `i3c_master_handle_ibi()` bypasses the newly introduced `i3c_dev_recycle_ibi_slot_controller()` helper.

Pre-existing issues:
- [High] Missing memory barrier in `i3c_dev_disable_ibi_locked` causes a lost wakeup and indefinite hang.
- [High] Use-after-free on error path in `i3c_dev_free_ibi_locked` when workqueue is drained after resources are freed.
--

--- Patch [2]: [PATCH v15 2/8] i3c: master: Add controller-only device operation helpers ---
commit 85e9cb3ce264f7889b2b4406bc379307ad054a14
Author: Lakshay Piplani <[email protected]>

i3c: master: Add controller-only device operation helpers

The generic I3C device attach and IBI paths perform both core-level
bookkeeping and controller-specific operations. An I3C hub, however,
maintains a logical device descriptor on the virtual downstream bus and
a separate parent-facing descriptor used by the physical parent
controller.

>     Add helpers for:
>       - attaching, reattaching, and detaching a device from a controller;
>       - requesting and freeing controller IBI resources;
>       - enabling and disabling IBIs at the controller;
>       - recycling controller IBI slots.
>     
>     Refactor the existing generic core paths to use these helpers, keeping
>     the current behaviour unchanged for regular I3C devices.

[Severity: Low]
The commit message mentions refactoring the existing generic core paths to
use the new controller-only helpers, including the one for recycling IBI 
slots.

However, looking at the code for i3c_master_handle_ibi(), it appears to
still bypass the newly introduced wrapper:

drivers/i3c/master.c:i3c_master_handle_ibi() {
        ...
        if (master->ops->recycle_ibi_slot)
                master->ops->recycle_ibi_slot(dev, slot);
        ...
}

Does this function need to be updated to use 
i3c_dev_recycle_ibi_slot_controller() to ensure the virtual controllers
in the new I3C hub framework can properly intercept these operations?

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 03fb41f0786c5..7ae6b3aaa9b4c 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c

[ ... ]

> @@ -3534,14 +3632,9 @@ EXPORT_SYMBOL_GPL(i3c_dev_do_xfers_locked);
>   */
>  int i3c_dev_disable_ibi_locked(struct i3c_dev_desc *dev)
>  {
> -	struct i3c_master_controller *master;
>  	int ret;
>  
> -	if (!dev->ibi)
> -		return -EINVAL;
> -
> -	master = i3c_dev_get_master(dev);
> -	ret = master->ops->disable_ibi(dev);
> +	ret = i3c_dev_disable_ibi_controller_locked(dev);
>  	if (ret)
>  		return ret;

[Severity: High]
This is a pre-existing issue, but does this sequence lack a memory barrier
before checking the pending IBIs?

Right after this return, the code does:

drivers/i3c/master.c:i3c_dev_disable_ibi_locked() {
        ...
        reinit_completion(&dev->ibi->all_ibis_handled);
        if (atomic_read(&dev->ibi->pending_ibis))
                wait_for_completion(&dev->ibi->all_ibis_handled);
        ...
}

Since reinit_completion() performs a plain store that can be delayed or
reordered after the atomic_read(), could a concurrent IBI worker
decrementing the pending count and calling complete() race with this?

If the worker executes between the load of pending_ibis and the delayed
store, could the store overwrite the completion state back to 0, causing
wait_for_completion() to hang indefinitely?

[ ... ]

> @@ -3667,7 +3835,7 @@ void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
>  			dev_err(&master->dev, "Failed to disable IBI before freeing\n");
>  	}
>  
> -	master->ops->free_ibi(dev);
> +	i3c_dev_free_ibi_controller_locked(dev);
>  
>  	if (dev->ibi->wq) {
>  		destroy_workqueue(dev->ibi->wq);

[Severity: High]
This is a pre-existing issue, but does this cleanup order create a
use-after-free risk during an error path?

If disabling the IBI fails (for example, due to a hardware timeout), the
error path continues and calls i3c_dev_free_ibi_controller_locked(),
which frees the underlying controller resources like the generic IBI pool.

Then destroy_workqueue() is called to drain the workqueue. If there are
pending or executing IBI works still in the workqueue, won't they try to
access the freed slot pool when they run, resulting in a use-after-free?

-- 
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