linux-kernel - Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <415dcaa7-8ad9-492e-5c5c-605173bc4345@axentia.se>
Date:	Wed, 6 Jul 2016 10:22:23 +0200
From:	Peter Rosin <peda@...ntia.se>
To:	Viresh Kumar <viresh.kumar@...aro.org>,
	Wolfram Sang <wsa@...-dreams.de>,
	Jean Delvare <jdelvare@...e.com>
CC:	<linux-i2c@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<gregkh@...uxfoundation.org>, Johan Hovold <johan@...nel.org>,
	Alex Elder <elder@...aro.org>
Subject: Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

On 2016-07-06 04:57, Viresh Kumar wrote:
> The i2c-dev calls i2c_get_adapter() from the .open() callback, which
> doesn't let the adapter device unregister unless the .close() callback
> is called.
> 
> On some platforms (like Google ARA), this doesn't let the modules
> (hardware attached to the phone) eject from the phone as the cleanup
> path for the module hasn't finished yet (i2c adapter not removed).
> 
> We can't let the userspace block the kernel forever in such cases.
> 
> Fix this by calling i2c_get_adapter() from all other file operations,
> i.e.  read/write/ioctl, to make sure the adapter doesn't get away while
> we are in the middle of a operation, but not otherwise. In .open() we
> will release the adapter device before returning and so if there is no
> data transfer in progress, then the i2c-dev doesn't block the adapter
> from unregistering.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
> ---
>  drivers/i2c/i2c-dev.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  include/linux/i2c.h   |  1 +
>  2 files changed, 66 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
> index 66f323fd3982..b2562603daa9 100644
> --- a/drivers/i2c/i2c-dev.c
> +++ b/drivers/i2c/i2c-dev.c
> @@ -142,13 +142,25 @@ static ssize_t i2cdev_read(struct file *file, char __user *buf, size_t count,
>  	int ret;
>  
>  	struct i2c_client *client = file->private_data;
> +	struct i2c_adapter *adap;
> +
> +	adap = i2c_get_adapter(client->adapter_nr);
> +	if (!adap)
> +		return -ENODEV;
> +
> +	if (adap != client->adapter) {
> +		ret = -EINVAL;
> +		goto put_adapter;
> +	}

I don't see how this can work with the i2c-demux-pinctrl driver.
I also wonder if/how other muxes handle this relaxed adapter
lifetime thingy?

Out of curiosity, why would client->adapter change anyway?
(that is, if not because of a demux-pinctrl op)

Or maybe I'm just paranoid, but this is not obvious...

Cheers,
Peter