| Message ID | 20211110161754.3312805-1-michael@walle.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: fsl-dspi: use devm_spi_alloc_master() | expand |
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 >
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 >>
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.
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);
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(-)