linux-kernel - Re: [PATCH] i2c: drop ancient protection against sysfs refcounting issues

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: <20150120071256.GA18983@kroah.com>
Date:	Tue, 20 Jan 2015 15:12:56 +0800
From:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:	Lars-Peter Clausen <lars@...afoo.de>
Cc:	Russell King - ARM Linux <linux@....linux.org.uk>,
	Wolfram Sang <wsa@...-dreams.de>, linux-mips@...ux-mips.org,
	Pantelis Antoniou <pantelis.antoniou@...sulko.com>,
	linux-kernel@...r.kernel.org, Julia Lawall <julia.lawall@...6.fr>,
	linux-arm-kernel@...ts.infradead.org,
	linuxppc-dev@...ts.ozlabs.org, Jean Delvare <jdelvare@...e.de>
Subject: Re: [PATCH] i2c: drop ancient protection against sysfs refcounting
 issues

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/