Re: users of spi_unregister_controller() broken?

[Vladimir Oltean <olteanv@xxxxxxxxx>]

On Mon, Sep 21, 2020 at 01:00:29PM +0100, Mark Brown wrote:
> On Mon, Sep 21, 2020 at 01:08:05PM +0200, Sascha Hauer wrote:
> 
> > I see the following KASan use-after-free messages from the fsl-dspi
> > driver. It seems that after spi_unregister_controller() has been called
> > no references to the SPI controller device are left and the device is
> > freed in spi_controller_release(). This also frees the driver data
> > structure which is allocated with spi_alloc_master(). Nevertheless all
> > users of spi_unregister_controller() still use their driver data after
> > having called spi_unregister_controller().
> 
> > Any idea what to do about this?
> 
> Drivers that need to use their driver_data in remove() should either use
> devm_spi_register_controller() or not use spi_register_controller() to
> allocate driver_data.  Depending on what the remove function does it may
> not be practical to do the former, most likely it won't be.

Please think of the consequences of making such a change, especially to
so many drivers.

For example, in spi-fsl-dspi.c, the reason why
spi_unregister_controller() is first is:

>From 7684580d45bd3d84ed9b453a4cadf7a9a5605a3f Mon Sep 17 00:00:00 2001
From: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
Date: Mon, 22 Jun 2020 13:05:40 +0200
Subject: [PATCH] spi: spi-fsl-dspi: Fix lockup if device is removed during SPI
 transfer

During device removal, the driver should unregister the SPI controller
and stop the hardware.  Otherwise the dspi_transfer_one_message() could
wait on completion infinitely.

Additionally, calling spi_unregister_controller() first in device
removal reverse-matches the probe function, where SPI controller is
registered at the end.

Fixes: 05209f457069 ("spi: fsl-dspi: add missing clk_disable_unprepare() in dspi_remove()")
Reported-by: Vladimir Oltean <olteanv@xxxxxxxxx>
Signed-off-by: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
Cc: <stable@xxxxxxxxxxxxxxx>
Link: https://lore.kernel.org/r/20200622110543.5035-1-krzk@xxxxxxxxxx
Signed-off-by: Mark Brown <broonie@xxxxxxxxxx>

We want to make sure there is no SPI transaction left in flight when we
remove the driver. So we need to unregister from the SPI core first, to
make it stop sending them.

I understand there is no spi_unregister_controller / spi_free_controller
API as there is for unregister_netdev / free_netdev.

Does it help if you call spi_controller_get(dspi->ctlr) at the beginning
of dspi_remove, and spi_controller_put(dspi->ctlr) at the end of it?

Thanks,
-Vladimir