[PATCH] Revert "spi/doc: spi_master_put must be followed up by kfree"

[PATCH] Revert "spi/doc: spi_master_put must be followed up by kfree"

[Guenter Roeck]

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(-)

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)
 {
-- 
1.7.9.7

Re: [PATCH] Revert "spi/doc: spi_master_put must be followed up by kfree"

[Uwe Kleine-König]

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)
>  {

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] Revert "spi/doc: spi_master_put must be followed up by kfree"

[Guenter Roeck]

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

Applied "spi: Fix documentation of spi_alloc_master()" to the spi tree

[Mark Brown]

The patch

   spi: Fix documentation of spi_alloc_master()

has been applied to the spi tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From a394d635193b641f2c86ead5ada5b115d57c51f8 Mon Sep 17 00:00:00 2001
From: Guenter Roeck <linux@roeck-us.net>
Date: Sun, 6 Sep 2015 01:46:54 +0300
Subject: [PATCH] spi: Fix documentation of spi_alloc_master()

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.

Link to patch and concerns: https://lkml.org/lkml/2012/9/3/269
or
http://lkml.iu.edu/hypermail/linux/kernel/1209.0/00790.html

Alexey Klimov: This revert becomes valid after
94c69f765f1b4a658d96905ec59928e3e3e07e6a when spi-imx.c
has been fixed and there is no need to call kfree() so comment
for spi_alloc_master() should be fixed.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Alexey Klimov <alexey.klimov@linaro.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
Cc: stable@vger.kernel.org
---
 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 cf8b91b23a76..9ce2f156d382 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1437,8 +1437,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)
 {
-- 
2.5.0