On Wed, 28 Jun 2023 at 17:49, Maxime Ripard mripard@kernel.org wrote:

The root devices show some odd behaviours compared to regular "bus" devices that have been probed through the usual mechanism, so let's create kunit tests to exercise those paths and odd cases.

Signed-off-by: Maxime Ripard maxime@cerno.tech

Reviewed-by: David Gow davidgow@google.com

There's definitely an argument that root devices are not supposed to be like regular devices, and so devm_ managed resources aren't supposed to work with them. Either way: - It needs to be documented somewhere (and this test makes for good documentation, IMO). - It should be consistent: if devm_ isn't to be used with root devices, it should fail everywhere, and if it is, it should work in all the cases below.

So I'm all in favour of including this test and making root devices work.

That being said, I am planning to send out a patchset adding a struct kunit_device for use in tests, which will be attached to a "kunit" bus. I think the combination of "fix devm_ with root devices" and "don't recommend root devices as a 'fake' device for testing" is probably the longer-term solution everyone can agree upon?

Cheers, -- David

drivers/base/test/.kunitconfig | 2 + drivers/base/test/Kconfig | 4 ++ drivers/base/test/Makefile | 2 + drivers/base/test/root-device-test.c | 110 +++++++++++++++++++++++++++++++++++ 4 files changed, 118 insertions(+)

diff --git a/drivers/base/test/.kunitconfig b/drivers/base/test/.kunitconfig new file mode 100644 index 000000000000..473923f0998b --- /dev/null +++ b/drivers/base/test/.kunitconfig @@ -0,0 +1,2 @@ +CONFIG_KUNIT=y +CONFIG_DM_KUNIT_TEST=y diff --git a/drivers/base/test/Kconfig b/drivers/base/test/Kconfig index 610a1ba7a467..9d42051f8f8e 100644 --- a/drivers/base/test/Kconfig +++ b/drivers/base/test/Kconfig @@ -9,6 +9,10 @@ config TEST_ASYNC_DRIVER_PROBE

      If unsure say N.

+config DM_KUNIT_TEST

•   tristate "KUnit Tests for the device model" if !KUNIT_ALL_TESTS
  

•   depends on KUNIT
  

Could we add "default KUNIT_ALL_TESTS" here? Or if there's a good reason to not make this run by default, remove the "if !KUNIT_ALL_TESTS" condition above.

config DRIVER_PE_KUNIT_TEST bool "KUnit Tests for property entry API" if !KUNIT_ALL_TESTS depends on KUNIT=y diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile index 7f76fee6f989..d589ca3fa8fc 100644 --- a/drivers/base/test/Makefile +++ b/drivers/base/test/Makefile @@ -1,5 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE) += test_async_driver_probe.o

+obj-$(CONFIG_DM_KUNIT_TEST) += root-device-test.o

obj-$(CONFIG_DRIVER_PE_KUNIT_TEST) += property-entry-test.o CFLAGS_property-entry-test.o += $(DISABLE_STRUCTLEAK_PLUGIN) diff --git a/drivers/base/test/root-device-test.c b/drivers/base/test/root-device-test.c new file mode 100644 index 000000000000..9a3e6cccae13 --- /dev/null +++ b/drivers/base/test/root-device-test.c @@ -0,0 +1,110 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright 2023 Maxime Ripard mripard@kernel.org

+#include <kunit/resource.h>

+#include <linux/device.h>

+#define DEVICE_NAME "test"

+struct test_priv {

•   bool probe_done;
  

•   bool release_done;
  

•   wait_queue_head_t release_wq;
  

•   struct device *dev;
  

+};

+static int root_device_devm_init(struct kunit *test) +{

•   struct test_priv *priv;
  

•   priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
  

•   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
  

•   init_waitqueue_head(&priv->release_wq);
  

•   test->priv = priv;
  

•   return 0;
  

+}

+static void devm_device_action(void *ptr) +{

•   struct test_priv *priv = ptr;
  

•   priv->release_done = true;
  

•   wake_up_interruptible(&priv->release_wq);
  

+}

+#define RELEASE_TIMEOUT_MS 100

+/*

• • Tests that a bus-less, non-probed device will run its device-managed
• • actions when unregistered.
• */

+static void root_device_devm_register_unregister_test(struct kunit *test) +{

•   struct test_priv *priv = test->priv;
  

•   int ret;
  

•   priv->dev = root_device_register(DEVICE_NAME);
  

•   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
  

•   ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
  

•   KUNIT_ASSERT_EQ(test, ret, 0);
  

•   root_device_unregister(priv->dev);
  

•   ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
  

•                                          msecs_to_jiffies(RELEASE_TIMEOUT_MS));
  

•   KUNIT_EXPECT_GT(test, ret, 0);
  

+}

+static void devm_put_device_action(void *ptr) +{

•   struct test_priv *priv = ptr;
  

•   put_device(priv->dev);
  

•   priv->release_done = true;
  

•   wake_up_interruptible(&priv->release_wq);
  

+}

+/*

• • Tests that a bus-less, non-probed device will run its device-managed
• • actions when unregistered, even if someone still holds a reference to
• • it.
• */

+static void root_device_devm_register_get_unregister_with_devm_test(struct kunit *test) +{

•   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);
  

•   get_device(priv->dev);
  

•   ret = devm_add_action_or_reset(priv->dev, devm_put_device_action, priv);
  

•   KUNIT_ASSERT_EQ(test, ret, 0);
  

•   root_device_unregister(priv->dev);
  

•   ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
  

•                                          msecs_to_jiffies(RELEASE_TIMEOUT_MS));
  

•   KUNIT_EXPECT_GT(test, ret, 0);
  

+}

+static struct kunit_case root_device_devm_tests[] = {

•   KUNIT_CASE(root_device_devm_register_unregister_test),
  

•   KUNIT_CASE(root_device_devm_register_get_unregister_with_devm_test),
  

•   {}
  

+};

+static struct kunit_suite root_device_devm_test_suite = {

•   .name = "root-device-devm",
  

•   .init = root_device_devm_init,
  

•   .test_cases = root_device_devm_tests,
  

+};

+kunit_test_suite(root_device_devm_test_suite);

-- 2.40.0

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