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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing memory barrier between `reinit_completion` and `atomic_read` can lead to an indefinite hang.
- [High] Use-after-free of `dev->ibi` and generic IBI pool if the controller fails to disable IBI.
- [High] Device address state corruption and resource collisions if reattach callback fails.
--

commit 6b2a9825b802ec5c65a800ad2b66b3cd53a9d304
Author: Lakshay Piplani <lakshay.piplani@nxp.com>

    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.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 9ea4726495ff6..b864fcbb1a594 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
[ ... ]
> @@ -1958,25 +2005,39 @@ int i3c_master_reattach_i3c_dev_locked(struct i3c_dev_desc *dev,
>  						     I3C_ADDR_SLOT_FREE);
>  	}
>  
> -	if (master->ops->reattach_i3c_dev) {
> -		ret = master->ops->reattach_i3c_dev(dev, old_dyn_addr);
> -		if (ret) {
> -			i3c_master_put_i3c_addrs(dev);
> -			return ret;
> -		}
> +	ret = i3c_master_reattach_i3c_dev_controller_locked(dev, old_dyn_addr);
> +	if (ret) {
> +		i3c_master_put_i3c_addrs(dev);
> +		return ret;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but on this error path, if the
reattach fails, we free the bus addresses by calling
i3c_master_put_i3c_addrs(dev), but we leave the device attached to the
master->bus.devs.i3c list.

If callers ignore the return value of the reattach function, couldn't this
physical device remain on the bus list while its address is marked as FREE,
leading to address collisions on the I3C bus during subsequent device
hot-joins or discoveries?

[ ... ]
> @@ -3963,14 +4072,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;
>  
> @@ -3984,6 +4088,32 @@ int i3c_dev_disable_ibi_locked(struct i3c_dev_desc *dev)
>  }
>  EXPORT_SYMBOL_GPL(i3c_dev_disable_ibi_locked);

[Severity: High]
This is a pre-existing issue, but missing memory barriers in this function
can lead to an indefinite hang.

i3c_dev_disable_ibi_locked() calls:

	reinit_completion(&dev->ibi->all_ibis_handled);
	if (atomic_read(&dev->ibi->pending_ibis))
		wait_for_completion(&dev->ibi->all_ibis_handled);

Because there is no memory barrier (such as smp_mb()) between
reinit_completion() (a non-volatile store) and atomic_read() (a volatile
read), weakly ordered CPUs could reorder the load of pending_ibis to execute
before the store to done. 

If this happens, could this cause CPU 0 to sleep in wait_for_completion()
forever if CPU 1 completes the last IBI before CPU 0 commits done = 0?

[ ... ]
> @@ -4096,7 +4279,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 this error path seems to unconditionally
free the IBI structures even if the hardware is still active.

In i3c_dev_free_ibi_locked(), if the controller fails to disable IBIs:

	if (ret)
		dev_err(&master->dev, "Failed to disable IBI before freeing\n");
    }

	i3c_dev_free_ibi_controller_locked(dev);

	if (dev->ibi->wq) {
		destroy_workqueue(dev->ibi->wq);
		dev->ibi->wq = NULL;
	}

	kfree(dev->ibi);

Because the hardware interrupt might still fire if disabling fails, couldn't 
a concurrent interrupt handler try to queue work to the destroyed
dev->ibi->wq, resulting in a use-after-free and potential kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903062108.2712296-1-lakshay.piplani@nxp.com?part=2