LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] i2c: drop ancient protection against sysfs refcounting issues
@ 2015-01-19 18:55 Wolfram Sang
  2015-01-19 18:59 ` Pantelis Antoniou
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Wolfram Sang @ 2015-01-19 18:55 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-arm-kernel, linuxppc-dev, linux-mips, Wolfram Sang,
	Greg Kroah-Hartman, Jean Delvare, Julia Lawall,
	Pantelis Antoniou

Back in the days, sysfs seemed to have refcounting issues and subsystems
needed a completion to be safe. This is not the case anymore, so I2C can
get rid of this code. There is noone else besides I2C doing something
like this currently (checked with the attached coccinelle script which
checks if a release function exists and if it contains a completion).

I have been digging through the history of linux.git and
linux-history.git and found that e.g. w1 used to have such a mechanism
and also simply removed it later.

Some more info from Greg Kroah-Hartman:
"Having that call "wait" for the other release call to happen is really
old, as Jean points out, from 2003.  We have "fixed" sysfs since then to
detach the files from the devices easier, we used to have some nasy
reference count issues in that area."

And some testing from Jean Delvare which matches my results:
"However I just tested unloading an i2c bus driver while its adapter's
new_device attribute was opened and rmmod returned immediately. So it
doesn't look like accessing sysfs attributes actually takes a reference
to the underlying i2c_adapter."

Let's get rid of this code before really nobody knows/understands
anymore what this was for and if it has a subtle use.

Reported-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jean Delvare <jdelvare@suse.de>
Cc: Julia Lawall <julia.lawall@lip6.fr>
---

Of course, more testing is appreciated. Here is the coccinelle script:

===

@has_type@
identifier d_type, rel_f;
@@

struct device_type d_type = {
	.release = rel_f,
};

@has_device@
struct device *d;
identifier rel_f, p;
@@

(
	p->dev.release = &rel_f;
|
	d->release = &rel_f;
)

@find_type depends on has_type@
identifier has_type.rel_f, d;
@@

void rel_f(struct device *d)
{
	...
*	complete(...);
	...
}

@find_device depends on has_device@
identifier has_device.rel_f, d;
@@

void rel_f(struct device *d)
{
	...
*	complete(...);
	...
}

===

 drivers/i2c/i2c-core.c | 10 +---------
 include/linux/i2c.h    |  1 -
 2 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 39d25a8cb1ad..15cc5902cf89 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -41,7 +41,6 @@
 #include <linux/of_device.h>
 #include <linux/of_irq.h>
 #include <linux/clk/clk-conf.h>
-#include <linux/completion.h>
 #include <linux/hardirq.h>
 #include <linux/irqflags.h>
 #include <linux/rwsem.h>
@@ -1184,8 +1183,7 @@ EXPORT_SYMBOL_GPL(i2c_new_dummy);
 
 static void i2c_adapter_dev_release(struct device *dev)
 {
-	struct i2c_adapter *adap = to_i2c_adapter(dev);
-	complete(&adap->dev_released);
+	/* empty, but the driver core insists we need a release function */
 }
 
 /*
@@ -1795,14 +1793,8 @@ void i2c_del_adapter(struct i2c_adapter *adap)
 
 	/* device name is gone after device_unregister */
 	dev_dbg(&adap->dev, "adapter [%s] unregistered\n", adap->name);
-
-	/* clean up the sysfs representation */
-	init_completion(&adap->dev_released);
 	device_unregister(&adap->dev);
 
-	/* wait for sysfs to drop all references */
-	wait_for_completion(&adap->dev_released);
-
 	/* free bus id */
 	mutex_lock(&core_lock);
 	idr_remove(&i2c_adapter_idr, adap->nr);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 524b45ea85a2..1df0c20ce648 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -466,7 +466,6 @@ struct i2c_adapter {
 
 	int nr;
 	char name[48];
-	struct completion dev_released;
 
 	struct mutex userspace_clients_lock;
 	struct list_head userspace_clients;
-- 
2.1.3


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] i2c: drop ancient protection against sysfs refcounting issues
  2015-01-19 18:55 [PATCH] i2c: drop ancient protection against sysfs refcounting issues Wolfram Sang
@ 2015-01-19 18:59 ` Pantelis Antoniou
  2015-01-19 19:01 ` Greg Kroah-Hartman
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 14+ messages in thread
From: Pantelis Antoniou @ 2015-01-19 18:59 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Linux Kernel Mailing List, linux-arm-kernel, linuxppc-dev,
	linux-mips, Greg Kroah-Hartman, Jean Delvare, Julia Lawall

Hi Wolfram,

> On Jan 19, 2015, at 20:55 , Wolfram Sang <wsa@the-dreams.de> wrote:
> 
> Back in the days, sysfs seemed to have refcounting issues and subsystems
> needed a completion to be safe. This is not the case anymore, so I2C can
> get rid of this code. There is noone else besides I2C doing something
> like this currently (checked with the attached coccinelle script which
> checks if a release function exists and if it contains a completion).
> 
> I have been digging through the history of linux.git and
> linux-history.git and found that e.g. w1 used to have such a mechanism
> and also simply removed it later.
> 
> Some more info from Greg Kroah-Hartman:
> "Having that call "wait" for the other release call to happen is really
> old, as Jean points out, from 2003.  We have "fixed" sysfs since then to
> detach the files from the devices easier, we used to have some nasy
> reference count issues in that area."
> 
> And some testing from Jean Delvare which matches my results:
> "However I just tested unloading an i2c bus driver while its adapter's
> new_device attribute was opened and rmmod returned immediately. So it
> doesn't look like accessing sysfs attributes actually takes a reference
> to the underlying i2c_adapter."
> 
> Let's get rid of this code before really nobody knows/understands
> anymore what this was for and if it has a subtle use.
> 

Hehe, rather obliquely tested by me too :)

Please save the reference counter hackers sanity and merge this :)

> Reported-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jean Delvare <jdelvare@suse.de>
> Cc: Julia Lawall <julia.lawall@lip6.fr>
> —
> 
Regards

— Pantelis

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] i2c: drop ancient protection against sysfs refcounting issues
  2015-01-19 18:55 [PATCH] i2c: drop ancient protection against sysfs refcounting issues Wolfram Sang
  2015-01-19 18:59 ` Pantelis Antoniou
@ 2015-01-19 19:01 ` Greg Kroah-Hartman
  2015-01-19 21:30   ` Wolfram Sang
  2015-01-19 23:04   ` Russell King - ARM Linux
  2015-01-19 19:12 ` Russell King - ARM Linux
  2015-01-19 20:01 ` Lars-Peter Clausen
  3 siblings, 2 replies; 14+ messages in thread
From: Greg Kroah-Hartman @ 2015-01-19 19:01 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-kernel, linux-arm-kernel, linuxppc-dev, linux-mips,
	Jean Delvare, Julia Lawall, Pantelis Antoniou

On Mon, Jan 19, 2015 at 07:55:56PM +0100, Wolfram Sang wrote:
> Back in the days, sysfs seemed to have refcounting issues and subsystems
> needed a completion to be safe. This is not the case anymore, so I2C can
> get rid of this code. There is noone else besides I2C doing something
> like this currently (checked with the attached coccinelle script which
> checks if a release function exists and if it contains a completion).
> 
> I have been digging through the history of linux.git and
> linux-history.git and found that e.g. w1 used to have such a mechanism
> and also simply removed it later.
> 
> Some more info from Greg Kroah-Hartman:
> "Having that call "wait" for the other release call to happen is really
> old, as Jean points out, from 2003.  We have "fixed" sysfs since then to
> detach the files from the devices easier, we used to have some nasy
> reference count issues in that area."
> 
> And some testing from Jean Delvare which matches my results:
> "However I just tested unloading an i2c bus driver while its adapter's
> new_device attribute was opened and rmmod returned immediately. So it
> doesn't look like accessing sysfs attributes actually takes a reference
> to the underlying i2c_adapter."
> 
> Let's get rid of this code before really nobody knows/understands
> anymore what this was for and if it has a subtle use.
> 
> Reported-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jean Delvare <jdelvare@suse.de>
> Cc: Julia Lawall <julia.lawall@lip6.fr>
> ---
> 
> Of course, more testing is appreciated. Here is the coccinelle script:
> 
> ===
> 
> @has_type@
> identifier d_type, rel_f;
> @@
> 
> struct device_type d_type = {
> 	.release = rel_f,
> };
> 
> @has_device@
> struct device *d;
> identifier rel_f, p;
> @@
> 
> (
> 	p->dev.release = &rel_f;
> |
> 	d->release = &rel_f;
> )
> 
> @find_type depends on has_type@
> identifier has_type.rel_f, d;
> @@
> 
> void rel_f(struct device *d)
> {
> 	...
> *	complete(...);
> 	...
> }
> 
> @find_device depends on has_device@
> identifier has_device.rel_f, d;
> @@
> 
> void rel_f(struct device *d)
> {
> 	...
> *	complete(...);
> 	...
> }
> 
> ===
> 
>  drivers/i2c/i2c-core.c | 10 +---------
>  include/linux/i2c.h    |  1 -
>  2 files changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 39d25a8cb1ad..15cc5902cf89 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -41,7 +41,6 @@
>  #include <linux/of_device.h>
>  #include <linux/of_irq.h>
>  #include <linux/clk/clk-conf.h>
> -#include <linux/completion.h>
>  #include <linux/hardirq.h>
>  #include <linux/irqflags.h>
>  #include <linux/rwsem.h>
> @@ -1184,8 +1183,7 @@ EXPORT_SYMBOL_GPL(i2c_new_dummy);
>  
>  static void i2c_adapter_dev_release(struct device *dev)
>  {
> -	struct i2c_adapter *adap = to_i2c_adapter(dev);
> -	complete(&adap->dev_released);
> +	/* empty, but the driver core insists we need a release function */

Yeah, it does, but I hate to see this in "real" code as something is
probably wrong with it if it happens.

Please move the rest of 'i2c_del_adapter' into the release function
(what was after the wait_for_completion() call), and then all should be
fine.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] i2c: drop ancient protection against sysfs refcounting issues
  2015-01-19 18:55 [PATCH] i2c: drop ancient protection against sysfs refcounting issues Wolfram Sang
  2015-01-19 18:59 ` Pantelis Antoniou
  2015-01-19 19:01 ` Greg Kroah-Hartman
@ 2015-01-19 19:12 ` Russell King - ARM Linux
  2015-01-19 19:39   ` Wolfram Sang
  2015-01-19 20:01 ` Lars-Peter Clausen
  3 siblings, 1 reply; 14+ messages in thread
From: Russell King - ARM Linux @ 2015-01-19 19:12 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-kernel, linux-mips, Greg Kroah-Hartman, Pantelis Antoniou,
	Julia Lawall, linux-arm-kernel, linuxppc-dev, Jean Delvare

On Mon, Jan 19, 2015 at 07:55:56PM +0100, Wolfram Sang wrote:
> Back in the days, sysfs seemed to have refcounting issues and subsystems
> needed a completion to be safe. This is not the case anymore, so I2C can
> get rid of this code. There is noone else besides I2C doing something
> like this currently (checked with the attached coccinelle script which
> checks if a release function exists and if it contains a completion).

Have you validated this with DEBUG_KOBJECT_RELEASE enabled?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] i2c: drop ancient protection against sysfs refcounting issues
  2015-01-19 19:12 ` Russell King - ARM Linux
@ 2015-01-19 19:39   ` Wolfram Sang
  0 siblings, 0 replies; 14+ messages in thread
From: Wolfram Sang @ 2015-01-19 19:39 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: linux-kernel, linux-mips, Greg Kroah-Hartman, Pantelis Antoniou,
	Julia Lawall, linux-arm-kernel, linuxppc-dev, Jean Delvare

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

On Mon, Jan 19, 2015 at 07:12:10PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 19, 2015 at 07:55:56PM +0100, Wolfram Sang wrote:
> > Back in the days, sysfs seemed to have refcounting issues and subsystems
> > needed a completion to be safe. This is not the case anymore, so I2C can
> > get rid of this code. There is noone else besides I2C doing something
> > like this currently (checked with the attached coccinelle script which
> > checks if a release function exists and if it contains a completion).
> 
> Have you validated this with DEBUG_KOBJECT_RELEASE enabled?

You saved me, thank you a lot for this pointer! Patch discarded.

I assume other subsystems also moved away from 'struct device_type' for
the release function as well; but this is just a guess for now and I'll
call it a day for today.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] i2c: drop ancient protection against sysfs refcounting issues
  2015-01-19 18:55 [PATCH] i2c: drop ancient protection against sysfs refcounting issues Wolfram Sang
                   ` (2 preceding siblings ...)
  2015-01-19 19:12 ` Russell King - ARM Linux
@ 2015-01-19 20:01 ` Lars-Peter Clausen
  3 siblings, 0 replies; 14+ messages in thread
From: Lars-Peter Clausen @ 2015-01-19 20:01 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-kernel, linux-arm-kernel, linuxppc-dev, linux-mips,
	Greg Kroah-Hartman, Jean Delvare, Julia Lawall,
	Pantelis Antoniou

On 01/19/2015 07:55 PM, Wolfram Sang wrote:
[...]
>
> Let's get rid of this code before really nobody knows/understands
> anymore what this was for and if it has a subtle use.

Getting rid of this is the right thing, cause it's just not how it should be 
done, but unfortunately it is not as simple as this. The problem is that the 
adapter is typically embedded in the parent device's state struct. This state 
struct is typically freed directly after calling i2c_del_adapter(). If there is 
still something holding a reference to the adapter this will result in a use 
after free. To do this properly i2c_add_adapter() needs to be changed to 
i2c_alloc_adapter() that returns a pointer to a newly allocated adapter. 
i2c_free_adapter() will then only drop a reference to the adapter, but not free 
any memory. Once the last reference has been removed the memory can then be 
freed in the release callback.

The other issue is as long as something has a reference to the adapter they can 
run operations on the adapter. So freeing the adapter also has to make sure 
that any further operations that are performed on the adapter do no longer call 
into the device specific ops, but rather returns -ENODEV, or similar.

- Lars

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] i2c: drop ancient protection against sysfs refcounting issues
  2015-01-19 19:01 ` Greg Kroah-Hartman
@ 2015-01-19 21:30   ` Wolfram Sang
  2015-01-19 23:04   ` Russell King - ARM Linux
  1 sibling, 0 replies; 14+ messages in thread
From: Wolfram Sang @ 2015-01-19 21:30 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, linux-arm-kernel, linuxppc-dev, linux-mips,
	Jean Delvare, Julia Lawall, Pantelis Antoniou

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

> > @@ -1184,8 +1183,7 @@ EXPORT_SYMBOL_GPL(i2c_new_dummy);
> >  
> >  static void i2c_adapter_dev_release(struct device *dev)
> >  {
> > -	struct i2c_adapter *adap = to_i2c_adapter(dev);
> > -	complete(&adap->dev_released);
> > +	/* empty, but the driver core insists we need a release function */
> 
> Yeah, it does, but I hate to see this in "real" code as something is
> probably wrong with it if it happens.
> 
> Please move the rest of 'i2c_del_adapter' into the release function
> (what was after the wait_for_completion() call), and then all should be
> fine.

I was about to do this as a follow-up patch. But Russell's and
Lars-Peter's responses made this obsolete already.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] i2c: drop ancient protection against sysfs refcounting issues
  2015-01-19 19:01 ` Greg Kroah-Hartman
  2015-01-19 21:30   ` Wolfram Sang
@ 2015-01-19 23:04   ` Russell King - ARM Linux
  2015-01-20  1:41     ` Greg Kroah-Hartman
  1 sibling, 1 reply; 14+ messages in thread
From: Russell King - ARM Linux @ 2015-01-19 23:04 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Wolfram Sang, linux-mips, Pantelis Antoniou, linux-kernel,
	Julia Lawall, linux-arm-kernel, linuxppc-dev, Jean Delvare

On Tue, Jan 20, 2015 at 03:01:42AM +0800, Greg Kroah-Hartman wrote:
> On Mon, Jan 19, 2015 at 07:55:56PM +0100, Wolfram Sang wrote:
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index 39d25a8cb1ad..15cc5902cf89 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -41,7 +41,6 @@
> >  #include <linux/of_device.h>
> >  #include <linux/of_irq.h>
> >  #include <linux/clk/clk-conf.h>
> > -#include <linux/completion.h>
> >  #include <linux/hardirq.h>
> >  #include <linux/irqflags.h>
> >  #include <linux/rwsem.h>
> > @@ -1184,8 +1183,7 @@ EXPORT_SYMBOL_GPL(i2c_new_dummy);
> >  
> >  static void i2c_adapter_dev_release(struct device *dev)
> >  {
> > -	struct i2c_adapter *adap = to_i2c_adapter(dev);
> > -	complete(&adap->dev_released);
> > +	/* empty, but the driver core insists we need a release function */
> 
> Yeah, it does, but I hate to see this in "real" code as something is
> probably wrong with it if it happens.
> 
> Please move the rest of 'i2c_del_adapter' into the release function
> (what was after the wait_for_completion() call), and then all should be
> fine.

Are you sure about that?  Some drivers do this, eg,

        i2c_del_adapter(&drv_data->adapter);
        free_irq(drv_data->irq, drv_data);

where drv_data was allocated using devm_kzalloc(), and so will be
released when the ->remove callback (which calls the above
i2c_del_adapter()) returns... freeing the embedded device struct.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] i2c: drop ancient protection against sysfs refcounting issues
  2015-01-19 23:04   ` Russell King - ARM Linux
@ 2015-01-20  1:41     ` Greg Kroah-Hartman
  2015-01-20  7:05       ` Lars-Peter Clausen
  0 siblings, 1 reply; 14+ messages in thread
From: Greg Kroah-Hartman @ 2015-01-20  1:41 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Wolfram Sang, linux-mips, Pantelis Antoniou, linux-kernel,
	Julia Lawall, linux-arm-kernel, linuxppc-dev, Jean Delvare

On Mon, Jan 19, 2015 at 11:04:27PM +0000, Russell King - ARM Linux wrote:
> On Tue, Jan 20, 2015 at 03:01:42AM +0800, Greg Kroah-Hartman wrote:
> > On Mon, Jan 19, 2015 at 07:55:56PM +0100, Wolfram Sang wrote:
> > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > > index 39d25a8cb1ad..15cc5902cf89 100644
> > > --- a/drivers/i2c/i2c-core.c
> > > +++ b/drivers/i2c/i2c-core.c
> > > @@ -41,7 +41,6 @@
> > >  #include <linux/of_device.h>
> > >  #include <linux/of_irq.h>
> > >  #include <linux/clk/clk-conf.h>
> > > -#include <linux/completion.h>
> > >  #include <linux/hardirq.h>
> > >  #include <linux/irqflags.h>
> > >  #include <linux/rwsem.h>
> > > @@ -1184,8 +1183,7 @@ EXPORT_SYMBOL_GPL(i2c_new_dummy);
> > >  
> > >  static void i2c_adapter_dev_release(struct device *dev)
> > >  {
> > > -	struct i2c_adapter *adap = to_i2c_adapter(dev);
> > > -	complete(&adap->dev_released);
> > > +	/* empty, but the driver core insists we need a release function */
> > 
> > Yeah, it does, but I hate to see this in "real" code as something is
> > probably wrong with it if it happens.
> > 
> > Please move the rest of 'i2c_del_adapter' into the release function
> > (what was after the wait_for_completion() call), and then all should be
> > fine.
> 
> Are you sure about that?  Some drivers do this, eg,
> 
>         i2c_del_adapter(&drv_data->adapter);
>         free_irq(drv_data->irq, drv_data);
> 
> where drv_data was allocated using devm_kzalloc(), and so will be
> released when the ->remove callback (which calls the above
> i2c_del_adapter()) returns... freeing the embedded device struct.

But that will fail today if the memory is freed in i2c_del_adapter(), so
there shouldn't be any change in logic here.

Or am I missing something obvious?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] i2c: drop ancient protection against sysfs refcounting issues
  2015-01-20  1:41     ` Greg Kroah-Hartman
@ 2015-01-20  7:05       ` Lars-Peter Clausen
  2015-01-20  7:12         ` Greg Kroah-Hartman
  0 siblings, 1 reply; 14+ messages in thread
From: Lars-Peter Clausen @ 2015-01-20  7:05 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Russell King - ARM Linux, Wolfram Sang, linux-mips,
	Pantelis Antoniou, linux-kernel, Julia Lawall, linux-arm-kernel,
	linuxppc-dev, Jean Delvare

On 01/20/2015 02:41 AM, Greg Kroah-Hartman wrote:
> On Mon, Jan 19, 2015 at 11:04:27PM +0000, Russell King - ARM Linux wrote:
>> On Tue, Jan 20, 2015 at 03:01:42AM +0800, Greg Kroah-Hartman wrote:
>>> On Mon, Jan 19, 2015 at 07:55:56PM +0100, Wolfram Sang wrote:
>>>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>>>> index 39d25a8cb1ad..15cc5902cf89 100644
>>>> --- a/drivers/i2c/i2c-core.c
>>>> +++ b/drivers/i2c/i2c-core.c
>>>> @@ -41,7 +41,6 @@
>>>>   #include <linux/of_device.h>
>>>>   #include <linux/of_irq.h>
>>>>   #include <linux/clk/clk-conf.h>
>>>> -#include <linux/completion.h>
>>>>   #include <linux/hardirq.h>
>>>>   #include <linux/irqflags.h>
>>>>   #include <linux/rwsem.h>
>>>> @@ -1184,8 +1183,7 @@ EXPORT_SYMBOL_GPL(i2c_new_dummy);
>>>>
>>>>   static void i2c_adapter_dev_release(struct device *dev)
>>>>   {
>>>> -	struct i2c_adapter *adap = to_i2c_adapter(dev);
>>>> -	complete(&adap->dev_released);
>>>> +	/* empty, but the driver core insists we need a release function */
>>>
>>> Yeah, it does, but I hate to see this in "real" code as something is
>>> probably wrong with it if it happens.
>>>
>>> Please move the rest of 'i2c_del_adapter' into the release function
>>> (what was after the wait_for_completion() call), and then all should be
>>> fine.
>>
>> Are you sure about that?  Some drivers do this, eg,
>>
>>          i2c_del_adapter(&drv_data->adapter);
>>          free_irq(drv_data->irq, drv_data);
>>
>> where drv_data was allocated using devm_kzalloc(), and so will be
>> released when the ->remove callback (which calls the above
>> i2c_del_adapter()) returns... freeing the embedded device struct.
>
> But that will fail today if the memory is freed in i2c_del_adapter(), so
> there shouldn't be any change in logic here.
>
> Or am I missing something obvious?

The memory is not freed in i2c_del_adapter().


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] i2c: drop ancient protection against sysfs refcounting issues
  2015-01-20  7:05       ` Lars-Peter Clausen
@ 2015-01-20  7:12         ` Greg Kroah-Hartman
  2015-01-20  7:27           ` Lars-Peter Clausen
  2015-01-20 10:17           ` Russell King - ARM Linux
  0 siblings, 2 replies; 14+ messages in thread
From: Greg Kroah-Hartman @ 2015-01-20  7:12 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: Russell King - ARM Linux, Wolfram Sang, linux-mips,
	Pantelis Antoniou, linux-kernel, Julia Lawall, linux-arm-kernel,
	linuxppc-dev, Jean Delvare

On Tue, Jan 20, 2015 at 08:05:20AM +0100, Lars-Peter Clausen wrote:
> On 01/20/2015 02:41 AM, Greg Kroah-Hartman wrote:
> >On Mon, Jan 19, 2015 at 11:04:27PM +0000, Russell King - ARM Linux wrote:
> >>On Tue, Jan 20, 2015 at 03:01:42AM +0800, Greg Kroah-Hartman wrote:
> >>>On Mon, Jan 19, 2015 at 07:55:56PM +0100, Wolfram Sang wrote:
> >>>>diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> >>>>index 39d25a8cb1ad..15cc5902cf89 100644
> >>>>--- a/drivers/i2c/i2c-core.c
> >>>>+++ b/drivers/i2c/i2c-core.c
> >>>>@@ -41,7 +41,6 @@
> >>>>  #include <linux/of_device.h>
> >>>>  #include <linux/of_irq.h>
> >>>>  #include <linux/clk/clk-conf.h>
> >>>>-#include <linux/completion.h>
> >>>>  #include <linux/hardirq.h>
> >>>>  #include <linux/irqflags.h>
> >>>>  #include <linux/rwsem.h>
> >>>>@@ -1184,8 +1183,7 @@ EXPORT_SYMBOL_GPL(i2c_new_dummy);
> >>>>
> >>>>  static void i2c_adapter_dev_release(struct device *dev)
> >>>>  {
> >>>>-	struct i2c_adapter *adap = to_i2c_adapter(dev);
> >>>>-	complete(&adap->dev_released);
> >>>>+	/* empty, but the driver core insists we need a release function */
> >>>
> >>>Yeah, it does, but I hate to see this in "real" code as something is
> >>>probably wrong with it if it happens.
> >>>
> >>>Please move the rest of 'i2c_del_adapter' into the release function
> >>>(what was after the wait_for_completion() call), and then all should be
> >>>fine.
> >>
> >>Are you sure about that?  Some drivers do this, eg,
> >>
> >>         i2c_del_adapter(&drv_data->adapter);
> >>         free_irq(drv_data->irq, drv_data);
> >>
> >>where drv_data was allocated using devm_kzalloc(), and so will be
> >>released when the ->remove callback (which calls the above
> >>i2c_del_adapter()) returns... freeing the embedded device struct.
> >
> >But that will fail today if the memory is freed in i2c_del_adapter(), so
> >there shouldn't be any change in logic here.
> >
> >Or am I missing something obvious?
> 
> The memory is not freed in i2c_del_adapter().

Right, and I'm not saying it should be, just move the existing logic
into the release callback, and the code flow should be the same and we
don't end up with an "empty" release callback.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] i2c: drop ancient protection against sysfs refcounting issues
  2015-01-20  7:12         ` Greg Kroah-Hartman
@ 2015-01-20  7:27           ` Lars-Peter Clausen
  2015-01-20 10:17           ` Russell King - ARM Linux
  1 sibling, 0 replies; 14+ messages in thread
From: Lars-Peter Clausen @ 2015-01-20  7:27 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Russell King - ARM Linux, Wolfram Sang, linux-mips,
	Pantelis Antoniou, linux-kernel, Julia Lawall, linux-arm-kernel,
	linuxppc-dev, Jean Delvare

On 01/20/2015 08:12 AM, Greg Kroah-Hartman wrote:
> On Tue, Jan 20, 2015 at 08:05:20AM +0100, Lars-Peter Clausen wrote:
>> On 01/20/2015 02:41 AM, Greg Kroah-Hartman wrote:
>>> On Mon, Jan 19, 2015 at 11:04:27PM +0000, Russell King - ARM Linux wrote:
>>>> On Tue, Jan 20, 2015 at 03:01:42AM +0800, Greg Kroah-Hartman wrote:
>>>>> On Mon, Jan 19, 2015 at 07:55:56PM +0100, Wolfram Sang wrote:
>>>>>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>>>>>> index 39d25a8cb1ad..15cc5902cf89 100644
>>>>>> --- a/drivers/i2c/i2c-core.c
>>>>>> +++ b/drivers/i2c/i2c-core.c
>>>>>> @@ -41,7 +41,6 @@
>>>>>>   #include <linux/of_device.h>
>>>>>>   #include <linux/of_irq.h>
>>>>>>   #include <linux/clk/clk-conf.h>
>>>>>> -#include <linux/completion.h>
>>>>>>   #include <linux/hardirq.h>
>>>>>>   #include <linux/irqflags.h>
>>>>>>   #include <linux/rwsem.h>
>>>>>> @@ -1184,8 +1183,7 @@ EXPORT_SYMBOL_GPL(i2c_new_dummy);
>>>>>>
>>>>>>   static void i2c_adapter_dev_release(struct device *dev)
>>>>>>   {
>>>>>> -	struct i2c_adapter *adap = to_i2c_adapter(dev);
>>>>>> -	complete(&adap->dev_released);
>>>>>> +	/* empty, but the driver core insists we need a release function */
>>>>>
>>>>> Yeah, it does, but I hate to see this in "real" code as something is
>>>>> probably wrong with it if it happens.
>>>>>
>>>>> Please move the rest of 'i2c_del_adapter' into the release function
>>>>> (what was after the wait_for_completion() call), and then all should be
>>>>> fine.
>>>>
>>>> Are you sure about that?  Some drivers do this, eg,
>>>>
>>>>          i2c_del_adapter(&drv_data->adapter);
>>>>          free_irq(drv_data->irq, drv_data);
>>>>
>>>> where drv_data was allocated using devm_kzalloc(), and so will be
>>>> released when the ->remove callback (which calls the above
>>>> i2c_del_adapter()) returns... freeing the embedded device struct.
>>>
>>> But that will fail today if the memory is freed in i2c_del_adapter(), so
>>> there shouldn't be any change in logic here.
>>>
>>> Or am I missing something obvious?
>>
>> The memory is not freed in i2c_del_adapter().
>
> Right, and I'm not saying it should be, just move the existing logic
> into the release callback, and the code flow should be the same and we
> don't end up with an "empty" release callback.

But the code flow often is.

i2c_del_adapter(&drvdata->adap);
kfree(drvdata);

That wont work anymore if i2c_del_adapter() returns before the last reference 
has been dropped. This needs to be restructured so that the adapter memory can 
be freed by the release callback.



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] i2c: drop ancient protection against sysfs refcounting issues
  2015-01-20  7:12         ` Greg Kroah-Hartman
  2015-01-20  7:27           ` Lars-Peter Clausen
@ 2015-01-20 10:17           ` Russell King - ARM Linux
  2015-01-20 11:35             ` Wolfram Sang
  1 sibling, 1 reply; 14+ messages in thread
From: Russell King - ARM Linux @ 2015-01-20 10:17 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Lars-Peter Clausen, Wolfram Sang, linux-mips, Pantelis Antoniou,
	linux-kernel, Julia Lawall, linux-arm-kernel, linuxppc-dev,
	Jean Delvare

On Tue, Jan 20, 2015 at 03:12:56PM +0800, Greg Kroah-Hartman wrote:
> On Tue, Jan 20, 2015 at 08:05:20AM +0100, Lars-Peter Clausen wrote:
> > On 01/20/2015 02:41 AM, Greg Kroah-Hartman wrote:
> > >On Mon, Jan 19, 2015 at 11:04:27PM +0000, Russell King - ARM Linux wrote:
> > >>On Tue, Jan 20, 2015 at 03:01:42AM +0800, Greg Kroah-Hartman wrote:
> > >>>On Mon, Jan 19, 2015 at 07:55:56PM +0100, Wolfram Sang wrote:
> > >>>>diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > >>>>index 39d25a8cb1ad..15cc5902cf89 100644
> > >>>>--- a/drivers/i2c/i2c-core.c
> > >>>>+++ b/drivers/i2c/i2c-core.c
> > >>>>@@ -41,7 +41,6 @@
> > >>>>  #include <linux/of_device.h>
> > >>>>  #include <linux/of_irq.h>
> > >>>>  #include <linux/clk/clk-conf.h>
> > >>>>-#include <linux/completion.h>
> > >>>>  #include <linux/hardirq.h>
> > >>>>  #include <linux/irqflags.h>
> > >>>>  #include <linux/rwsem.h>
> > >>>>@@ -1184,8 +1183,7 @@ EXPORT_SYMBOL_GPL(i2c_new_dummy);
> > >>>>
> > >>>>  static void i2c_adapter_dev_release(struct device *dev)
> > >>>>  {
> > >>>>-	struct i2c_adapter *adap = to_i2c_adapter(dev);
> > >>>>-	complete(&adap->dev_released);
> > >>>>+	/* empty, but the driver core insists we need a release function */
> > >>>
> > >>>Yeah, it does, but I hate to see this in "real" code as something is
> > >>>probably wrong with it if it happens.
> > >>>
> > >>>Please move the rest of 'i2c_del_adapter' into the release function
> > >>>(what was after the wait_for_completion() call), and then all should be
> > >>>fine.
> > >>
> > >>Are you sure about that?  Some drivers do this, eg,
> > >>
> > >>         i2c_del_adapter(&drv_data->adapter);
> > >>         free_irq(drv_data->irq, drv_data);
> > >>
> > >>where drv_data was allocated using devm_kzalloc(), and so will be
> > >>released when the ->remove callback (which calls the above
> > >>i2c_del_adapter()) returns... freeing the embedded device struct.
> > >
> > >But that will fail today if the memory is freed in i2c_del_adapter(), so
> > >there shouldn't be any change in logic here.
> > >
> > >Or am I missing something obvious?
> > 
> > The memory is not freed in i2c_del_adapter().
> 
> Right, and I'm not saying it should be, just move the existing logic
> into the release callback, and the code flow should be the same and we
> don't end up with an "empty" release callback.

IMHO there are two possibilities here:

1. leave it as-is, where we ensure that the remainder of i2c_del_adapter
   does not complete until the release callback has been called.

2. fix it properly by taking (eg) the netdev approach to i2c_adapter,
   or an alternative solution which results in decoupling the lifetime
   of the struct device from the i2c_adapter.

Either of these would be much better than removing the completion and
then moving a chunk of code to make it "look" safer than it actually is
and thereby introducing potential use-after-free bugs.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] i2c: drop ancient protection against sysfs refcounting issues
  2015-01-20 10:17           ` Russell King - ARM Linux
@ 2015-01-20 11:35             ` Wolfram Sang
  0 siblings, 0 replies; 14+ messages in thread
From: Wolfram Sang @ 2015-01-20 11:35 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Greg Kroah-Hartman, Lars-Peter Clausen, linux-mips,
	Pantelis Antoniou, linux-kernel, Julia Lawall, linux-arm-kernel,
	linuxppc-dev, Jean Delvare

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


> > Right, and I'm not saying it should be, just move the existing logic
> > into the release callback, and the code flow should be the same and we
> > don't end up with an "empty" release callback.

But as Russell says, even if we don't have the empty callback, we still
create the problem shown by DEBUG_KOBJECT_RELEASE which wasn't there
before?

> IMHO there are two possibilities here:
> 
> 1. leave it as-is, where we ensure that the remainder of i2c_del_adapter
>    does not complete until the release callback has been called.
> 
> 2. fix it properly by taking (eg) the netdev approach to i2c_adapter,
>    or an alternative solution which results in decoupling the lifetime
>    of the struct device from the i2c_adapter.
> 
> Either of these would be much better than removing the completion and
> then moving a chunk of code to make it "look" safer than it actually is
> and thereby introducing potential use-after-free bugs.

I agree. As much as I'd love option 2) I don't see that on the horizon.
So, let's keep things as they are. What probably makes sense is to
update the comment with something like this? I took the liberty and used
some wording from Russell:

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index e227dff62a85..1c89a08fae2a 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1778,11 +1778,14 @@ void i2c_del_adapter(struct i2c_adapter *adap)
 	/* device name is gone after device_unregister */
 	dev_dbg(&adap->dev, "adapter [%s] unregistered\n", adap->name);
 
-	/* clean up the sysfs representation */
+	/* wait until all references to the device are gone
+	 *
+	 * FIXME: This is old code and should ideally be replaced by an
+	 * alternative which results in decoupling the lifetime of the struct
+	 * device from the i2c_adapter, like spi or netdev do.
+	 */
 	init_completion(&adap->dev_released);
 	device_unregister(&adap->dev);
-
-	/* wait for sysfs to drop all references */
 	wait_for_completion(&adap->dev_released);
 
 	/* free bus id */


Thanks for all the input, it is very much appreciated!

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2015-01-20 11:35 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-19 18:55 [PATCH] i2c: drop ancient protection against sysfs refcounting issues Wolfram Sang
2015-01-19 18:59 ` Pantelis Antoniou
2015-01-19 19:01 ` Greg Kroah-Hartman
2015-01-19 21:30   ` Wolfram Sang
2015-01-19 23:04   ` Russell King - ARM Linux
2015-01-20  1:41     ` Greg Kroah-Hartman
2015-01-20  7:05       ` Lars-Peter Clausen
2015-01-20  7:12         ` Greg Kroah-Hartman
2015-01-20  7:27           ` Lars-Peter Clausen
2015-01-20 10:17           ` Russell King - ARM Linux
2015-01-20 11:35             ` Wolfram Sang
2015-01-19 19:12 ` Russell King - ARM Linux
2015-01-19 19:39   ` Wolfram Sang
2015-01-19 20:01 ` Lars-Peter Clausen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).