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: <20160706143323.GK2671@ubuntu>
Date:	Wed, 6 Jul 2016 07:33:23 -0700
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	Peter Rosin <peda@...ntia.se>
Cc:	Wolfram Sang <wsa@...-dreams.de>, Jean Delvare <jdelvare@...e.com>,
	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 06-07-16, 10:22, Peter Rosin wrote:
> 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?

I would like to mention here that I am no I2C expert and have limited
knowledge of it :)

I haven't had a look at the muxes implementation earlier, now that I
looked at them, I see that they unregister/register the adapter,
perhaps while switching functionality.

I am not sure though, if this patch will break it or not. And I don't
have a way of testing it out.

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

I didn't mean that it will change, and perhaps we can add a
WARN_ON(adap != client->adapter).

But, thinking about it again now, I think it is possible.

What about this sequence:

- i2c-adap-register (address P1)
- .open(), client->adapter = P1;
- .read/write/ioctl()..
- i2c-adap-unregister (adapter freed)
- i2c-adap-register with same adapter_nr (address P2);
- .read/write/ioctl().

Wouldn't the address differ here ?

-- 
viresh