| Message ID | 20230329-kunit-devm-inconsistencies-test-v2-3-19feb71e864b@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 699fb50d99039a50e7494de644f96c889279aca3 |
| Headers | show |
| Series | drivers: base: Add tests showing devm handling inconsistencies | expand |
On Wed, 28 Jun 2023 at 17:50, Maxime Ripard <mripard@kernel.org> wrote: > > From: David Gow <davidgow@google.com> > > In the current code, devres_release_all() only gets called if the device > has a bus and has been probed. > > This leads to issues when using bus-less or driver-less devices where > the device might never get freed if a managed resource holds a reference > to the device. This is happening in the DRM framework for example. > > We should thus call devres_release_all() in the device_del() function to > make sure that the device-managed actions are properly executed when the > device is unregistered, even if it has neither a bus nor a driver. > > This is effectively the same change than commit 2f8d16a996da ("devres: > release resources on device_del()") that got reverted by commit > a525a3ddeaca ("driver core: free devres in device_release") over > use-after-free concerns. > > It's not clear whether those concerns are legitimate though, but I would > expect drivers not to register new resources in their device-managed > actions. It might be clearer to notice that this patch effectively combines the two patches above, freeing _both_ on device_del() and device_release(). This should give us the best of both worlds. I'm not aware of a use-after-free issue that could result here, though it's possible there's a double free I'm missing now that we are freeing things twice. My understanding is that commit a525a3ddeaca ("driver core: free devres in device_release") was more to avoid a leak than a use-after-free, but I could be wrong. > Fixes: a525a3ddeaca ("driver core: free devres in device_release") > Signed-off-by: Maxime Ripard <mripard@kernel.org> > --- Signed-off-by: David Gow <davidgow@google.com> Personally, I feel that this is the right way to go, but I'm definitely not an expert, so I'll let someone else review it in case there's something I'm missing. Cheers, -- David > drivers/base/core.c | 11 +++++++++++ > drivers/base/test/platform-device-test.c | 2 -- > drivers/base/test/root-device-test.c | 2 -- > 3 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 3dff5037943e..6ceaf50f5a67 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -3817,6 +3817,17 @@ void device_del(struct device *dev) > device_platform_notify_remove(dev); > device_links_purge(dev); > > + /* > + * If a device does not have a driver attached, we need to clean > + * up any managed resources. We do this in device_release(), but > + * it's never called (and we leak the device) if a managed > + * resource holds a reference to the device. So release all > + * managed resources here, like we do in driver_detach(). We > + * still need to do so again in device_release() in case someone > + * adds a new resource after this point, though. > + */ > + devres_release_all(dev); > + > bus_notify(dev, BUS_NOTIFY_REMOVED_DEVICE); > kobject_uevent(&dev->kobj, KOBJ_REMOVE); > glue_dir = get_glue_dir(dev); > diff --git a/drivers/base/test/platform-device-test.c b/drivers/base/test/platform-device-test.c > index b6ebf1dcdffb..1ae5ce8bd366 100644 > --- a/drivers/base/test/platform-device-test.c > +++ b/drivers/base/test/platform-device-test.c > @@ -87,8 +87,6 @@ static void platform_device_devm_register_get_unregister_with_devm_test(struct k > struct test_priv *priv = test->priv; > int ret; > > - kunit_skip(test, "This needs to be fixed in the core."); > - > pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE); > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev); > > diff --git a/drivers/base/test/root-device-test.c b/drivers/base/test/root-device-test.c > index 9a3e6cccae13..780d07455f57 100644 > --- a/drivers/base/test/root-device-test.c > +++ b/drivers/base/test/root-device-test.c > @@ -78,8 +78,6 @@ static void root_device_devm_register_get_unregister_with_devm_test(struct kunit > struct test_priv *priv = test->priv; > int ret; > > - kunit_skip(test, "This needs to be fixed in the core."); > - > priv->dev = root_device_register(DEVICE_NAME); > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev); > > > -- > 2.40.0 >
On Wed, Jul 19, 2023 at 05:13:58PM +0800, David Gow wrote: > On Wed, 28 Jun 2023 at 17:50, Maxime Ripard <mripard@kernel.org> wrote: > > > > From: David Gow <davidgow@google.com> > > > > In the current code, devres_release_all() only gets called if the device > > has a bus and has been probed. > > > > This leads to issues when using bus-less or driver-less devices where > > the device might never get freed if a managed resource holds a reference > > to the device. This is happening in the DRM framework for example. > > > > We should thus call devres_release_all() in the device_del() function to > > make sure that the device-managed actions are properly executed when the > > device is unregistered, even if it has neither a bus nor a driver. > > > > This is effectively the same change than commit 2f8d16a996da ("devres: > > release resources on device_del()") that got reverted by commit > > a525a3ddeaca ("driver core: free devres in device_release") over > > use-after-free concerns. > > > > It's not clear whether those concerns are legitimate though, but I would > > expect drivers not to register new resources in their device-managed > > actions. > > It might be clearer to notice that this patch effectively combines the > two patches above, freeing _both_ on device_del() and > device_release(). This should give us the best of both worlds. You're right I'll add that part to the commit log. > I'm not aware of a use-after-free issue that could result here, though > it's possible there's a double free I'm missing now that we are > freeing things twice. My understanding is that commit a525a3ddeaca > ("driver core: free devres in device_release") was more to avoid a > leak than a use-after-free, but I could be wrong. Yeah, I'm not sure where I got the UAF from. I probably misread/misremembered. Maxime
diff --git a/drivers/base/core.c b/drivers/base/core.c index 3dff5037943e..6ceaf50f5a67 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -3817,6 +3817,17 @@ void device_del(struct device *dev) device_platform_notify_remove(dev); device_links_purge(dev); + /* + * If a device does not have a driver attached, we need to clean + * up any managed resources. We do this in device_release(), but + * it's never called (and we leak the device) if a managed + * resource holds a reference to the device. So release all + * managed resources here, like we do in driver_detach(). We + * still need to do so again in device_release() in case someone + * adds a new resource after this point, though. + */ + devres_release_all(dev); + bus_notify(dev, BUS_NOTIFY_REMOVED_DEVICE); kobject_uevent(&dev->kobj, KOBJ_REMOVE); glue_dir = get_glue_dir(dev); diff --git a/drivers/base/test/platform-device-test.c b/drivers/base/test/platform-device-test.c index b6ebf1dcdffb..1ae5ce8bd366 100644 --- a/drivers/base/test/platform-device-test.c +++ b/drivers/base/test/platform-device-test.c @@ -87,8 +87,6 @@ static void platform_device_devm_register_get_unregister_with_devm_test(struct k struct test_priv *priv = test->priv; int ret; - kunit_skip(test, "This needs to be fixed in the core."); - pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev); diff --git a/drivers/base/test/root-device-test.c b/drivers/base/test/root-device-test.c index 9a3e6cccae13..780d07455f57 100644 --- a/drivers/base/test/root-device-test.c +++ b/drivers/base/test/root-device-test.c @@ -78,8 +78,6 @@ static void root_device_devm_register_get_unregister_with_devm_test(struct kunit struct test_priv *priv = test->priv; int ret; - kunit_skip(test, "This needs to be fixed in the core."); - priv->dev = root_device_register(DEVICE_NAME); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);