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: <20160725223140.GE25667@ubuntu>
Date:	Mon, 25 Jul 2016 15:31:40 -0700
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	Jean Delvare <jdelvare@...e.de>
Cc:	Greg KH <gregkh@...uxfoundation.org>,
	Wolfram Sang <wsa@...-dreams.de>, linux-i2c@...r.kernel.org,
	linux-kernel@...r.kernel.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

Hi Jean,

On 25-07-16, 11:39, Jean Delvare wrote:
> The problem is that the patch proposed by Viresh has nothing to do with
> this. It's not adding notifications, just changing the time frame during
> which user-space holds a reference to the i2c (bus) device. The goal as
> I understand it is to allow *prepared* hot-unplug (in the form of
> "rmmod i2c-bus-device-driver" or sysfs-based offlining?) while

Not really. We are concerned about both prepared and Unprepared cases.

This *hacky* patch was useful in case of *unprepared* hot-unplug as well.

Here is the sequence of events:
- open() i2c device from userspace
- do some operations on the device read/write/ioctls() ..
- Module hot-unplugged (*unprepared*)
- Some of the ongoing i2c transactions may just fail, that is fine ..
- Kernel detected the interrupt about module removal and tries to
  cleanup the devices..
- Now, kernel can not remove the i2c device, unless user application
  has closed the file descriptor.

And so kernel is waiting in the driver's ->remove() callback forever.

Also, there is no way to co-ordinate (in Android) with the
Applications using the device.  They can crash or fail out if they
want to, but the kernel shouldn't stop removal of a hardware module in
that case.

> user-space processes have i2c device nodes open. Unprepared hot-unplug
> will still go wrong exactly as it goes now.

> My point is that prepared hot-unplug can already be achieved today
> without any patch.

Yeah, if we have the option of stopping the applications before the
device is gone.

> Or possibly improved by adding a notification
> mechanism. But not by changing the reference holding design.
> 
> Not only the proposed patch does not help and degrades the performance,
> but it breaks assumptions. For example, it would allow an application
> to open an i2c bus, then you remove its driver and load another i2c bus
> driver, which gets the same bus number, and now the application writes
> to a completely different I2C bus segment. The current reference model
> prevents that, on purpose.
> 
> So, again, nack from me.

Yeah, the patch wasn't great and I knew it from the beginning. But we
are looking for a solution that can be accepted and so need advice
from you guys :)

-- 
viresh