| Message ID | 1346703986-7849-1-git-send-email-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | a394d635193b641f2c86ead5ada5b115d57c51f8 |
| Headers | show |
On Mon, Sep 03, 2012 at 01:26:26PM -0700, Guenter Roeck wrote: > Actually, spi_master_put() after spi_alloc_master() must _not_ be followed > by kfree(). The memory is already freed with the call to spi_master_put() > through spi_master_class, which registers a release function. Calling both > spi_master_put() and kfree() results in often nasty (and delayed) crashes > elsewhere in the kernel, often in the networking stack. > > This reverts commit eb4af0f5349235df2e4a5057a72fc8962d00308a. > > Cc: Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> I didn't check the callback, but I introduced eb4af0f5349235df2e4a5057a72fc8962d00308a because I saw the kfree in drivers/spi/spi-imx.c. So I guess this needs fixing, too?! Best regards Uwe > --- > drivers/spi/spi.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index 2d9b5bb..6470750 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -1082,8 +1082,7 @@ static struct class spi_master_class = { > * > * The caller is responsible for assigning the bus number and initializing > * the master's methods before calling spi_register_master(); and (after errors > - * adding the device) calling spi_master_put() and kfree() to prevent a memory > - * leak. > + * adding the device) calling spi_master_put() to prevent a memory leak. > */ > struct spi_master *spi_alloc_master(struct device *dev, unsigned size) > {
On Tue, Sep 04, 2012 at 09:25:23AM +0200, Uwe Kleine-König wrote: > On Mon, Sep 03, 2012 at 01:26:26PM -0700, Guenter Roeck wrote: > > Actually, spi_master_put() after spi_alloc_master() must _not_ be followed > > by kfree(). The memory is already freed with the call to spi_master_put() > > through spi_master_class, which registers a release function. Calling both > > spi_master_put() and kfree() results in often nasty (and delayed) crashes > > elsewhere in the kernel, often in the networking stack. > > > > This reverts commit eb4af0f5349235df2e4a5057a72fc8962d00308a. > > > > Cc: Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > I didn't check the callback, but I introduced > eb4af0f5349235df2e4a5057a72fc8962d00308a because I saw the kfree in > drivers/spi/spi-imx.c. So I guess this needs fixing, too?! > This is a bitbang driver, which is one of the tricky ones which I have not touched yet. That driver calls spi_alloc_master spi_master_get .. error: spi_master_put kfree which works out fine, since there are two references to master (one from spi_alloc_master and one from spi_master_put). Calling spi_master_put twice would be cleaner, of course, but one can not have everything and at least nothing bad will happen. Other bitbang drivers are problematic, though. For example, looking at spi-altera.c: hw->bitbang.master = spi_master_get(master); if (!hw->bitbang.master) return err; That error case should never happen, but if it does the return would leave master unreleased. In practice it is not necessary to check the return code here since it will only be NULL if master is NULL. exit: spi_master_put(master); return err; So this driver calls spi_alloc_master and spi_master_get, but only calls spi_master_put once in the error path, meaning one reference is left and master will not be freed. Given the context, it should really be spi_master_put(hw->bitbang.master); spi_master_put(master); return err; This is just an example. spi-ath79.c and spi-au1550.c are wrong as well, and many others. The issue really is that one must keep track of the number of references to master, and many drivers don't get that right. Bitbang drivers usually call spi_master_get() to get an additional reference to master, and thus must call spi_master_put twice (or spi_master_put/kfree) in the error path. Non-bitbang drivers don't call spi_master_get() and thus don't need the additional call to spi_master_put (or kfree). Guenter ------------------------------------------------------------------------------ Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 2d9b5bb..6470750 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1082,8 +1082,7 @@ static struct class spi_master_class = { * * The caller is responsible for assigning the bus number and initializing * the master's methods before calling spi_register_master(); and (after errors - * adding the device) calling spi_master_put() and kfree() to prevent a memory - * leak. + * adding the device) calling spi_master_put() to prevent a memory leak. */ struct spi_master *spi_alloc_master(struct device *dev, unsigned size) {
Actually, spi_master_put() after spi_alloc_master() must _not_ be followed by kfree(). The memory is already freed with the call to spi_master_put() through spi_master_class, which registers a release function. Calling both spi_master_put() and kfree() results in often nasty (and delayed) crashes elsewhere in the kernel, often in the networking stack. This reverts commit eb4af0f5349235df2e4a5057a72fc8962d00308a. Cc: Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de> Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/spi/spi.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)