Re: [PATCH] i2c: core: Prevent race condition when removing i2c devices

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 3114 bytes --]

On Thu, Jun 08, 2017 at 02:43:44PM +1200, David Thomson wrote:
> There's a race condition that occurs when deleting & closing multiple
> i2c devices, which results in a kernel warning on an incorrect reference
> count:

Thanks for the report and patch!

CCing peda. Do you have time to look into it? Muxes and locking are
kinda calling for your expertise ;)

> 
> Call Trace:
> [c00000007e2cfaf0] [c0000000000b84e4] .module_put+0x24/0xb0 (unreliable)
> [c00000007e2cfb70] [c0000000005db830] .i2c_put_adapter+0x30/0x50
> [c00000007e2cfbf0] [c0000000005deb74] .i2cdev_release+0x24/0x60
> [c00000007e2cfc70] [c00000000014015c] .__fput+0xbc/0x290
> [c00000007e2cfd10] [c000000000059b64] .task_work_run+0xf4/0x130
> [c00000007e2cfdb0] [c00000000000a0a4] .do_notify_resume+0x74/0x80
> [c00000007e2cfe30] [c000000000000acc] .ret_from_except_lite+0x78/0x7c
> 
> The race is seen when an i2c adapter is being deleted while
> file descriptor on another i2c adapter is being closed. The following
> shows two threads executing concurrently (the first number is the thread
> id):
> 
> 4580 i2c_del_mux_adapter adapter i2c-6-mux (chan_id 2)
> 4580 i2c_del_adapter adapter i2c-6-mux (chan_id 2)
> 4580 i2cdev_detach_adapter adapter i2c-6-mux (chan_id 2)
> 2627 i2cdev_release START adapter i2c-6-mux (chan_id 2)
> 2627 i2c_put_adapter START adapter i2c-6-mux (chan_id 2) owner (null)
> 2627 i2c_adapter_dev_release adapter i2c-6-mux (chan_id 2)
> 4580 i2c_del_mux_adapter adapter i2c-6-mux (chan_id 3)
> 4580 i2c_del_adapter adapter i2c-6-mux (chan_id 3)
> 4580 i2cdev_detach_adapter adapter i2c-6-mux (chan_id 3)
> 2627 i2c_put_adapter END adapter i2c-6-mux (chan_id 2) owner c0000000edf66400
> 2627 i2cdev_release END adapter i2c-6-mux (chan_id 2)
> 2627 i2cdev_release START adapter i2c-6-mux (chan_id 3)
> 2627 i2c_put_adapter START adapter i2c-6-mux (chan_id 3) owner (null)
> 2627 i2c_adapter_dev_release adapter i2c-6-mux (chan_id 3)
> 2627 i2c_put_adapter END adapter i2c-6-mux (chan_id 3) owner (null)
> 2627 i2cdev_release END adapter i2c-6-mux (chan_id 3)
> 
> When thread 4580 for chan_id 3 interrupts 2627 doing
> i2c_adapter_dev_release, the 'owner' field for chan_id 2 becomes
> non-null. This causes the code to attempt to decrement the reference for
> this owner, but the current reference count is invalid. I'm not sure how
> the owner is getting set, but adding a lock around the
> put_device/module_put calls resolves the issue.
> 
> Signed-off-by: David Thomson <david.thomson@alliedtelesis.co.nz>
> ---
>  drivers/i2c/i2c-core.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 82576aaccc90..8831d9b363a7 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -3151,8 +3151,12 @@ void i2c_put_adapter(struct i2c_adapter *adap)
>  	if (!adap)
>  		return;
>  
> +	mutex_lock(&core_lock);
> +
>  	put_device(&adap->dev);
>  	module_put(adap->owner);
> +
> +	mutex_unlock(&core_lock);
>  }
>  EXPORT_SYMBOL(i2c_put_adapter);
>  
> -- 
> 2.13.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]