* [PATCH 4.9-stable 1/5] spi: Introduce device-managed SPI controller allocation
@ 2020-12-06 12:46 Lukas Wunner
2020-12-06 12:46 ` [PATCH 4.9-stable 2/5] spi: bcm-qspi: Fix use-after-free on unbind Lukas Wunner
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Lukas Wunner @ 2020-12-06 12:46 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Mark Brown, Sudip Mukherjee, stable
[ Upstream commit 5e844cc37a5cbaa460e68f9a989d321d63088a89 ]
SPI driver probing currently comprises two steps, whereas removal
comprises only one step:
spi_alloc_master()
spi_register_master()
spi_unregister_master()
That's because spi_unregister_master() calls device_unregister()
instead of device_del(), thereby releasing the reference on the
spi_master which was obtained by spi_alloc_master().
An SPI driver's private data is contained in the same memory allocation
as the spi_master struct. Thus, once spi_unregister_master() has been
called, the private data is inaccessible. But some drivers need to
access it after spi_unregister_master() to perform further teardown
steps.
Introduce devm_spi_alloc_master(), which releases a reference on the
spi_master struct only after the driver has unbound, thereby keeping the
memory allocation accessible. Change spi_unregister_master() to not
release a reference if the spi_master was allocated by the new devm
function.
The present commit is small enough to be backportable to stable.
It allows fixing drivers which use the private data in their ->remove()
hook after it's been freed. It also allows fixing drivers which neglect
to release a reference on the spi_master in the probe error path.
Long-term, most SPI drivers shall be moved over to the devm function
introduced herein. The few that can't shall be changed in a treewide
commit to explicitly release the last reference on the master.
That commit shall amend spi_unregister_master() to no longer release
a reference, thereby completing the migration.
As a result, the behaviour will be less surprising and more consistent
with subsystems such as IIO, which also includes the private data in the
allocation of the generic iio_dev struct, but calls device_del() in
iio_device_unregister().
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Link: https://lore.kernel.org/r/272bae2ef08abd21388c98e23729886663d19192.1605121038.git.lukas@wunner.de
Signed-off-by: Mark Brown <broonie@kernel.org>
---
drivers/spi/spi.c | 54 ++++++++++++++++++++++++++++++++++++++++-
include/linux/spi/spi.h | 2 ++
2 files changed, 55 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 3fadc564d781..c7c9ca3178ad 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1827,6 +1827,46 @@ struct spi_master *spi_alloc_master(struct device *dev, unsigned size)
}
EXPORT_SYMBOL_GPL(spi_alloc_master);
+static void devm_spi_release_master(struct device *dev, void *master)
+{
+ spi_master_put(*(struct spi_master **)master);
+}
+
+/**
+ * devm_spi_alloc_master - resource-managed spi_alloc_master()
+ * @dev: physical device of SPI master
+ * @size: how much zeroed driver-private data to allocate
+ * Context: can sleep
+ *
+ * Allocate an SPI master and automatically release a reference on it
+ * when @dev is unbound from its driver. Drivers are thus relieved from
+ * having to call spi_master_put().
+ *
+ * The arguments to this function are identical to spi_alloc_master().
+ *
+ * Return: the SPI master structure on success, else NULL.
+ */
+struct spi_master *devm_spi_alloc_master(struct device *dev, unsigned int size)
+{
+ struct spi_master **ptr, *master;
+
+ ptr = devres_alloc(devm_spi_release_master, sizeof(*ptr),
+ GFP_KERNEL);
+ if (!ptr)
+ return NULL;
+
+ master = spi_alloc_master(dev, size);
+ if (master) {
+ *ptr = master;
+ devres_add(dev, ptr);
+ } else {
+ devres_free(ptr);
+ }
+
+ return master;
+}
+EXPORT_SYMBOL_GPL(devm_spi_alloc_master);
+
#ifdef CONFIG_OF
static int of_spi_register_master(struct spi_master *master)
{
@@ -2007,6 +2047,11 @@ int devm_spi_register_master(struct device *dev, struct spi_master *master)
}
EXPORT_SYMBOL_GPL(devm_spi_register_master);
+static int devm_spi_match_master(struct device *dev, void *res, void *master)
+{
+ return *(struct spi_master **)res == master;
+}
+
static int __unregister(struct device *dev, void *null)
{
spi_unregister_device(to_spi_device(dev));
@@ -2036,7 +2081,14 @@ void spi_unregister_master(struct spi_master *master)
list_del(&master->list);
mutex_unlock(&board_lock);
- device_unregister(&master->dev);
+ device_del(&master->dev);
+
+ /* Release the last reference on the master if its driver
+ * has not yet been converted to devm_spi_alloc_master().
+ */
+ if (!devres_find(master->dev.parent, devm_spi_release_master,
+ devm_spi_match_master, master))
+ put_device(&master->dev);
}
EXPORT_SYMBOL_GPL(spi_unregister_master);
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 4b743ac35396..8470695e5dd7 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -601,6 +601,8 @@ extern void spi_finalize_current_transfer(struct spi_master *master);
/* the spi driver core manages memory for the spi_master classdev */
extern struct spi_master *
spi_alloc_master(struct device *host, unsigned size);
+extern struct spi_master *
+devm_spi_alloc_master(struct device *dev, unsigned int size);
extern int spi_register_master(struct spi_master *master);
extern int devm_spi_register_master(struct device *dev,
--
2.29.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 4.9-stable 2/5] spi: bcm-qspi: Fix use-after-free on unbind
2020-12-06 12:46 [PATCH 4.9-stable 1/5] spi: Introduce device-managed SPI controller allocation Lukas Wunner
@ 2020-12-06 12:46 ` Lukas Wunner
2020-12-06 12:46 ` [PATCH 4.9-stable 3/5] spi: bcm2835: " Lukas Wunner
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Lukas Wunner @ 2020-12-06 12:46 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Mark Brown, Sudip Mukherjee, stable
[ Upstream commit 63c5395bb7a9777a33f0e7b5906f2c0170a23692 ]
bcm_qspi_remove() calls spi_unregister_master() even though
bcm_qspi_probe() calls devm_spi_register_master(). The spi_master is
therefore unregistered and freed twice on unbind.
Fix by switching over to the new devm_spi_alloc_master() helper which
keeps the private data accessible until the driver has unbound.
While at it, fix an ordering issue in bcm_qspi_remove() wherein
spi_unregister_master() is called after uninitializing the hardware,
disabling the clock and freeing an IRQ data structure. The correct
order is to call spi_unregister_master() *before* those teardown steps
because bus accesses may still be ongoing until that function returns.
Fixes: fa236a7ef240 ("spi: bcm-qspi: Add Broadcom MSPI driver")
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: <stable@vger.kernel.org> # v4.9+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation
Cc: <stable@vger.kernel.org> # v4.9+
Cc: Kamal Dasu <kdasu.kdev@gmail.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Tested-by: Florian Fainelli <f.fainelli@gmail.com>
Link: https://lore.kernel.org/r/5e31a9a59fd1c0d0b795b2fe219f25e5ee855f9d.1605121038.git.lukas@wunner.de
Signed-off-by: Mark Brown <broonie@kernel.org>
---
drivers/spi/spi-bcm-qspi.c | 34 ++++++++++++----------------------
1 file changed, 12 insertions(+), 22 deletions(-)
diff --git a/drivers/spi/spi-bcm-qspi.c b/drivers/spi/spi-bcm-qspi.c
index 1906b2319e5b..5453910d8abc 100644
--- a/drivers/spi/spi-bcm-qspi.c
+++ b/drivers/spi/spi-bcm-qspi.c
@@ -1185,7 +1185,7 @@ int bcm_qspi_probe(struct platform_device *pdev,
if (!of_match_node(bcm_qspi_of_match, dev->of_node))
return -ENODEV;
- master = spi_alloc_master(dev, sizeof(struct bcm_qspi));
+ master = devm_spi_alloc_master(dev, sizeof(struct bcm_qspi));
if (!master) {
dev_err(dev, "error allocating spi_master\n");
return -ENOMEM;
@@ -1218,21 +1218,17 @@ int bcm_qspi_probe(struct platform_device *pdev,
if (res) {
qspi->base[MSPI] = devm_ioremap_resource(dev, res);
- if (IS_ERR(qspi->base[MSPI])) {
- ret = PTR_ERR(qspi->base[MSPI]);
- goto qspi_resource_err;
- }
+ if (IS_ERR(qspi->base[MSPI]))
+ return PTR_ERR(qspi->base[MSPI]);
} else {
- goto qspi_resource_err;
+ return 0;
}
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "bspi");
if (res) {
qspi->base[BSPI] = devm_ioremap_resource(dev, res);
- if (IS_ERR(qspi->base[BSPI])) {
- ret = PTR_ERR(qspi->base[BSPI]);
- goto qspi_resource_err;
- }
+ if (IS_ERR(qspi->base[BSPI]))
+ return PTR_ERR(qspi->base[BSPI]);
qspi->bspi_mode = true;
} else {
qspi->bspi_mode = false;
@@ -1243,18 +1239,14 @@ int bcm_qspi_probe(struct platform_device *pdev,
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cs_reg");
if (res) {
qspi->base[CHIP_SELECT] = devm_ioremap_resource(dev, res);
- if (IS_ERR(qspi->base[CHIP_SELECT])) {
- ret = PTR_ERR(qspi->base[CHIP_SELECT]);
- goto qspi_resource_err;
- }
+ if (IS_ERR(qspi->base[CHIP_SELECT]))
+ return PTR_ERR(qspi->base[CHIP_SELECT]);
}
qspi->dev_ids = kcalloc(num_irqs, sizeof(struct bcm_qspi_dev_id),
GFP_KERNEL);
- if (!qspi->dev_ids) {
- ret = -ENOMEM;
- goto qspi_resource_err;
- }
+ if (!qspi->dev_ids)
+ return -ENOMEM;
for (val = 0; val < num_irqs; val++) {
irq = -1;
@@ -1330,7 +1322,7 @@ int bcm_qspi_probe(struct platform_device *pdev,
qspi->xfer_mode.addrlen = -1;
qspi->xfer_mode.hp = -1;
- ret = devm_spi_register_master(&pdev->dev, master);
+ ret = spi_register_master(master);
if (ret < 0) {
dev_err(dev, "can't register master\n");
goto qspi_reg_err;
@@ -1343,8 +1335,6 @@ int bcm_qspi_probe(struct platform_device *pdev,
clk_disable_unprepare(qspi->clk);
qspi_probe_err:
kfree(qspi->dev_ids);
-qspi_resource_err:
- spi_master_put(master);
return ret;
}
/* probe function to be called by SoC specific platform driver probe */
@@ -1355,10 +1345,10 @@ int bcm_qspi_remove(struct platform_device *pdev)
struct bcm_qspi *qspi = platform_get_drvdata(pdev);
platform_set_drvdata(pdev, NULL);
+ spi_unregister_master(qspi->master);
bcm_qspi_hw_uninit(qspi);
clk_disable_unprepare(qspi->clk);
kfree(qspi->dev_ids);
- spi_unregister_master(qspi->master);
return 0;
}
--
2.29.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 4.9-stable 3/5] spi: bcm2835: Fix use-after-free on unbind
2020-12-06 12:46 [PATCH 4.9-stable 1/5] spi: Introduce device-managed SPI controller allocation Lukas Wunner
2020-12-06 12:46 ` [PATCH 4.9-stable 2/5] spi: bcm-qspi: Fix use-after-free on unbind Lukas Wunner
@ 2020-12-06 12:46 ` Lukas Wunner
2020-12-06 12:46 ` [PATCH 4.9-stable 4/5] spi: bcm2835aux: " Lukas Wunner
2020-12-06 12:46 ` [PATCH 4.9-stable 5/5] spi: bcm2835: Release the DMA channel if probe fails after dma_init Lukas Wunner
3 siblings, 0 replies; 5+ messages in thread
From: Lukas Wunner @ 2020-12-06 12:46 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Mark Brown, Sudip Mukherjee, stable
[ Upstream commit e1483ac030fb4c57734289742f1c1d38dca61e22 ]
bcm2835_spi_remove() accesses the driver's private data after calling
spi_unregister_controller() even though that function releases the last
reference on the spi_controller and thereby frees the private data.
Fix by switching over to the new devm_spi_alloc_master() helper which
keeps the private data accessible until the driver has unbound.
Fixes: f8043872e796 ("spi: add driver for BCM2835")
Reported-by: Sascha Hauer <s.hauer@pengutronix.de>
Reported-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: <stable@vger.kernel.org> # v3.10+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation
Cc: <stable@vger.kernel.org> # v3.10+
Cc: Vladimir Oltean <olteanv@gmail.com>
Tested-by: Florian Fainelli <f.fainelli@gmail.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Link: https://lore.kernel.org/r/ad66e0a0ad96feb848814842ecf5b6a4539ef35c.1605121038.git.lukas@wunner.de
Signed-off-by: Mark Brown <broonie@kernel.org>
---
drivers/spi/spi-bcm2835.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index df6abc75bc16..2908df35466f 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -737,7 +737,7 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
struct resource *res;
int err;
- master = spi_alloc_master(&pdev->dev, sizeof(*bs));
+ master = devm_spi_alloc_master(&pdev->dev, sizeof(*bs));
if (!master) {
dev_err(&pdev->dev, "spi_alloc_master() failed\n");
return -ENOMEM;
@@ -759,23 +759,20 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
bs->regs = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(bs->regs)) {
- err = PTR_ERR(bs->regs);
- goto out_master_put;
- }
+ if (IS_ERR(bs->regs))
+ return PTR_ERR(bs->regs);
bs->clk = devm_clk_get(&pdev->dev, NULL);
if (IS_ERR(bs->clk)) {
err = PTR_ERR(bs->clk);
dev_err(&pdev->dev, "could not get clk: %d\n", err);
- goto out_master_put;
+ return err;
}
bs->irq = platform_get_irq(pdev, 0);
if (bs->irq <= 0) {
dev_err(&pdev->dev, "could not get IRQ: %d\n", bs->irq);
- err = bs->irq ? bs->irq : -ENODEV;
- goto out_master_put;
+ return bs->irq ? bs->irq : -ENODEV;
}
clk_prepare_enable(bs->clk);
@@ -803,8 +800,6 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
out_clk_disable:
clk_disable_unprepare(bs->clk);
-out_master_put:
- spi_master_put(master);
return err;
}
--
2.29.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 4.9-stable 4/5] spi: bcm2835aux: Fix use-after-free on unbind
2020-12-06 12:46 [PATCH 4.9-stable 1/5] spi: Introduce device-managed SPI controller allocation Lukas Wunner
2020-12-06 12:46 ` [PATCH 4.9-stable 2/5] spi: bcm-qspi: Fix use-after-free on unbind Lukas Wunner
2020-12-06 12:46 ` [PATCH 4.9-stable 3/5] spi: bcm2835: " Lukas Wunner
@ 2020-12-06 12:46 ` Lukas Wunner
2020-12-06 12:46 ` [PATCH 4.9-stable 5/5] spi: bcm2835: Release the DMA channel if probe fails after dma_init Lukas Wunner
3 siblings, 0 replies; 5+ messages in thread
From: Lukas Wunner @ 2020-12-06 12:46 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Mark Brown, Sudip Mukherjee, stable
[ Upstream commit e13ee6cc4781edaf8c7321bee19217e3702ed481 ]
bcm2835aux_spi_remove() accesses the driver's private data after calling
spi_unregister_master() even though that function releases the last
reference on the spi_master and thereby frees the private data.
Fix by switching over to the new devm_spi_alloc_master() helper which
keeps the private data accessible until the driver has unbound.
Fixes: b9dd3f6d4172 ("spi: bcm2835aux: Fix controller unregister order")
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: <stable@vger.kernel.org> # v4.4+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation
Cc: <stable@vger.kernel.org> # v4.4+: b9dd3f6d4172: spi: bcm2835aux: Fix controller unregister order
Cc: <stable@vger.kernel.org> # v4.4+
Link: https://lore.kernel.org/r/b290b06357d0c0bdee9cecc539b840a90630f101.1605121038.git.lukas@wunner.de
Signed-off-by: Mark Brown <broonie@kernel.org>
---
drivers/spi/spi-bcm2835aux.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index b7f78e6d9bec..88772efda830 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -407,7 +407,7 @@ static int bcm2835aux_spi_probe(struct platform_device *pdev)
unsigned long clk_hz;
int err;
- master = spi_alloc_master(&pdev->dev, sizeof(*bs));
+ master = devm_spi_alloc_master(&pdev->dev, sizeof(*bs));
if (!master) {
dev_err(&pdev->dev, "spi_alloc_master() failed\n");
return -ENOMEM;
@@ -439,30 +439,27 @@ static int bcm2835aux_spi_probe(struct platform_device *pdev)
/* the main area */
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
bs->regs = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(bs->regs)) {
- err = PTR_ERR(bs->regs);
- goto out_master_put;
- }
+ if (IS_ERR(bs->regs))
+ return PTR_ERR(bs->regs);
bs->clk = devm_clk_get(&pdev->dev, NULL);
if ((!bs->clk) || (IS_ERR(bs->clk))) {
err = PTR_ERR(bs->clk);
dev_err(&pdev->dev, "could not get clk: %d\n", err);
- goto out_master_put;
+ return err;
}
bs->irq = platform_get_irq(pdev, 0);
if (bs->irq <= 0) {
dev_err(&pdev->dev, "could not get IRQ: %d\n", bs->irq);
- err = bs->irq ? bs->irq : -ENODEV;
- goto out_master_put;
+ return bs->irq ? bs->irq : -ENODEV;
}
/* this also enables the HW block */
err = clk_prepare_enable(bs->clk);
if (err) {
dev_err(&pdev->dev, "could not prepare clock: %d\n", err);
- goto out_master_put;
+ return err;
}
/* just checking if the clock returns a sane value */
@@ -495,8 +492,6 @@ static int bcm2835aux_spi_probe(struct platform_device *pdev)
out_clk_disable:
clk_disable_unprepare(bs->clk);
-out_master_put:
- spi_master_put(master);
return err;
}
--
2.29.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 4.9-stable 5/5] spi: bcm2835: Release the DMA channel if probe fails after dma_init
2020-12-06 12:46 [PATCH 4.9-stable 1/5] spi: Introduce device-managed SPI controller allocation Lukas Wunner
` (2 preceding siblings ...)
2020-12-06 12:46 ` [PATCH 4.9-stable 4/5] spi: bcm2835aux: " Lukas Wunner
@ 2020-12-06 12:46 ` Lukas Wunner
3 siblings, 0 replies; 5+ messages in thread
From: Lukas Wunner @ 2020-12-06 12:46 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Mark Brown, Sudip Mukherjee, stable
From: Peter Ujfalusi <peter.ujfalusi@ti.com>
[ Upstream commit 666224b43b4bd4612ce3b758c038f9bc5c5e3fcb ]
The DMA channel was not released if either devm_request_irq() or
devm_spi_register_controller() failed.
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Reviewed-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
Link: https://lore.kernel.org/r/20191212135550.4634-3-peter.ujfalusi@ti.com
Signed-off-by: Mark Brown <broonie@kernel.org>
[lukas: backport to 4.19-stable]
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
drivers/spi/spi-bcm2835.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index 2908df35466f..6824beae18e4 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -787,18 +787,19 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
dev_name(&pdev->dev), master);
if (err) {
dev_err(&pdev->dev, "could not request IRQ: %d\n", err);
- goto out_clk_disable;
+ goto out_dma_release;
}
err = spi_register_master(master);
if (err) {
dev_err(&pdev->dev, "could not register SPI master: %d\n", err);
- goto out_clk_disable;
+ goto out_dma_release;
}
return 0;
-out_clk_disable:
+out_dma_release:
+ bcm2835_dma_release(master);
clk_disable_unprepare(bs->clk);
return err;
}
--
2.29.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-12-06 12:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-06 12:46 [PATCH 4.9-stable 1/5] spi: Introduce device-managed SPI controller allocation Lukas Wunner
2020-12-06 12:46 ` [PATCH 4.9-stable 2/5] spi: bcm-qspi: Fix use-after-free on unbind Lukas Wunner
2020-12-06 12:46 ` [PATCH 4.9-stable 3/5] spi: bcm2835: " Lukas Wunner
2020-12-06 12:46 ` [PATCH 4.9-stable 4/5] spi: bcm2835aux: " Lukas Wunner
2020-12-06 12:46 ` [PATCH 4.9-stable 5/5] spi: bcm2835: Release the DMA channel if probe fails after dma_init Lukas Wunner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).