* [PATCH RESEND] spi: erase pointer to drvdata on removal
@ 2012-12-03 20:28 Vivien Didelot
2012-12-04 14:56 ` Grant Likely
0 siblings, 1 reply; 3+ messages in thread
From: Vivien Didelot @ 2012-12-03 20:28 UTC (permalink / raw)
To: spi-devel-general
Cc: Vivien Didelot, Rob Herring, Grant Likely, linux-kernel,
Andrew Morton, Greg Kroah-Hartman
Without this patch, a SPI device may keep its drvdata whereas it was unbound
from its driver. This may result in accessing an invalid pointer.
As for i2c-core, let the SPI core handle the removal of the device's drvdata,
after a remove(), or a probe() failure.
This is a resent of the previous patch https://lkml.org/lkml/2012/11/1/314
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
drivers/spi/spi.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 84c2861..fe636fe 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -233,15 +233,25 @@ EXPORT_SYMBOL_GPL(spi_bus_type);
static int spi_drv_probe(struct device *dev)
{
const struct spi_driver *sdrv = to_spi_driver(dev->driver);
+ struct spi_device *sdev = to_spi_device(dev);
+ int status;
- return sdrv->probe(to_spi_device(dev));
+ status = sdrv->probe(sdev);
+ if (status)
+ spi_set_drvdata(sdev, NULL);
+ return status;
}
static int spi_drv_remove(struct device *dev)
{
const struct spi_driver *sdrv = to_spi_driver(dev->driver);
+ struct spi_device *sdev = to_spi_device(dev);
+ int status;
- return sdrv->remove(to_spi_device(dev));
+ status = sdrv->remove(sdev);
+ if (status == 0)
+ spi_set_drvdata(sdev, NULL);
+ return status;
}
static void spi_drv_shutdown(struct device *dev)
--
1.7.11.7
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH RESEND] spi: erase pointer to drvdata on removal
2012-12-03 20:28 [PATCH RESEND] spi: erase pointer to drvdata on removal Vivien Didelot
@ 2012-12-04 14:56 ` Grant Likely
2012-12-04 16:39 ` Vivien Didelot
0 siblings, 1 reply; 3+ messages in thread
From: Grant Likely @ 2012-12-04 14:56 UTC (permalink / raw)
To: Vivien Didelot
Cc: spi-devel-general, Rob Herring, Linux Kernel Mailing List,
Andrew Morton, Greg Kroah-Hartman
On Mon, Dec 3, 2012 at 8:28 PM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:
> Without this patch, a SPI device may keep its drvdata whereas it was unbound
> from its driver. This may result in accessing an invalid pointer.
>
> As for i2c-core, let the SPI core handle the removal of the device's drvdata,
> after a remove(), or a probe() failure.
>
> This is a resent of the previous patch https://lkml.org/lkml/2012/11/1/314
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Are you seeing a real bug without this patch? If so, then that is a
driver bug. I don't want to mask over bugs silently. The kernel should
complain.
There are actually lots of things that drivers should/should-not do
that could be checked by the probe/remove hooks, and not just in the
SPI code. spi_set_drvdata() is actually just a wrapper around
dev_set_drvdata().
Instead of clearing the pointer only in the spi code, perhaps the
checks should be in really_probe() and __device_release_driver() so it
covers all bus types. Also, don't clear the pointer. Just use
dev_err() to report on the driver bug so that it gets fixed properly.
g.
> ---
> drivers/spi/spi.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 84c2861..fe636fe 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -233,15 +233,25 @@ EXPORT_SYMBOL_GPL(spi_bus_type);
> static int spi_drv_probe(struct device *dev)
> {
> const struct spi_driver *sdrv = to_spi_driver(dev->driver);
> + struct spi_device *sdev = to_spi_device(dev);
> + int status;
>
> - return sdrv->probe(to_spi_device(dev));
> + status = sdrv->probe(sdev);
> + if (status)
> + spi_set_drvdata(sdev, NULL);
> + return status;
> }
>
> static int spi_drv_remove(struct device *dev)
> {
> const struct spi_driver *sdrv = to_spi_driver(dev->driver);
> + struct spi_device *sdev = to_spi_device(dev);
> + int status;
>
> - return sdrv->remove(to_spi_device(dev));
> + status = sdrv->remove(sdev);
> + if (status == 0)
> + spi_set_drvdata(sdev, NULL);
> + return status;
> }
>
> static void spi_drv_shutdown(struct device *dev)
> --
> 1.7.11.7
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH RESEND] spi: erase pointer to drvdata on removal
2012-12-04 14:56 ` Grant Likely
@ 2012-12-04 16:39 ` Vivien Didelot
0 siblings, 0 replies; 3+ messages in thread
From: Vivien Didelot @ 2012-12-04 16:39 UTC (permalink / raw)
To: Grant Likely
Cc: spi-devel-general, Rob Herring, Linux Kernel Mailing List,
Andrew Morton, Greg Kroah-Hartman
Hi Grant,
On Tue, 2012-12-04 at 14:56 +0000, Grant Likely wrote:
> Instead of clearing the pointer only in the spi code, perhaps the
> checks should be in really_probe() and __device_release_driver() so it
> covers all bus types. Also, don't clear the pointer. Just use
> dev_err() to report on the driver bug so that it gets fixed properly.
I agree with you. I just figured out that on 2012-05-23, Hans de Goede
added a patch (0998d063) to enforce drvdata to be NULL after
device_release or on probe failure. Good to know.
Thanks,
Vivien
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-12-04 16:39 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-12-03 20:28 [PATCH RESEND] spi: erase pointer to drvdata on removal Vivien Didelot
2012-12-04 14:56 ` Grant Likely
2012-12-04 16:39 ` Vivien Didelot
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).