[PATCH] spi: fsl-dspi: use devm_spi_alloc_master()

[PATCH] spi: fsl-dspi: use devm_spi_alloc_master()

[Michael Walle]

Commit 530b5affc675 ("spi: fsl-dspi: fix use-after-free in remove path")
fixed the use-after-free by manually allocating memory. Nowadays, since
commit 5e844cc37a5c ("spi: Introduce device-managed SPI controller
allocation"), there is a devres version of spi_alloc_master() for exactly
this purpose. Revert the commit which introduced the manual allocation
and use the new devm_spi_alloc_master().

Signed-off-by: Michael Walle <michael@walle.cc>
---
Btw, using the devm_ version of spi_controller_register() doesn't seem to
be a good idea, see commit 8d559a64f00b ("spi: stm32: drop devres version
of spi_register_master").

 drivers/spi/spi-fsl-dspi.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index fd004c9db9dc..29f8a596c8ee 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -1227,17 +1227,11 @@ static int dspi_probe(struct platform_device *pdev)
 	void __iomem *base;
 	bool big_endian;
 
-	dspi = devm_kzalloc(&pdev->dev, sizeof(*dspi), GFP_KERNEL);
-	if (!dspi)
-		return -ENOMEM;
-
-	ctlr = spi_alloc_master(&pdev->dev, 0);
+	ctlr = devm_spi_alloc_master(&pdev->dev, sizeof(struct fsl_dspi));
 	if (!ctlr)
 		return -ENOMEM;
 
-	spi_controller_set_devdata(ctlr, dspi);
-	platform_set_drvdata(pdev, dspi);
-
+	dspi = spi_controller_get_devdata(ctlr);
 	dspi->pdev = pdev;
 	dspi->ctlr = ctlr;
 
@@ -1373,6 +1367,8 @@ static int dspi_probe(struct platform_device *pdev)
 	if (dspi->devtype_data->trans_mode != DSPI_DMA_MODE)
 		ctlr->ptp_sts_supported = true;
 
+	platform_set_drvdata(pdev, ctlr);
+
 	ret = spi_register_controller(ctlr);
 	if (ret != 0) {
 		dev_err(&pdev->dev, "Problem registering DSPI ctlr\n");
@@ -1396,7 +1392,8 @@ static int dspi_probe(struct platform_device *pdev)
 
 static int dspi_remove(struct platform_device *pdev)
 {
-	struct fsl_dspi *dspi = platform_get_drvdata(pdev);
+	struct spi_controller *ctlr = platform_get_drvdata(pdev);
+	struct fsl_dspi *dspi = spi_controller_get_devdata(ctlr);
 
 	/* Disconnect from the SPI framework */
 	spi_unregister_controller(dspi->ctlr);
-- 
2.30.2

Re: [PATCH] spi: fsl-dspi: use devm_spi_alloc_master()

[Vladimir Oltean]

On Wed, Nov 10, 2021 at 05:17:54PM +0100, Michael Walle wrote:
> Commit 530b5affc675 ("spi: fsl-dspi: fix use-after-free in remove path")
> fixed the use-after-free by manually allocating memory. Nowadays, since
> commit 5e844cc37a5c ("spi: Introduce device-managed SPI controller
> allocation"), there is a devres version of spi_alloc_master() for exactly
> this purpose. Revert the commit which introduced the manual allocation
> and use the new devm_spi_alloc_master().
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> Btw, using the devm_ version of spi_controller_register() doesn't seem to

you mean spi_register_controller.

> be a good idea, see commit 8d559a64f00b ("spi: stm32: drop devres version
> of spi_register_master").

And you mention this because? The dspi driver doesn't use
devm_spi_register_controller(). In fact, if it could be made to not use
devres at all, I couldn't be happier. At this stage, the devres wrappers
for SPI are doing more harm than good.

>  drivers/spi/spi-fsl-dspi.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index fd004c9db9dc..29f8a596c8ee 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -1227,17 +1227,11 @@ static int dspi_probe(struct platform_device *pdev)
>  	void __iomem *base;
>  	bool big_endian;
>  
> -	dspi = devm_kzalloc(&pdev->dev, sizeof(*dspi), GFP_KERNEL);
> -	if (!dspi)
> -		return -ENOMEM;
> -
> -	ctlr = spi_alloc_master(&pdev->dev, 0);
> +	ctlr = devm_spi_alloc_master(&pdev->dev, sizeof(struct fsl_dspi));
>  	if (!ctlr)
>  		return -ENOMEM;
>  
> -	spi_controller_set_devdata(ctlr, dspi);
> -	platform_set_drvdata(pdev, dspi);
> -
> +	dspi = spi_controller_get_devdata(ctlr);
>  	dspi->pdev = pdev;
>  	dspi->ctlr = ctlr;
>  
> @@ -1373,6 +1367,8 @@ static int dspi_probe(struct platform_device *pdev)
>  	if (dspi->devtype_data->trans_mode != DSPI_DMA_MODE)
>  		ctlr->ptp_sts_supported = true;
>  
> +	platform_set_drvdata(pdev, ctlr);
> +

Why do you feel a need to change the drvdata from "dspi" to "ctrl"?

>  	ret = spi_register_controller(ctlr);
>  	if (ret != 0) {
>  		dev_err(&pdev->dev, "Problem registering DSPI ctlr\n");
> @@ -1396,7 +1392,8 @@ static int dspi_probe(struct platform_device *pdev)
>  
>  static int dspi_remove(struct platform_device *pdev)
>  {
> -	struct fsl_dspi *dspi = platform_get_drvdata(pdev);
> +	struct spi_controller *ctlr = platform_get_drvdata(pdev);
> +	struct fsl_dspi *dspi = spi_controller_get_devdata(ctlr);
>  
>  	/* Disconnect from the SPI framework */
>  	spi_unregister_controller(dspi->ctlr);
> -- 
> 2.30.2
> 

Re: [PATCH] spi: fsl-dspi: use devm_spi_alloc_master()

[Michael Walle]

Am 2021-11-13 19:06, schrieb Vladimir Oltean:
> On Wed, Nov 10, 2021 at 05:17:54PM +0100, Michael Walle wrote:
>> Commit 530b5affc675 ("spi: fsl-dspi: fix use-after-free in remove 
>> path")
>> fixed the use-after-free by manually allocating memory. Nowadays, 
>> since
>> commit 5e844cc37a5c ("spi: Introduce device-managed SPI controller
>> allocation"), there is a devres version of spi_alloc_master() for 
>> exactly
>> this purpose. Revert the commit which introduced the manual allocation
>> and use the new devm_spi_alloc_master().
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>> Btw, using the devm_ version of spi_controller_register() doesn't seem 
>> to
> 
> you mean spi_register_controller.
> 
>> be a good idea, see commit 8d559a64f00b ("spi: stm32: drop devres 
>> version
>> of spi_register_master").
> 
> And you mention this because?

just in case someone thinks it is a good idea to convert the
spi_register_controller() call.

> The dspi driver doesn't use
> devm_spi_register_controller(). In fact, if it could be made to not use
> devres at all, I couldn't be happier. At this stage, the devres 
> wrappers
> for SPI are doing more harm than good.
> 
>>  drivers/spi/spi-fsl-dspi.c | 15 ++++++---------
>>  1 file changed, 6 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
>> index fd004c9db9dc..29f8a596c8ee 100644
>> --- a/drivers/spi/spi-fsl-dspi.c
>> +++ b/drivers/spi/spi-fsl-dspi.c
>> @@ -1227,17 +1227,11 @@ static int dspi_probe(struct platform_device 
>> *pdev)
>>  	void __iomem *base;
>>  	bool big_endian;
>> 
>> -	dspi = devm_kzalloc(&pdev->dev, sizeof(*dspi), GFP_KERNEL);
>> -	if (!dspi)
>> -		return -ENOMEM;
>> -
>> -	ctlr = spi_alloc_master(&pdev->dev, 0);
>> +	ctlr = devm_spi_alloc_master(&pdev->dev, sizeof(struct fsl_dspi));
>>  	if (!ctlr)
>>  		return -ENOMEM;
>> 
>> -	spi_controller_set_devdata(ctlr, dspi);
>> -	platform_set_drvdata(pdev, dspi);
>> -
>> +	dspi = spi_controller_get_devdata(ctlr);
>>  	dspi->pdev = pdev;
>>  	dspi->ctlr = ctlr;
>> 
>> @@ -1373,6 +1367,8 @@ static int dspi_probe(struct platform_device 
>> *pdev)
>>  	if (dspi->devtype_data->trans_mode != DSPI_DMA_MODE)
>>  		ctlr->ptp_sts_supported = true;
>> 
>> +	platform_set_drvdata(pdev, ctlr);
>> +
> 
> Why do you feel a need to change the drvdata from "dspi" to "ctrl"?

It was just a revert and that was the way it was done before
the patch mentioned above. You're right, dspi is better
here and then the hunk below will be superfluous.

Let me know if I should respin the patch or if you like to
keep the devm_kzalloc() as it is now, because you've mentioned
you don't like the spi devres mappers. During earlier debugging
I just noticed the following comment in drivers/spi/spi.c and
noticed that this driver isn't converted:

/* Release the last reference on the controller if its driver
  * has not yet been converted to devm_spi_alloc_master/slave().
  */

-michael

> 
>>  	ret = spi_register_controller(ctlr);
>>  	if (ret != 0) {
>>  		dev_err(&pdev->dev, "Problem registering DSPI ctlr\n");
>> @@ -1396,7 +1392,8 @@ static int dspi_probe(struct platform_device 
>> *pdev)
>> 
>>  static int dspi_remove(struct platform_device *pdev)
>>  {
>> -	struct fsl_dspi *dspi = platform_get_drvdata(pdev);
>> +	struct spi_controller *ctlr = platform_get_drvdata(pdev);
>> +	struct fsl_dspi *dspi = spi_controller_get_devdata(ctlr);
>> 
>>  	/* Disconnect from the SPI framework */
>>  	spi_unregister_controller(dspi->ctlr);
>> --
>> 2.30.2
>> 

Re: [PATCH] spi: fsl-dspi: use devm_spi_alloc_master()

[Vladimir Oltean]

On Sun, Nov 14, 2021 at 04:44:18PM +0100, Michael Walle wrote:
> Let me know if I should respin the patch or if you like to
> keep the devm_kzalloc() as it is now, because you've mentioned
> you don't like the spi devres mappers. During earlier debugging
> I just noticed the following comment in drivers/spi/spi.c and
> noticed that this driver isn't converted:
> 
> /* Release the last reference on the controller if its driver
>  * has not yet been converted to devm_spi_alloc_master/slave().
>  */

I'm not a huge fan of cargo cult refactoring. I admit I haven't followed
Lukas' work too closely and I don't see why drivers _should_ be
converted to devm_spi_alloc_controller. Traditionally, the devres
helpers were riddled with various gotchas related to deregistration
being performed way too late (leading to peripheral drivers being unable
to access their hardware because the controller's ->remove had already
executed). I said that not making any use of devres would be great
because I sadly don't have enough time to keep up with all the
subtleties of the devm_spi_* helpers, and having everything implemented
in a plain and simple way makes errors more obvious for me to reason
about. So if there isn't any technical reason for this change, I'd
personally pass on it, but it's up to Mark, really.