Re: [PATCH] driver core: platform: don't oops on unbound devices
Hello Dmitry,
On Sat, Dec 12, 2020 at 11:49:26PM +0300, Dmitry Baryshkov wrote:
> On Sat, 12 Dec 2020 at 18:39, Uwe Kleine-König
> wrote:
> > On Sat, Dec 12, 2020 at 12:41:32PM +0100, Greg Kroah-Hartman wrote:
> > > On Sat, Dec 12, 2020 at 04:14:26AM +0300, Dmitry Baryshkov wrote:
> > > > Platform code stopped checking if the device is bound to the actual
> > > > platform driver, thus calling non-existing drv->shutdown(). Verify that
> > > > _dev->driver is not NULL before calling remove/shutdown callbacks.
> > > >
> > > > Signed-off-by: Dmitry Baryshkov
> > > > Fixes: 9c30921fe799 ("driver core: platform: use bus_type functions")
> > > > ---
> > > > drivers/base/platform.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > > > index 0358dc3ea3ad..93f44e69b472 100644
> > > > --- a/drivers/base/platform.c
> > > > +++ b/drivers/base/platform.c
> > > > @@ -1342,7 +1342,7 @@ static int platform_remove(struct device *_dev)
> > > > struct platform_device *dev = to_platform_device(_dev);
> > > > int ret = 0;
> > > >
> > > > - if (drv->remove)
> > > > + if (_dev->driver && drv->remove)
> > > > ret = drv->remove(dev);
> > > > dev_pm_domain_detach(_dev, true);
> > >
> > > I don't object to this, but it always feels odd to be doing pointer math
> > > on a NULL value, wait until the static-checkers get ahold of this and
> > > you get crazy emails saying you are crashing the kernel (hint, they are
> > > broken).
> >
> > I think you refer to the line
> >
> > struct platform_driver *drv = to_platform_driver(_dev->driver);
> >
> > which when _dev->driver is NULL results in drv being something really
> > big?!
>
> Yes. To remove pointer math on NULL value I can move the check for
> _dev->driver before calculating drv.
Yeah, that would be good.
> > Accoding to my understanding platform_remove() shouldn't be called if
> > the device isn't bound to a driver.
> >
> > > But, I don't see why this check is needed? If a driver is not bound to
> > > a device, shouldn't this whole function just not be called? Or error
> > > out at the top?
> > >
> > > Uwe, I'd really like your review/ack of this before taking it.
> >
> > So I agree and have the same question. So I wonder: @Dmitry, did you see
> > a crash? When did it happen?
>
> The crash happens in the platform_shutdown() function, which gets
> called for unbound devices after commit 9c30921fe ("driver core:
> platform: use bus_type functions").
> I can include crash trace into v2.
Ah, now I understood. I didn't look too closely on your patch, only on
what Greg quoted. So you added a check to platform_remove (which should
be unnecessary) and to platform_shutdown (where I agree the check is
necessary).
> I added a check to platform_remove() as a safety measure. All current
> calls for dev->bus->remove() in dd.c seem to happen only when
> dev->driver is set, but I thought that it might be a good check. I can
> drop it if you'd like.
Yes, I'd like you to drop this. .remove isn't called for devices without
drivers.
Best regards and thanks for cleaning up after me,
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH] driver core: platform: don't oops on unbound devices
Hello,
On Sat, 12 Dec 2020 at 18:39, Uwe Kleine-König
wrote:
>
> Hello,
>
> On Sat, Dec 12, 2020 at 12:41:32PM +0100, Greg Kroah-Hartman wrote:
> > On Sat, Dec 12, 2020 at 04:14:26AM +0300, Dmitry Baryshkov wrote:
> > > Platform code stopped checking if the device is bound to the actual
> > > platform driver, thus calling non-existing drv->shutdown(). Verify that
> > > _dev->driver is not NULL before calling remove/shutdown callbacks.
> > >
> > > Signed-off-by: Dmitry Baryshkov
> > > Fixes: 9c30921fe799 ("driver core: platform: use bus_type functions")
> > > ---
> > > drivers/base/platform.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > > index 0358dc3ea3ad..93f44e69b472 100644
> > > --- a/drivers/base/platform.c
> > > +++ b/drivers/base/platform.c
> > > @@ -1342,7 +1342,7 @@ static int platform_remove(struct device *_dev)
> > > struct platform_device *dev = to_platform_device(_dev);
> > > int ret = 0;
> > >
> > > - if (drv->remove)
> > > + if (_dev->driver && drv->remove)
> > > ret = drv->remove(dev);
> > > dev_pm_domain_detach(_dev, true);
> >
> > I don't object to this, but it always feels odd to be doing pointer math
> > on a NULL value, wait until the static-checkers get ahold of this and
> > you get crazy emails saying you are crashing the kernel (hint, they are
> > broken).
>
> I think you refer to the line
>
> struct platform_driver *drv = to_platform_driver(_dev->driver);
>
> which when _dev->driver is NULL results in drv being something really
> big?!
Yes. To remove pointer math on NULL value I can move the check for
_dev->driver before calculating drv.
>
> Accoding to my understanding platform_remove() shouldn't be called if
> the device isn't bound to a driver.
>
> > But, I don't see why this check is needed? If a driver is not bound to
> > a device, shouldn't this whole function just not be called? Or error
> > out at the top?
> >
> > Uwe, I'd really like your review/ack of this before taking it.
>
> So I agree and have the same question. So I wonder: @Dmitry, did you see
> a crash? When did it happen?
The crash happens in the platform_shutdown() function, which gets
called for unbound devices after commit 9c30921fe ("driver core:
platform: use bus_type functions").
I can include crash trace into v2.
I added a check to platform_remove() as a safety measure. All current
calls for dev->bus->remove() in dd.c seem to happen only when
dev->driver is set, but I thought that it might be a good check. I can
drop it if you'd like.
>
> For one of the bus types I changed recently
> (arch/powerpc/platforms/ps3/system-bus.c) the bus's shutdown function
> does:
>
> if (drv->shutdown)
> drv->shutdown(dev);
> else if (drv->remove) {
> dev_dbg(>core, ...
> drv->remove(dev);
> } ...
>
> but for the platform bus I'm not aware that remove is used in absence of
> a shutdown callback.
>
> Relevant callers of bus->remove are all in drivers/base/dd.c, and for
> all of them dev->driver should be set.
>
> I look forward to an explaination about why this patch was created.
Here is an explanation: the 3d6a.gmu device is not bound to a
driver, causing a crash during reboot.
[ 57.832972] platform 3d6a000.gmu: shutdown
[ 57.837778] Unable to handle kernel paging request at virtual
address ffe8
[ 57.846391] Mem abort info:
[ 57.849704] ESR = 0x9604
[ 57.853286] EC = 0x25: DABT (current EL), IL = 32 bits
[ 57.859177] SET = 0, FnV = 0
[ 57.862751] EA = 0, S1PTW = 0
[ 57.866415] Data abort info:
[ 57.869801] ISV = 0, ISS = 0x0004
[ 57.874171] CM = 0, WnR = 0
[ 57.877634] swapper pgtable: 4k pages, 48-bit VAs, pgdp=a1646000
[ 57.884937] [ffe8] pgd=, p4d=
[ 57.892323] Internal error: Oops: 9604 [#1] PREEMPT SMP
[ 57.898471] Modules linked in:
[ 57.902022] CPU: 7 PID: 387 Comm: reboot Tainted: GW
5.10.0-rc7-next-20201211-13328-gb9e15b9c1940-dirty #1270
[ 57.914043] Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
[ 57.921340] pstate: 6045 (nZCv daif +PAN -UAO -TCO BTYPE=--)
[ 57.927930] pc : platform_shutdown+0x8/0x34
[ 57.932661] lr : device_shutdown+0x158/0x32c
[ 57.937483] sp : 800010773c70
[ 57.941319] x29: 800010773c70 x28: 14f80c41c600
[ 57.947208] x27: x26: 14f80129c490
[ 57.953100] x25: aa6264ece398 x24: 0008
[ 57.958990] x23: aa62655be030 x22: aa6265671600
[ 57.964875] x21: 14f80122b010 x20: 14f80129c410
[ 57.970765] x19: 14f80129c418 x18: 0030
[ 57.976665] x17: x16: 0001
[ 57.982590] x15: 0004 x14: 019f
[ 57.988478] x13: x12:
[ 57.994394] x11: x10:
Re: [PATCH] driver core: platform: don't oops on unbound devices
Hello,
On Sat, Dec 12, 2020 at 12:41:32PM +0100, Greg Kroah-Hartman wrote:
> On Sat, Dec 12, 2020 at 04:14:26AM +0300, Dmitry Baryshkov wrote:
> > Platform code stopped checking if the device is bound to the actual
> > platform driver, thus calling non-existing drv->shutdown(). Verify that
> > _dev->driver is not NULL before calling remove/shutdown callbacks.
> >
> > Signed-off-by: Dmitry Baryshkov
> > Fixes: 9c30921fe799 ("driver core: platform: use bus_type functions")
> > ---
> > drivers/base/platform.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 0358dc3ea3ad..93f44e69b472 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -1342,7 +1342,7 @@ static int platform_remove(struct device *_dev)
> > struct platform_device *dev = to_platform_device(_dev);
> > int ret = 0;
> >
> > - if (drv->remove)
> > + if (_dev->driver && drv->remove)
> > ret = drv->remove(dev);
> > dev_pm_domain_detach(_dev, true);
>
> I don't object to this, but it always feels odd to be doing pointer math
> on a NULL value, wait until the static-checkers get ahold of this and
> you get crazy emails saying you are crashing the kernel (hint, they are
> broken).
I think you refer to the line
struct platform_driver *drv = to_platform_driver(_dev->driver);
which when _dev->driver is NULL results in drv being something really
big?!
Accoding to my understanding platform_remove() shouldn't be called if
the device isn't bound to a driver.
> But, I don't see why this check is needed? If a driver is not bound to
> a device, shouldn't this whole function just not be called? Or error
> out at the top?
>
> Uwe, I'd really like your review/ack of this before taking it.
So I agree and have the same question. So I wonder: @Dmitry, did you see
a crash? When did it happen?
For one of the bus types I changed recently
(arch/powerpc/platforms/ps3/system-bus.c) the bus's shutdown function
does:
if (drv->shutdown)
drv->shutdown(dev);
else if (drv->remove) {
dev_dbg(>core, ...
drv->remove(dev);
} ...
but for the platform bus I'm not aware that remove is used in absence of
a shutdown callback.
Relevant callers of bus->remove are all in drivers/base/dd.c, and for
all of them dev->driver should be set.
I look forward to an explaination about why this patch was created.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH] driver core: platform: don't oops on unbound devices
On Sat, Dec 12, 2020 at 04:14:26AM +0300, Dmitry Baryshkov wrote:
> Platform code stopped checking if the device is bound to the actual
> platform driver, thus calling non-existing drv->shutdown(). Verify that
> _dev->driver is not NULL before calling remove/shutdown callbacks.
>
> Signed-off-by: Dmitry Baryshkov
> Fixes: 9c30921fe799 ("driver core: platform: use bus_type functions")
> ---
> drivers/base/platform.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 0358dc3ea3ad..93f44e69b472 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -1342,7 +1342,7 @@ static int platform_remove(struct device *_dev)
> struct platform_device *dev = to_platform_device(_dev);
> int ret = 0;
>
> - if (drv->remove)
> + if (_dev->driver && drv->remove)
> ret = drv->remove(dev);
> dev_pm_domain_detach(_dev, true);
I don't object to this, but it always feels odd to be doing pointer math
on a NULL value, wait until the static-checkers get ahold of this and
you get crazy emails saying you are crashing the kernel (hint, they are
broken).
But, I don't see why this check is needed? If a driver is not bound to
a device, shouldn't this whole function just not be called? Or error
out at the top?
Uwe, I'd really like your review/ack of this before taking it.
thanks,
greg k-h
[PATCH] driver core: platform: don't oops on unbound devices
Platform code stopped checking if the device is bound to the actual
platform driver, thus calling non-existing drv->shutdown(). Verify that
_dev->driver is not NULL before calling remove/shutdown callbacks.
Signed-off-by: Dmitry Baryshkov
Fixes: 9c30921fe799 ("driver core: platform: use bus_type functions")
---
drivers/base/platform.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 0358dc3ea3ad..93f44e69b472 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -1342,7 +1342,7 @@ static int platform_remove(struct device *_dev)
struct platform_device *dev = to_platform_device(_dev);
int ret = 0;
- if (drv->remove)
+ if (_dev->driver && drv->remove)
ret = drv->remove(dev);
dev_pm_domain_detach(_dev, true);
@@ -1354,7 +1354,7 @@ static void platform_shutdown(struct device *_dev)
struct platform_driver *drv = to_platform_driver(_dev->driver);
struct platform_device *dev = to_platform_device(_dev);
- if (drv->shutdown)
+ if (_dev->driver && drv->shutdown)
drv->shutdown(dev);
}
--
2.29.2
