[PATCH 0/3] drm/drm_exec, drm/drm_kunit: Fix / WA for uaf and lock alloc tracking.

[PATCH 0/3] drm/drm_exec, drm/drm_kunit: Fix / WA for uaf and lock alloc tracking.

[Thomas Hellström]

While trying to replicate a weird drm_exec lock alloc tracking warning
using the drm_exec kunit test, the warning was shadowed by a UAF warning
due to a bug in the drm kunit helpers.

Patch 1 fixes that drm kunit UAF.
Patch 2 introduces a drm_exec kunit subtest that shows the weird lock
alloc tracking warning if CONFIG_DEBUG_LOCK_ALLOC is set.
Patch 3 modifies drm_exec to avoid that lock alloc tracking warning. Although
the proper fix would probably be to modify lockdep to track the lock held
status differently, this workaround at pushes the problem out of drm_exec,
so that any drm_exec users relying on its refcounting don't run into this.

Cc: Christian König <christian.koenig@amd.com>
Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Danilo Krummrich <dakr@redhat.com>
Cc: dri-devel@lists.freedesktop.org

Thomas Hellström (3):
  drm/kunit: Avoid a driver uaf
  drm/tests/drm_exec: Add a test for object freeing within
    drm_exec_fini()
  drm/drm_exec: Work around a WW mutex lockdep oddity

 drivers/gpu/drm/drm_exec.c            |  2 +-
 drivers/gpu/drm/tests/drm_exec_test.c | 47 +++++++++++++++++++++++++++
 include/drm/drm_exec.h                | 35 +++++++++++++++++---
 include/drm/drm_kunit_helpers.h       |  4 ++-
 4 files changed, 82 insertions(+), 6 deletions(-)

-- 
2.41.0

[PATCH 1/3] drm/kunit: Avoid a driver uaf

[Thomas Hellström]

when using __drm_kunit_helper_alloc_drm_device() the driver may be
dereferenced by device-managed resources up until the device is
freed, which is typically later than the kunit-managed resource code
frees it. Fix this by simply make the driver device-managed as well.

Related KASAN message:
[55272.551542] ==================================================================
[55272.551551] BUG: KASAN: slab-use-after-free in drm_dev_put.part.0+0xd4/0xe0 [drm]
[55272.551603] Read of size 8 at addr ffff888127502828 by task kunit_try_catch/10353

[55272.551612] CPU: 4 PID: 10353 Comm: kunit_try_catch Tainted: G     U           N 6.5.0-rc7+ #155
[55272.551620] Hardware name: ASUS System Product Name/PRIME B560M-A AC, BIOS 0403 01/26/2021
[55272.551626] Call Trace:
[55272.551629]  <TASK>
[55272.551633]  dump_stack_lvl+0x57/0x90
[55272.551639]  print_report+0xcf/0x630
[55272.551645]  ? _raw_spin_lock_irqsave+0x5f/0x70
[55272.551652]  ? drm_dev_put.part.0+0xd4/0xe0 [drm]
[55272.551694]  kasan_report+0xd7/0x110
[55272.551699]  ? drm_dev_put.part.0+0xd4/0xe0 [drm]
[55272.551742]  drm_dev_put.part.0+0xd4/0xe0 [drm]
[55272.551783]  devres_release_all+0x15d/0x1f0
[55272.551790]  ? __pfx_devres_release_all+0x10/0x10
[55272.551797]  device_unbind_cleanup+0x16/0x1a0
[55272.551802]  device_release_driver_internal+0x3e5/0x540
[55272.551808]  ? kobject_put+0x5d/0x4b0
[55272.551814]  bus_remove_device+0x1f1/0x3f0
[55272.551819]  device_del+0x342/0x910
[55272.551826]  ? __pfx_device_del+0x10/0x10
[55272.551830]  ? lock_release+0x339/0x5e0
[55272.551836]  ? kunit_remove_resource+0x128/0x290 [kunit]
[55272.551845]  ? __pfx_lock_release+0x10/0x10
[55272.551851]  platform_device_del.part.0+0x1f/0x1e0
[55272.551856]  ? _raw_spin_unlock_irqrestore+0x30/0x60
[55272.551863]  kunit_remove_resource+0x195/0x290 [kunit]
[55272.551871]  ? _raw_spin_unlock_irqrestore+0x30/0x60
[55272.551877]  kunit_cleanup+0x78/0x120 [kunit]
[55272.551885]  ? __kthread_parkme+0xc1/0x1f0
[55272.551891]  ? __pfx_kunit_try_run_case_cleanup+0x10/0x10 [kunit]
[55272.551900]  ? __pfx_kunit_generic_run_threadfn_adapter+0x10/0x10 [kunit]
[55272.551909]  kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit]
[55272.551919]  kthread+0x2e7/0x3c0
[55272.551924]  ? __pfx_kthread+0x10/0x10
[55272.551929]  ret_from_fork+0x2d/0x70
[55272.551935]  ? __pfx_kthread+0x10/0x10
[55272.551940]  ret_from_fork_asm+0x1b/0x30
[55272.551948]  </TASK>

[55272.551953] Allocated by task 10351:
[55272.551956]  kasan_save_stack+0x1c/0x40
[55272.551962]  kasan_set_track+0x21/0x30
[55272.551966]  __kasan_kmalloc+0x8b/0x90
[55272.551970]  __kmalloc+0x5e/0x160
[55272.551976]  kunit_kmalloc_array+0x1c/0x50 [kunit]
[55272.551984]  drm_exec_test_init+0xfa/0x2c0 [drm_exec_test]
[55272.551991]  kunit_try_run_case+0xdd/0x250 [kunit]
[55272.551999]  kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit]
[55272.552008]  kthread+0x2e7/0x3c0
[55272.552012]  ret_from_fork+0x2d/0x70
[55272.552017]  ret_from_fork_asm+0x1b/0x30

[55272.552024] Freed by task 10353:
[55272.552027]  kasan_save_stack+0x1c/0x40
[55272.552032]  kasan_set_track+0x21/0x30
[55272.552036]  kasan_save_free_info+0x27/0x40
[55272.552041]  __kasan_slab_free+0x106/0x180
[55272.552046]  slab_free_freelist_hook+0xb3/0x160
[55272.552051]  __kmem_cache_free+0xb2/0x290
[55272.552056]  kunit_remove_resource+0x195/0x290 [kunit]
[55272.552064]  kunit_cleanup+0x78/0x120 [kunit]
[55272.552072]  kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit]
[55272.552080]  kthread+0x2e7/0x3c0
[55272.552085]  ret_from_fork+0x2d/0x70
[55272.552089]  ret_from_fork_asm+0x1b/0x30

[55272.552096] The buggy address belongs to the object at ffff888127502800
                which belongs to the cache kmalloc-512 of size 512
[55272.552105] The buggy address is located 40 bytes inside of
                freed 512-byte region [ffff888127502800, ffff888127502a00)

[55272.552115] The buggy address belongs to the physical page:
[55272.552119] page:00000000af6c70ff refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x127500
[55272.552127] head:00000000af6c70ff order:3 entire_mapcount:0 nr_pages_mapped:0 pincount:0
[55272.552133] anon flags: 0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
[55272.552141] page_type: 0xffffffff()
[55272.552145] raw: 0017ffffc0010200 ffff888100042c80 0000000000000000 dead000000000001
[55272.552152] raw: 0000000000000000 0000000080200020 00000001ffffffff 0000000000000000
[55272.552157] page dumped because: kasan: bad access detected

[55272.552163] Memory state around the buggy address:
[55272.552167]  ffff888127502700: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[55272.552173]  ffff888127502780: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[55272.552178] >ffff888127502800: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[55272.552184]                                   ^
[55272.552187]  ffff888127502880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[55272.552193]  ffff888127502900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[55272.552198] ==================================================================
[55272.552203] Disabling lock debugging due to kernel taint

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 include/drm/drm_kunit_helpers.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/drm/drm_kunit_helpers.h b/include/drm/drm_kunit_helpers.h
index 514c8a7a32f0..ba483c87f0e7 100644
--- a/include/drm/drm_kunit_helpers.h
+++ b/include/drm/drm_kunit_helpers.h
@@ -3,6 +3,8 @@
 #ifndef DRM_KUNIT_HELPERS_H_
 #define DRM_KUNIT_HELPERS_H_
 
+#include <linux/device.h>
+
 #include <kunit/test.h>
 
 struct drm_device;
@@ -51,7 +53,7 @@ __drm_kunit_helper_alloc_drm_device(struct kunit *test,
 {
 	struct drm_driver *driver;
 
-	driver = kunit_kzalloc(test, sizeof(*driver), GFP_KERNEL);
+	driver = devm_kzalloc(dev, sizeof(*driver), GFP_KERNEL);
 	KUNIT_ASSERT_NOT_NULL(test, driver);
 
 	driver->driver_features = features;
-- 
2.41.0

[PATCH 2/3] drm/tests/drm_exec: Add a test for object freeing within drm_exec_fini()

[Thomas Hellström]

Check that object freeing from within drm_exec_fini() works as expected
and doesn't generate any warnings.

Cc: Christian König <christian.koenig@amd.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/tests/drm_exec_test.c | 47 +++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_exec_test.c b/drivers/gpu/drm/tests/drm_exec_test.c
index 563949d777dd..294c25f49cc7 100644
--- a/drivers/gpu/drm/tests/drm_exec_test.c
+++ b/drivers/gpu/drm/tests/drm_exec_test.c
@@ -170,6 +170,52 @@ static void test_prepare_array(struct kunit *test)
 	drm_gem_private_object_fini(&gobj2);
 }
 
+static const struct drm_gem_object_funcs put_funcs = {
+	.free = (void *)kfree,
+};
+
+/*
+ * Check that freeing objects from within drm_exec_fini()
+ * behaves as expected.
+ */
+static void test_early_put(struct kunit *test)
+{
+	struct drm_exec_priv *priv = test->priv;
+	struct drm_gem_object *gobj1;
+	struct drm_gem_object *gobj2;
+	struct drm_gem_object *array[2];
+	struct drm_exec exec;
+	int ret;
+
+	gobj1 = kzalloc(sizeof(*gobj1), GFP_KERNEL);
+	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj1);
+	if (!gobj1)
+		return;
+
+	gobj2 = kzalloc(sizeof(*gobj2), GFP_KERNEL);
+	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj2);
+	if (!gobj2) {
+		kfree(gobj1);
+		return;
+	}
+
+	gobj1->funcs = &put_funcs;
+	gobj2->funcs = &put_funcs;
+	array[0] = gobj1;
+	array[1] = gobj2;
+	drm_gem_private_object_init(priv->drm, gobj1, PAGE_SIZE);
+	drm_gem_private_object_init(priv->drm, gobj2, PAGE_SIZE);
+
+	drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT);
+	drm_exec_until_all_locked(&exec)
+		ret = drm_exec_prepare_array(&exec, array, ARRAY_SIZE(array),
+					     1);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+	drm_gem_object_put(gobj1);
+	drm_gem_object_put(gobj2);
+	drm_exec_fini(&exec);
+}
+
 static void test_multiple_loops(struct kunit *test)
 {
 	struct drm_exec exec;
@@ -198,6 +244,7 @@ static struct kunit_case drm_exec_tests[] = {
 	KUNIT_CASE(test_prepare),
 	KUNIT_CASE(test_prepare_array),
 	KUNIT_CASE(test_multiple_loops),
+	KUNIT_CASE(test_early_put),
 	{}
 };
 
-- 
2.41.0

[PATCH 3/3] drm/drm_exec: Work around a WW mutex lockdep oddity

[Thomas Hellström]

If *any* object of a certain WW mutex class is locked, lockdep will
consider *all* mutexes of that class as locked. Also the lock allocation
tracking code will apparently register only the address of the first
mutex locked in a sequence.
This has the odd consequence that if that first mutex is unlocked and
its memory then freed, the lock alloc tracking code will assume that memory
is freed with a held lock in there.

For now, work around that for drm_exec by releasing the first grabbed
object lock last.

Related lock alloc tracking warning:
[  322.660067] =========================
[  322.660070] WARNING: held lock freed!
[  322.660074] 6.5.0-rc7+ #155 Tainted: G     U           N
[  322.660078] -------------------------
[  322.660081] kunit_try_catch/4981 is freeing memory ffff888112adc000-ffff888112adc3ff, with a lock still held there!
[  322.660089] ffff888112adc1a0 (reservation_ww_class_mutex){+.+.}-{3:3}, at: drm_exec_lock_obj+0x11a/0x600 [drm_exec]
[  322.660104] 2 locks held by kunit_try_catch/4981:
[  322.660108]  #0: ffffc9000343fe18 (reservation_ww_class_acquire){+.+.}-{0:0}, at: test_early_put+0x22f/0x490 [drm_exec_test]
[  322.660123]  #1: ffff888112adc1a0 (reservation_ww_class_mutex){+.+.}-{3:3}, at: drm_exec_lock_obj+0x11a/0x600 [drm_exec]
[  322.660135]
               stack backtrace:
[  322.660139] CPU: 7 PID: 4981 Comm: kunit_try_catch Tainted: G     U           N 6.5.0-rc7+ #155
[  322.660146] Hardware name: ASUS System Product Name/PRIME B560M-A AC, BIOS 0403 01/26/2021
[  322.660152] Call Trace:
[  322.660155]  <TASK>
[  322.660158]  dump_stack_lvl+0x57/0x90
[  322.660164]  debug_check_no_locks_freed+0x20b/0x2b0
[  322.660172]  slab_free_freelist_hook+0xa1/0x160
[  322.660179]  ? drm_exec_unlock_all+0x168/0x2a0 [drm_exec]
[  322.660186]  __kmem_cache_free+0xb2/0x290
[  322.660192]  drm_exec_unlock_all+0x168/0x2a0 [drm_exec]
[  322.660200]  drm_exec_fini+0xf/0x1c0 [drm_exec]
[  322.660206]  test_early_put+0x289/0x490 [drm_exec_test]
[  322.660215]  ? __pfx_test_early_put+0x10/0x10 [drm_exec_test]
[  322.660222]  ? __kasan_check_byte+0xf/0x40
[  322.660227]  ? __ksize+0x63/0x140
[  322.660233]  ? drmm_add_final_kfree+0x3e/0xa0 [drm]
[  322.660289]  ? _raw_spin_unlock_irqrestore+0x30/0x60
[  322.660294]  ? lockdep_hardirqs_on+0x7d/0x100
[  322.660301]  ? __pfx_kunit_try_run_case+0x10/0x10 [kunit]
[  322.660310]  ? __pfx_kunit_generic_run_threadfn_adapter+0x10/0x10 [kunit]
[  322.660319]  kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit]
[  322.660328]  kthread+0x2e7/0x3c0
[  322.660334]  ? __pfx_kthread+0x10/0x10
[  322.660339]  ret_from_fork+0x2d/0x70
[  322.660345]  ? __pfx_kthread+0x10/0x10
[  322.660349]  ret_from_fork_asm+0x1b/0x30
[  322.660358]  </TASK>
[  322.660818]     ok 8 test_early_put

Cc: Christian König <christian.koenig@amd.com>
Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Danilo Krummrich <dakr@redhat.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/drm_exec.c |  2 +-
 include/drm/drm_exec.h     | 35 +++++++++++++++++++++++++++++++----
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c
index ff69cf0fb42a..5d2809de4517 100644
--- a/drivers/gpu/drm/drm_exec.c
+++ b/drivers/gpu/drm/drm_exec.c
@@ -56,7 +56,7 @@ static void drm_exec_unlock_all(struct drm_exec *exec)
 	struct drm_gem_object *obj;
 	unsigned long index;
 
-	drm_exec_for_each_locked_object(exec, index, obj) {
+	drm_exec_for_each_locked_object_reverse(exec, index, obj) {
 		dma_resv_unlock(obj->resv);
 		drm_gem_object_put(obj);
 	}
diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
index e0462361adf9..55764cf7c374 100644
--- a/include/drm/drm_exec.h
+++ b/include/drm/drm_exec.h
@@ -51,6 +51,20 @@ struct drm_exec {
 	struct drm_gem_object *prelocked;
 };
 
+/**
+ * drm_exec_obj() - Return the object for a give drm_exec index
+ * @exec: Pointer to the drm_exec context
+ * @index: The index.
+ *
+ * Return: Pointer to the locked object corresponding to @index if
+ * index is within the number of locked objects. NULL otherwise.
+ */
+static inline struct drm_gem_object *
+drm_exec_obj(struct drm_exec *exec, unsigned long index)
+{
+	return index < exec->num_objects ? exec->objects[index] : NULL;
+}
+
 /**
  * drm_exec_for_each_locked_object - iterate over all the locked objects
  * @exec: drm_exec object
@@ -59,10 +73,23 @@ struct drm_exec {
  *
  * Iterate over all the locked GEM objects inside the drm_exec object.
  */
-#define drm_exec_for_each_locked_object(exec, index, obj)	\
-	for (index = 0, obj = (exec)->objects[0];		\
-	     index < (exec)->num_objects;			\
-	     ++index, obj = (exec)->objects[index])
+#define drm_exec_for_each_locked_object(exec, index, obj)		\
+	for ((index) = 0; ((obj) = drm_exec_obj(exec, index)); ++(index))
+
+/**
+ * drm_exec_for_each_locked_object_reverse - iterate over all the locked
+ * objects in reverse locking order
+ * @exec: drm_exec object
+ * @index: unsigned long index for the iteration
+ * @obj: the current GEM object
+ *
+ * Iterate over all the locked GEM objects inside the drm_exec object in
+ * reverse locking order. Note that @index may go below zero and wrap,
+ * but that will be caught by drm_exec_object(), returning a NULL object.
+ */
+#define drm_exec_for_each_locked_object_reverse(exec, index, obj)	\
+	for ((index) = (exec)->num_objects - 1;				\
+	     ((obj) = drm_exec_obj(exec, index)); --(index))
 
 /**
  * drm_exec_until_all_locked - loop until all GEM objects are locked
-- 
2.41.0

Re: [PATCH 3/3] drm/drm_exec: Work around a WW mutex lockdep oddity

[Boris Brezillon]

On Tue,  5 Sep 2023 10:58:32 +0200
Thomas Hellström <thomas.hellstrom@linux.intel.com> wrote:

> If *any* object of a certain WW mutex class is locked, lockdep will
> consider *all* mutexes of that class as locked. Also the lock allocation
> tracking code will apparently register only the address of the first
> mutex locked in a sequence.
> This has the odd consequence that if that first mutex is unlocked and
> its memory then freed, the lock alloc tracking code will assume that memory
> is freed with a held lock in there.
> 
> For now, work around that for drm_exec by releasing the first grabbed
> object lock last.

It's probably a good thing to unlock in reverse order anyway, just like
we do for regular locks.

> 
> Related lock alloc tracking warning:
> [  322.660067] =========================
> [  322.660070] WARNING: held lock freed!
> [  322.660074] 6.5.0-rc7+ #155 Tainted: G     U           N
> [  322.660078] -------------------------
> [  322.660081] kunit_try_catch/4981 is freeing memory ffff888112adc000-ffff888112adc3ff, with a lock still held there!
> [  322.660089] ffff888112adc1a0 (reservation_ww_class_mutex){+.+.}-{3:3}, at: drm_exec_lock_obj+0x11a/0x600 [drm_exec]
> [  322.660104] 2 locks held by kunit_try_catch/4981:
> [  322.660108]  #0: ffffc9000343fe18 (reservation_ww_class_acquire){+.+.}-{0:0}, at: test_early_put+0x22f/0x490 [drm_exec_test]
> [  322.660123]  #1: ffff888112adc1a0 (reservation_ww_class_mutex){+.+.}-{3:3}, at: drm_exec_lock_obj+0x11a/0x600 [drm_exec]
> [  322.660135]
>                stack backtrace:
> [  322.660139] CPU: 7 PID: 4981 Comm: kunit_try_catch Tainted: G     U           N 6.5.0-rc7+ #155
> [  322.660146] Hardware name: ASUS System Product Name/PRIME B560M-A AC, BIOS 0403 01/26/2021
> [  322.660152] Call Trace:
> [  322.660155]  <TASK>
> [  322.660158]  dump_stack_lvl+0x57/0x90
> [  322.660164]  debug_check_no_locks_freed+0x20b/0x2b0
> [  322.660172]  slab_free_freelist_hook+0xa1/0x160
> [  322.660179]  ? drm_exec_unlock_all+0x168/0x2a0 [drm_exec]
> [  322.660186]  __kmem_cache_free+0xb2/0x290
> [  322.660192]  drm_exec_unlock_all+0x168/0x2a0 [drm_exec]
> [  322.660200]  drm_exec_fini+0xf/0x1c0 [drm_exec]
> [  322.660206]  test_early_put+0x289/0x490 [drm_exec_test]
> [  322.660215]  ? __pfx_test_early_put+0x10/0x10 [drm_exec_test]
> [  322.660222]  ? __kasan_check_byte+0xf/0x40
> [  322.660227]  ? __ksize+0x63/0x140
> [  322.660233]  ? drmm_add_final_kfree+0x3e/0xa0 [drm]
> [  322.660289]  ? _raw_spin_unlock_irqrestore+0x30/0x60
> [  322.660294]  ? lockdep_hardirqs_on+0x7d/0x100
> [  322.660301]  ? __pfx_kunit_try_run_case+0x10/0x10 [kunit]
> [  322.660310]  ? __pfx_kunit_generic_run_threadfn_adapter+0x10/0x10 [kunit]
> [  322.660319]  kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit]
> [  322.660328]  kthread+0x2e7/0x3c0
> [  322.660334]  ? __pfx_kthread+0x10/0x10
> [  322.660339]  ret_from_fork+0x2d/0x70
> [  322.660345]  ? __pfx_kthread+0x10/0x10
> [  322.660349]  ret_from_fork_asm+0x1b/0x30
> [  322.660358]  </TASK>
> [  322.660818]     ok 8 test_early_put
> 
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> Cc: Danilo Krummrich <dakr@redhat.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
>  drivers/gpu/drm/drm_exec.c |  2 +-
>  include/drm/drm_exec.h     | 35 +++++++++++++++++++++++++++++++----
>  2 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c
> index ff69cf0fb42a..5d2809de4517 100644
> --- a/drivers/gpu/drm/drm_exec.c
> +++ b/drivers/gpu/drm/drm_exec.c
> @@ -56,7 +56,7 @@ static void drm_exec_unlock_all(struct drm_exec *exec)
>  	struct drm_gem_object *obj;
>  	unsigned long index;
>  
> -	drm_exec_for_each_locked_object(exec, index, obj) {
> +	drm_exec_for_each_locked_object_reverse(exec, index, obj) {
>  		dma_resv_unlock(obj->resv);
>  		drm_gem_object_put(obj);
>  	}
> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
> index e0462361adf9..55764cf7c374 100644
> --- a/include/drm/drm_exec.h
> +++ b/include/drm/drm_exec.h
> @@ -51,6 +51,20 @@ struct drm_exec {
>  	struct drm_gem_object *prelocked;
>  };
>  
> +/**
> + * drm_exec_obj() - Return the object for a give drm_exec index
> + * @exec: Pointer to the drm_exec context
> + * @index: The index.
> + *
> + * Return: Pointer to the locked object corresponding to @index if
> + * index is within the number of locked objects. NULL otherwise.
> + */
> +static inline struct drm_gem_object *
> +drm_exec_obj(struct drm_exec *exec, unsigned long index)
> +{
> +	return index < exec->num_objects ? exec->objects[index] : NULL;
> +}
> +
>  /**
>   * drm_exec_for_each_locked_object - iterate over all the locked objects
>   * @exec: drm_exec object
> @@ -59,10 +73,23 @@ struct drm_exec {
>   *
>   * Iterate over all the locked GEM objects inside the drm_exec object.
>   */
> -#define drm_exec_for_each_locked_object(exec, index, obj)	\
> -	for (index = 0, obj = (exec)->objects[0];		\
> -	     index < (exec)->num_objects;			\
> -	     ++index, obj = (exec)->objects[index])
> +#define drm_exec_for_each_locked_object(exec, index, obj)		\
> +	for ((index) = 0; ((obj) = drm_exec_obj(exec, index)); ++(index))
> +
> +/**
> + * drm_exec_for_each_locked_object_reverse - iterate over all the locked
> + * objects in reverse locking order
> + * @exec: drm_exec object
> + * @index: unsigned long index for the iteration
> + * @obj: the current GEM object
> + *
> + * Iterate over all the locked GEM objects inside the drm_exec object in
> + * reverse locking order. Note that @index may go below zero and wrap,
> + * but that will be caught by drm_exec_object(), returning a NULL object.
> + */
> +#define drm_exec_for_each_locked_object_reverse(exec, index, obj)	\
> +	for ((index) = (exec)->num_objects - 1;				\
> +	     ((obj) = drm_exec_obj(exec, index)); --(index))
>  
>  /**
>   * drm_exec_until_all_locked - loop until all GEM objects are locked

Re: [PATCH 3/3] drm/drm_exec: Work around a WW mutex lockdep oddity

[Danilo Krummrich]

On 9/5/23 10:58, Thomas Hellström wrote:
> If *any* object of a certain WW mutex class is locked, lockdep will
> consider *all* mutexes of that class as locked. Also the lock allocation
> tracking code will apparently register only the address of the first
> mutex locked in a sequence.
> This has the odd consequence that if that first mutex is unlocked and
> its memory then freed, the lock alloc tracking code will assume that memory
> is freed with a held lock in there.
> 
> For now, work around that for drm_exec by releasing the first grabbed
> object lock last.
> 
> Related lock alloc tracking warning:
> [  322.660067] =========================
> [  322.660070] WARNING: held lock freed!
> [  322.660074] 6.5.0-rc7+ #155 Tainted: G     U           N
> [  322.660078] -------------------------
> [  322.660081] kunit_try_catch/4981 is freeing memory ffff888112adc000-ffff888112adc3ff, with a lock still held there!
> [  322.660089] ffff888112adc1a0 (reservation_ww_class_mutex){+.+.}-{3:3}, at: drm_exec_lock_obj+0x11a/0x600 [drm_exec]
> [  322.660104] 2 locks held by kunit_try_catch/4981:
> [  322.660108]  #0: ffffc9000343fe18 (reservation_ww_class_acquire){+.+.}-{0:0}, at: test_early_put+0x22f/0x490 [drm_exec_test]
> [  322.660123]  #1: ffff888112adc1a0 (reservation_ww_class_mutex){+.+.}-{3:3}, at: drm_exec_lock_obj+0x11a/0x600 [drm_exec]
> [  322.660135]
>                 stack backtrace:
> [  322.660139] CPU: 7 PID: 4981 Comm: kunit_try_catch Tainted: G     U           N 6.5.0-rc7+ #155
> [  322.660146] Hardware name: ASUS System Product Name/PRIME B560M-A AC, BIOS 0403 01/26/2021
> [  322.660152] Call Trace:
> [  322.660155]  <TASK>
> [  322.660158]  dump_stack_lvl+0x57/0x90
> [  322.660164]  debug_check_no_locks_freed+0x20b/0x2b0
> [  322.660172]  slab_free_freelist_hook+0xa1/0x160
> [  322.660179]  ? drm_exec_unlock_all+0x168/0x2a0 [drm_exec]
> [  322.660186]  __kmem_cache_free+0xb2/0x290
> [  322.660192]  drm_exec_unlock_all+0x168/0x2a0 [drm_exec]
> [  322.660200]  drm_exec_fini+0xf/0x1c0 [drm_exec]
> [  322.660206]  test_early_put+0x289/0x490 [drm_exec_test]
> [  322.660215]  ? __pfx_test_early_put+0x10/0x10 [drm_exec_test]
> [  322.660222]  ? __kasan_check_byte+0xf/0x40
> [  322.660227]  ? __ksize+0x63/0x140
> [  322.660233]  ? drmm_add_final_kfree+0x3e/0xa0 [drm]
> [  322.660289]  ? _raw_spin_unlock_irqrestore+0x30/0x60
> [  322.660294]  ? lockdep_hardirqs_on+0x7d/0x100
> [  322.660301]  ? __pfx_kunit_try_run_case+0x10/0x10 [kunit]
> [  322.660310]  ? __pfx_kunit_generic_run_threadfn_adapter+0x10/0x10 [kunit]
> [  322.660319]  kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit]
> [  322.660328]  kthread+0x2e7/0x3c0
> [  322.660334]  ? __pfx_kthread+0x10/0x10
> [  322.660339]  ret_from_fork+0x2d/0x70
> [  322.660345]  ? __pfx_kthread+0x10/0x10
> [  322.660349]  ret_from_fork_asm+0x1b/0x30
> [  322.660358]  </TASK>
> [  322.660818]     ok 8 test_early_put
> 
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> Cc: Danilo Krummrich <dakr@redhat.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>

Reviewed-by: Danilo Krummrich <dakr@redhat.com>

One typo below.

> ---
>   drivers/gpu/drm/drm_exec.c |  2 +-
>   include/drm/drm_exec.h     | 35 +++++++++++++++++++++++++++++++----
>   2 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c
> index ff69cf0fb42a..5d2809de4517 100644
> --- a/drivers/gpu/drm/drm_exec.c
> +++ b/drivers/gpu/drm/drm_exec.c
> @@ -56,7 +56,7 @@ static void drm_exec_unlock_all(struct drm_exec *exec)
>   	struct drm_gem_object *obj;
>   	unsigned long index;
>   
> -	drm_exec_for_each_locked_object(exec, index, obj) {
> +	drm_exec_for_each_locked_object_reverse(exec, index, obj) {
>   		dma_resv_unlock(obj->resv);
>   		drm_gem_object_put(obj);
>   	}
> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
> index e0462361adf9..55764cf7c374 100644
> --- a/include/drm/drm_exec.h
> +++ b/include/drm/drm_exec.h
> @@ -51,6 +51,20 @@ struct drm_exec {
>   	struct drm_gem_object *prelocked;
>   };
>   
> +/**
> + * drm_exec_obj() - Return the object for a give drm_exec index
> + * @exec: Pointer to the drm_exec context
> + * @index: The index.
> + *
> + * Return: Pointer to the locked object corresponding to @index if
> + * index is within the number of locked objects. NULL otherwise.
> + */
> +static inline struct drm_gem_object *
> +drm_exec_obj(struct drm_exec *exec, unsigned long index)
> +{
> +	return index < exec->num_objects ? exec->objects[index] : NULL;
> +}
> +
>   /**
>    * drm_exec_for_each_locked_object - iterate over all the locked objects
>    * @exec: drm_exec object
> @@ -59,10 +73,23 @@ struct drm_exec {
>    *
>    * Iterate over all the locked GEM objects inside the drm_exec object.
>    */
> -#define drm_exec_for_each_locked_object(exec, index, obj)	\
> -	for (index = 0, obj = (exec)->objects[0];		\
> -	     index < (exec)->num_objects;			\
> -	     ++index, obj = (exec)->objects[index])
> +#define drm_exec_for_each_locked_object(exec, index, obj)		\
> +	for ((index) = 0; ((obj) = drm_exec_obj(exec, index)); ++(index))
> +
> +/**
> + * drm_exec_for_each_locked_object_reverse - iterate over all the locked
> + * objects in reverse locking order
> + * @exec: drm_exec object
> + * @index: unsigned long index for the iteration
> + * @obj: the current GEM object
> + *
> + * Iterate over all the locked GEM objects inside the drm_exec object in
> + * reverse locking order. Note that @index may go below zero and wrap,
> + * but that will be caught by drm_exec_object(), returning a NULL object.

drm_exec_obj()

> + */
> +#define drm_exec_for_each_locked_object_reverse(exec, index, obj)	\
> +	for ((index) = (exec)->num_objects - 1;				\
> +	     ((obj) = drm_exec_obj(exec, index)); --(index))
>   
>   /**
>    * drm_exec_until_all_locked - loop until all GEM objects are locked

Re: [Intel-xe] [PATCH 2/3] drm/tests/drm_exec: Add a test for object freeing within drm_exec_fini()

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2318 bytes --]

Hi,

On Tue, Sep 05, 2023 at 10:58:31AM +0200, Thomas Hellström wrote:
> Check that object freeing from within drm_exec_fini() works as expected
> and doesn't generate any warnings.
> 
> Cc: Christian König <christian.koenig@amd.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>  drivers/gpu/drm/tests/drm_exec_test.c | 47 +++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tests/drm_exec_test.c b/drivers/gpu/drm/tests/drm_exec_test.c
> index 563949d777dd..294c25f49cc7 100644
> --- a/drivers/gpu/drm/tests/drm_exec_test.c
> +++ b/drivers/gpu/drm/tests/drm_exec_test.c
> @@ -170,6 +170,52 @@ static void test_prepare_array(struct kunit *test)
>  	drm_gem_private_object_fini(&gobj2);
>  }
>  
> +static const struct drm_gem_object_funcs put_funcs = {
> +	.free = (void *)kfree,
> +};
> +
> +/*
> + * Check that freeing objects from within drm_exec_fini()
> + * behaves as expected.
> + */
> +static void test_early_put(struct kunit *test)
> +{
> +	struct drm_exec_priv *priv = test->priv;
> +	struct drm_gem_object *gobj1;
> +	struct drm_gem_object *gobj2;
> +	struct drm_gem_object *array[2];
> +	struct drm_exec exec;
> +	int ret;
> +
> +	gobj1 = kzalloc(sizeof(*gobj1), GFP_KERNEL);
> +	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj1);
> +	if (!gobj1)
> +		return;
> +
> +	gobj2 = kzalloc(sizeof(*gobj2), GFP_KERNEL);
> +	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj2);
> +	if (!gobj2) {
> +		kfree(gobj1);
> +		return;
> +	}
> +
> +	gobj1->funcs = &put_funcs;
> +	gobj2->funcs = &put_funcs;
> +	array[0] = gobj1;
> +	array[1] = gobj2;
> +	drm_gem_private_object_init(priv->drm, gobj1, PAGE_SIZE);
> +	drm_gem_private_object_init(priv->drm, gobj2, PAGE_SIZE);
> +
> +	drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT);
> +	drm_exec_until_all_locked(&exec)
> +		ret = drm_exec_prepare_array(&exec, array, ARRAY_SIZE(array),
> +					     1);
> +	KUNIT_EXPECT_EQ(test, ret, 0);
> +	drm_gem_object_put(gobj1);
> +	drm_gem_object_put(gobj2);
> +	drm_exec_fini(&exec);

It doesn't look like you actually check that "freeing objects from
within drm_exec_fini() behaves as expected." What is the expectation
here, and how is it checked?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [Intel-xe] [PATCH 1/3] drm/kunit: Avoid a driver uaf

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 385 bytes --]

On Tue, Sep 05, 2023 at 10:58:30AM +0200, Thomas Hellström wrote:
> when using __drm_kunit_helper_alloc_drm_device() the driver may be
> dereferenced by device-managed resources up until the device is
> freed, which is typically later than the kunit-managed resource code
> frees it.

I'd like to have a bit more context on how a driver can end up in that
situation?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [Intel-xe] [PATCH 2/3] drm/tests/drm_exec: Add a test for object freeing within drm_exec_fini()

[Thomas Hellström]

Hi,

On 9/5/23 14:05, Maxime Ripard wrote:
> Hi,
>
> On Tue, Sep 05, 2023 at 10:58:31AM +0200, Thomas Hellström wrote:
>> Check that object freeing from within drm_exec_fini() works as expected
>> and doesn't generate any warnings.
>>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: dri-devel@lists.freedesktop.org
>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> ---
>>   drivers/gpu/drm/tests/drm_exec_test.c | 47 +++++++++++++++++++++++++++
>>   1 file changed, 47 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tests/drm_exec_test.c b/drivers/gpu/drm/tests/drm_exec_test.c
>> index 563949d777dd..294c25f49cc7 100644
>> --- a/drivers/gpu/drm/tests/drm_exec_test.c
>> +++ b/drivers/gpu/drm/tests/drm_exec_test.c
>> @@ -170,6 +170,52 @@ static void test_prepare_array(struct kunit *test)
>>   	drm_gem_private_object_fini(&gobj2);
>>   }
>>   
>> +static const struct drm_gem_object_funcs put_funcs = {
>> +	.free = (void *)kfree,
>> +};
>> +
>> +/*
>> + * Check that freeing objects from within drm_exec_fini()
>> + * behaves as expected.
>> + */
>> +static void test_early_put(struct kunit *test)
>> +{
>> +	struct drm_exec_priv *priv = test->priv;
>> +	struct drm_gem_object *gobj1;
>> +	struct drm_gem_object *gobj2;
>> +	struct drm_gem_object *array[2];
>> +	struct drm_exec exec;
>> +	int ret;
>> +
>> +	gobj1 = kzalloc(sizeof(*gobj1), GFP_KERNEL);
>> +	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj1);
>> +	if (!gobj1)
>> +		return;
>> +
>> +	gobj2 = kzalloc(sizeof(*gobj2), GFP_KERNEL);
>> +	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj2);
>> +	if (!gobj2) {
>> +		kfree(gobj1);
>> +		return;
>> +	}
>> +
>> +	gobj1->funcs = &put_funcs;
>> +	gobj2->funcs = &put_funcs;
>> +	array[0] = gobj1;
>> +	array[1] = gobj2;
>> +	drm_gem_private_object_init(priv->drm, gobj1, PAGE_SIZE);
>> +	drm_gem_private_object_init(priv->drm, gobj2, PAGE_SIZE);
>> +
>> +	drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT);
>> +	drm_exec_until_all_locked(&exec)
>> +		ret = drm_exec_prepare_array(&exec, array, ARRAY_SIZE(array),
>> +					     1);
>> +	KUNIT_EXPECT_EQ(test, ret, 0);
>> +	drm_gem_object_put(gobj1);
>> +	drm_gem_object_put(gobj2);
>> +	drm_exec_fini(&exec);
> It doesn't look like you actually check that "freeing objects from
> within drm_exec_fini() behaves as expected." What is the expectation
> here, and how is it checked?

Hm. Good question, I've been manually checking dmesg for lockdep splats. 
Is there a way to automate that?

/Thomas



> Maxime

Re: [Intel-xe] [PATCH 1/3] drm/kunit: Avoid a driver uaf

[Thomas Hellström]

Hi maxime,

On 9/5/23 14:06, Maxime Ripard wrote:
> On Tue, Sep 05, 2023 at 10:58:30AM +0200, Thomas Hellström wrote:
>> when using __drm_kunit_helper_alloc_drm_device() the driver may be
>> dereferenced by device-managed resources up until the device is
>> freed, which is typically later than the kunit-managed resource code
>> frees it.
> I'd like to have a bit more context on how a driver can end up in that
> situation?

I interpret the attached traces as follows.

INIT:

Code allocates a struct device as a kunit-managed resource.
Code allocates a drm driver as a kunit-managed resource.
Code allocates a drm device as a device-managed resource.

EXIT:

Kunit resource cleanup frees the drm driver
Kunit resource cleanup frees the struct device, which starts a 
device-managed resource cleanup
device-managed cleanup calls drm_dev_put()
drm_dev_put() dereferences the (now freed) drm driver -> Boom.

It should be sufficient to enable KASAN and run the drm_exec_test kunit 
test to trigger this.

Thanks,
Thomas


>
> Maxime

Re: [PATCH 3/3] drm/drm_exec: Work around a WW mutex lockdep oddity

[Christian König]

Am 05.09.23 um 10:58 schrieb Thomas Hellström:
> If *any* object of a certain WW mutex class is locked, lockdep will
> consider *all* mutexes of that class as locked. Also the lock allocation
> tracking code will apparently register only the address of the first
> mutex locked in a sequence.
> This has the odd consequence that if that first mutex is unlocked and
> its memory then freed, the lock alloc tracking code will assume that memory
> is freed with a held lock in there.
>
> For now, work around that for drm_exec by releasing the first grabbed
> object lock last.
>
> Related lock alloc tracking warning:
> [  322.660067] =========================
> [  322.660070] WARNING: held lock freed!
> [  322.660074] 6.5.0-rc7+ #155 Tainted: G     U           N
> [  322.660078] -------------------------
> [  322.660081] kunit_try_catch/4981 is freeing memory ffff888112adc000-ffff888112adc3ff, with a lock still held there!
> [  322.660089] ffff888112adc1a0 (reservation_ww_class_mutex){+.+.}-{3:3}, at: drm_exec_lock_obj+0x11a/0x600 [drm_exec]
> [  322.660104] 2 locks held by kunit_try_catch/4981:
> [  322.660108]  #0: ffffc9000343fe18 (reservation_ww_class_acquire){+.+.}-{0:0}, at: test_early_put+0x22f/0x490 [drm_exec_test]
> [  322.660123]  #1: ffff888112adc1a0 (reservation_ww_class_mutex){+.+.}-{3:3}, at: drm_exec_lock_obj+0x11a/0x600 [drm_exec]
> [  322.660135]
>                 stack backtrace:
> [  322.660139] CPU: 7 PID: 4981 Comm: kunit_try_catch Tainted: G     U           N 6.5.0-rc7+ #155
> [  322.660146] Hardware name: ASUS System Product Name/PRIME B560M-A AC, BIOS 0403 01/26/2021
> [  322.660152] Call Trace:
> [  322.660155]  <TASK>
> [  322.660158]  dump_stack_lvl+0x57/0x90
> [  322.660164]  debug_check_no_locks_freed+0x20b/0x2b0
> [  322.660172]  slab_free_freelist_hook+0xa1/0x160
> [  322.660179]  ? drm_exec_unlock_all+0x168/0x2a0 [drm_exec]
> [  322.660186]  __kmem_cache_free+0xb2/0x290
> [  322.660192]  drm_exec_unlock_all+0x168/0x2a0 [drm_exec]
> [  322.660200]  drm_exec_fini+0xf/0x1c0 [drm_exec]
> [  322.660206]  test_early_put+0x289/0x490 [drm_exec_test]
> [  322.660215]  ? __pfx_test_early_put+0x10/0x10 [drm_exec_test]
> [  322.660222]  ? __kasan_check_byte+0xf/0x40
> [  322.660227]  ? __ksize+0x63/0x140
> [  322.660233]  ? drmm_add_final_kfree+0x3e/0xa0 [drm]
> [  322.660289]  ? _raw_spin_unlock_irqrestore+0x30/0x60
> [  322.660294]  ? lockdep_hardirqs_on+0x7d/0x100
> [  322.660301]  ? __pfx_kunit_try_run_case+0x10/0x10 [kunit]
> [  322.660310]  ? __pfx_kunit_generic_run_threadfn_adapter+0x10/0x10 [kunit]
> [  322.660319]  kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit]
> [  322.660328]  kthread+0x2e7/0x3c0
> [  322.660334]  ? __pfx_kthread+0x10/0x10
> [  322.660339]  ret_from_fork+0x2d/0x70
> [  322.660345]  ? __pfx_kthread+0x10/0x10
> [  322.660349]  ret_from_fork_asm+0x1b/0x30
> [  322.660358]  </TASK>
> [  322.660818]     ok 8 test_early_put
>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> Cc: Danilo Krummrich <dakr@redhat.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>   drivers/gpu/drm/drm_exec.c |  2 +-
>   include/drm/drm_exec.h     | 35 +++++++++++++++++++++++++++++++----
>   2 files changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c
> index ff69cf0fb42a..5d2809de4517 100644
> --- a/drivers/gpu/drm/drm_exec.c
> +++ b/drivers/gpu/drm/drm_exec.c
> @@ -56,7 +56,7 @@ static void drm_exec_unlock_all(struct drm_exec *exec)
>   	struct drm_gem_object *obj;
>   	unsigned long index;
>   
> -	drm_exec_for_each_locked_object(exec, index, obj) {
> +	drm_exec_for_each_locked_object_reverse(exec, index, obj) {

Well that's a really good catch, just one more additional thought below.

>   		dma_resv_unlock(obj->resv);
>   		drm_gem_object_put(obj);
>   	}
> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
> index e0462361adf9..55764cf7c374 100644
> --- a/include/drm/drm_exec.h
> +++ b/include/drm/drm_exec.h
> @@ -51,6 +51,20 @@ struct drm_exec {
>   	struct drm_gem_object *prelocked;
>   };
>   
> +/**
> + * drm_exec_obj() - Return the object for a give drm_exec index
> + * @exec: Pointer to the drm_exec context
> + * @index: The index.
> + *
> + * Return: Pointer to the locked object corresponding to @index if
> + * index is within the number of locked objects. NULL otherwise.
> + */
> +static inline struct drm_gem_object *
> +drm_exec_obj(struct drm_exec *exec, unsigned long index)
> +{
> +	return index < exec->num_objects ? exec->objects[index] : NULL;
> +}
> +
>   /**
>    * drm_exec_for_each_locked_object - iterate over all the locked objects
>    * @exec: drm_exec object
> @@ -59,10 +73,23 @@ struct drm_exec {
>    *
>    * Iterate over all the locked GEM objects inside the drm_exec object.
>    */
> -#define drm_exec_for_each_locked_object(exec, index, obj)	\
> -	for (index = 0, obj = (exec)->objects[0];		\
> -	     index < (exec)->num_objects;			\
> -	     ++index, obj = (exec)->objects[index])
> +#define drm_exec_for_each_locked_object(exec, index, obj)		\
> +	for ((index) = 0; ((obj) = drm_exec_obj(exec, index)); ++(index))

Mhm, that makes it possible to modify the number of objects while inside 
the loop, doesn't it?

I'm not sure if that's a good idea or not.

Regards,
Christian.

> +
> +/**
> + * drm_exec_for_each_locked_object_reverse - iterate over all the locked
> + * objects in reverse locking order
> + * @exec: drm_exec object
> + * @index: unsigned long index for the iteration
> + * @obj: the current GEM object
> + *
> + * Iterate over all the locked GEM objects inside the drm_exec object in
> + * reverse locking order. Note that @index may go below zero and wrap,
> + * but that will be caught by drm_exec_object(), returning a NULL object.
> + */
> +#define drm_exec_for_each_locked_object_reverse(exec, index, obj)	\
> +	for ((index) = (exec)->num_objects - 1;				\
> +	     ((obj) = drm_exec_obj(exec, index)); --(index))
>   
>   /**
>    * drm_exec_until_all_locked - loop until all GEM objects are locked

Re: [Intel-xe] [PATCH 2/3] drm/tests/drm_exec: Add a test for object freeing within drm_exec_fini()

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4176 bytes --]

On Tue, Sep 05, 2023 at 02:32:38PM +0200, Thomas Hellström wrote:
> Hi,
> 
> On 9/5/23 14:05, Maxime Ripard wrote:
> > Hi,
> > 
> > On Tue, Sep 05, 2023 at 10:58:31AM +0200, Thomas Hellström wrote:
> > > Check that object freeing from within drm_exec_fini() works as expected
> > > and doesn't generate any warnings.
> > > 
> > > Cc: Christian König <christian.koenig@amd.com>
> > > Cc: dri-devel@lists.freedesktop.org
> > > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > ---
> > >   drivers/gpu/drm/tests/drm_exec_test.c | 47 +++++++++++++++++++++++++++
> > >   1 file changed, 47 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/tests/drm_exec_test.c b/drivers/gpu/drm/tests/drm_exec_test.c
> > > index 563949d777dd..294c25f49cc7 100644
> > > --- a/drivers/gpu/drm/tests/drm_exec_test.c
> > > +++ b/drivers/gpu/drm/tests/drm_exec_test.c
> > > @@ -170,6 +170,52 @@ static void test_prepare_array(struct kunit *test)
> > >   	drm_gem_private_object_fini(&gobj2);
> > >   }
> > > +static const struct drm_gem_object_funcs put_funcs = {
> > > +	.free = (void *)kfree,
> > > +};
> > > +
> > > +/*
> > > + * Check that freeing objects from within drm_exec_fini()
> > > + * behaves as expected.
> > > + */
> > > +static void test_early_put(struct kunit *test)
> > > +{
> > > +	struct drm_exec_priv *priv = test->priv;
> > > +	struct drm_gem_object *gobj1;
> > > +	struct drm_gem_object *gobj2;
> > > +	struct drm_gem_object *array[2];
> > > +	struct drm_exec exec;
> > > +	int ret;
> > > +
> > > +	gobj1 = kzalloc(sizeof(*gobj1), GFP_KERNEL);
> > > +	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj1);
> > > +	if (!gobj1)
> > > +		return;
> > > +
> > > +	gobj2 = kzalloc(sizeof(*gobj2), GFP_KERNEL);
> > > +	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj2);
> > > +	if (!gobj2) {
> > > +		kfree(gobj1);
> > > +		return;
> > > +	}
> > > +
> > > +	gobj1->funcs = &put_funcs;
> > > +	gobj2->funcs = &put_funcs;
> > > +	array[0] = gobj1;
> > > +	array[1] = gobj2;
> > > +	drm_gem_private_object_init(priv->drm, gobj1, PAGE_SIZE);
> > > +	drm_gem_private_object_init(priv->drm, gobj2, PAGE_SIZE);
> > > +
> > > +	drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT);
> > > +	drm_exec_until_all_locked(&exec)
> > > +		ret = drm_exec_prepare_array(&exec, array, ARRAY_SIZE(array),
> > > +					     1);
> > > +	KUNIT_EXPECT_EQ(test, ret, 0);
> > > +	drm_gem_object_put(gobj1);
> > > +	drm_gem_object_put(gobj2);
> > > +	drm_exec_fini(&exec);
> > It doesn't look like you actually check that "freeing objects from
> > within drm_exec_fini() behaves as expected." What is the expectation
> > here, and how is it checked?
> 
> Hm. Good question, I've been manually checking dmesg for lockdep splats. Is
> there a way to automate that?

I'm not familiar with the drm_exec API, but judging by the code I'd
assume you want to check that gobj1 and gobj2 are actually freed using
kfree?

If so, I've used tested for that by creating a waitqueue and completing
it from the free function. You won't be certain that you have gone
through kfree, but you'll know that drm_gem_object_funcs.free will have
been called which is what you actually care about I think?

So something along those lines would work I think:

struct test_gem_object {
	struct drm_gem_object base;
	wait_queue_head_t freed_wq;
	bool freed_done;
};

void free_test_gem_object(struct drm_gem_object *obj)
{
	struct test_gem_object *test_obj =
		container_of(obj, struct test_gem_object, base)

	test_obj->freed_done = true;
	wake_up(&test_obj->freed_wq);
};

static const struct drm_gem_object_funcs put_funcs = {
	.free = free_test_gem_object,
}

static void test_early_put(struct kunit *test)
{
	struct test_gem_object *gobj1;
	...
	gobj1 = kunit_kzalloc(test, sizeof(*gobj1), GFP_KERNEL);
	...
	gobj1->base.funcs = &put_funcs;
	...
	array[0] = &gobj1->base;

	drm_exec_fini(&exec);

	ret = wait_event_interruptible_timeout(gobj1->freed_wq, gobj1->freed_done,
                                               msecs_to_jiffies(TIMEOUT_MS));
	KUNIT_EXPECT_GT(test, ret, 0);
}

I guess?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [Intel-xe] [PATCH 2/3] drm/tests/drm_exec: Add a test for object freeing within drm_exec_fini()

[Thomas Hellström]

Hi, Maxime

On 9/5/23 15:16, Maxime Ripard wrote:
> On Tue, Sep 05, 2023 at 02:32:38PM +0200, Thomas Hellström wrote:
>> Hi,
>>
>> On 9/5/23 14:05, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Tue, Sep 05, 2023 at 10:58:31AM +0200, Thomas Hellström wrote:
>>>> Check that object freeing from within drm_exec_fini() works as expected
>>>> and doesn't generate any warnings.
>>>>
>>>> Cc: Christian König <christian.koenig@amd.com>
>>>> Cc: dri-devel@lists.freedesktop.org
>>>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>>> ---
>>>>    drivers/gpu/drm/tests/drm_exec_test.c | 47 +++++++++++++++++++++++++++
>>>>    1 file changed, 47 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/tests/drm_exec_test.c b/drivers/gpu/drm/tests/drm_exec_test.c
>>>> index 563949d777dd..294c25f49cc7 100644
>>>> --- a/drivers/gpu/drm/tests/drm_exec_test.c
>>>> +++ b/drivers/gpu/drm/tests/drm_exec_test.c
>>>> @@ -170,6 +170,52 @@ static void test_prepare_array(struct kunit *test)
>>>>    	drm_gem_private_object_fini(&gobj2);
>>>>    }
>>>> +static const struct drm_gem_object_funcs put_funcs = {
>>>> +	.free = (void *)kfree,
>>>> +};
>>>> +
>>>> +/*
>>>> + * Check that freeing objects from within drm_exec_fini()
>>>> + * behaves as expected.
>>>> + */
>>>> +static void test_early_put(struct kunit *test)
>>>> +{
>>>> +	struct drm_exec_priv *priv = test->priv;
>>>> +	struct drm_gem_object *gobj1;
>>>> +	struct drm_gem_object *gobj2;
>>>> +	struct drm_gem_object *array[2];
>>>> +	struct drm_exec exec;
>>>> +	int ret;
>>>> +
>>>> +	gobj1 = kzalloc(sizeof(*gobj1), GFP_KERNEL);
>>>> +	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj1);
>>>> +	if (!gobj1)
>>>> +		return;
>>>> +
>>>> +	gobj2 = kzalloc(sizeof(*gobj2), GFP_KERNEL);
>>>> +	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj2);
>>>> +	if (!gobj2) {
>>>> +		kfree(gobj1);
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	gobj1->funcs = &put_funcs;
>>>> +	gobj2->funcs = &put_funcs;
>>>> +	array[0] = gobj1;
>>>> +	array[1] = gobj2;
>>>> +	drm_gem_private_object_init(priv->drm, gobj1, PAGE_SIZE);
>>>> +	drm_gem_private_object_init(priv->drm, gobj2, PAGE_SIZE);
>>>> +
>>>> +	drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT);
>>>> +	drm_exec_until_all_locked(&exec)
>>>> +		ret = drm_exec_prepare_array(&exec, array, ARRAY_SIZE(array),
>>>> +					     1);
>>>> +	KUNIT_EXPECT_EQ(test, ret, 0);
>>>> +	drm_gem_object_put(gobj1);
>>>> +	drm_gem_object_put(gobj2);
>>>> +	drm_exec_fini(&exec);
>>> It doesn't look like you actually check that "freeing objects from
>>> within drm_exec_fini() behaves as expected." What is the expectation
>>> here, and how is it checked?
>> Hm. Good question, I've been manually checking dmesg for lockdep splats. Is
>> there a way to automate that?
> I'm not familiar with the drm_exec API, but judging by the code I'd
> assume you want to check that gobj1 and gobj2 are actually freed using
> kfree?

Actually not. What's important here is that the call to drm_exec_fini(), 
which puts the last references to gobj1 and gobj2 doesn't trigger any 
lockdep splats, like the one in the commit message of patch 3/3. So to 
make more sense, the test could perhaps be conditioned on 
CONFIG_DEBUG_LOCK_ALLOC. Still it would require manual checking of 
dmesg() after being run.

/Thomas

Re: [PATCH 3/3] drm/drm_exec: Work around a WW mutex lockdep oddity

[Thomas Hellström]

Hi, Christian

On 9/5/23 15:14, Christian König wrote:
> Am 05.09.23 um 10:58 schrieb Thomas Hellström:
>> If *any* object of a certain WW mutex class is locked, lockdep will
>> consider *all* mutexes of that class as locked. Also the lock allocation
>> tracking code will apparently register only the address of the first
>> mutex locked in a sequence.
>> This has the odd consequence that if that first mutex is unlocked and
>> its memory then freed, the lock alloc tracking code will assume that 
>> memory
>> is freed with a held lock in there.
>>
>> For now, work around that for drm_exec by releasing the first grabbed
>> object lock last.
>>
>> Related lock alloc tracking warning:
>> [  322.660067] =========================
>> [  322.660070] WARNING: held lock freed!
>> [  322.660074] 6.5.0-rc7+ #155 Tainted: G     U           N
>> [  322.660078] -------------------------
>> [  322.660081] kunit_try_catch/4981 is freeing memory 
>> ffff888112adc000-ffff888112adc3ff, with a lock still held there!
>> [  322.660089] ffff888112adc1a0 
>> (reservation_ww_class_mutex){+.+.}-{3:3}, at: 
>> drm_exec_lock_obj+0x11a/0x600 [drm_exec]
>> [  322.660104] 2 locks held by kunit_try_catch/4981:
>> [  322.660108]  #0: ffffc9000343fe18 
>> (reservation_ww_class_acquire){+.+.}-{0:0}, at: 
>> test_early_put+0x22f/0x490 [drm_exec_test]
>> [  322.660123]  #1: ffff888112adc1a0 
>> (reservation_ww_class_mutex){+.+.}-{3:3}, at: 
>> drm_exec_lock_obj+0x11a/0x600 [drm_exec]
>> [  322.660135]
>>                 stack backtrace:
>> [  322.660139] CPU: 7 PID: 4981 Comm: kunit_try_catch Tainted: G     
>> U           N 6.5.0-rc7+ #155
>> [  322.660146] Hardware name: ASUS System Product Name/PRIME B560M-A 
>> AC, BIOS 0403 01/26/2021
>> [  322.660152] Call Trace:
>> [  322.660155]  <TASK>
>> [  322.660158]  dump_stack_lvl+0x57/0x90
>> [  322.660164]  debug_check_no_locks_freed+0x20b/0x2b0
>> [  322.660172]  slab_free_freelist_hook+0xa1/0x160
>> [  322.660179]  ? drm_exec_unlock_all+0x168/0x2a0 [drm_exec]
>> [  322.660186]  __kmem_cache_free+0xb2/0x290
>> [  322.660192]  drm_exec_unlock_all+0x168/0x2a0 [drm_exec]
>> [  322.660200]  drm_exec_fini+0xf/0x1c0 [drm_exec]
>> [  322.660206]  test_early_put+0x289/0x490 [drm_exec_test]
>> [  322.660215]  ? __pfx_test_early_put+0x10/0x10 [drm_exec_test]
>> [  322.660222]  ? __kasan_check_byte+0xf/0x40
>> [  322.660227]  ? __ksize+0x63/0x140
>> [  322.660233]  ? drmm_add_final_kfree+0x3e/0xa0 [drm]
>> [  322.660289]  ? _raw_spin_unlock_irqrestore+0x30/0x60
>> [  322.660294]  ? lockdep_hardirqs_on+0x7d/0x100
>> [  322.660301]  ? __pfx_kunit_try_run_case+0x10/0x10 [kunit]
>> [  322.660310]  ? __pfx_kunit_generic_run_threadfn_adapter+0x10/0x10 
>> [kunit]
>> [  322.660319]  kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit]
>> [  322.660328]  kthread+0x2e7/0x3c0
>> [  322.660334]  ? __pfx_kthread+0x10/0x10
>> [  322.660339]  ret_from_fork+0x2d/0x70
>> [  322.660345]  ? __pfx_kthread+0x10/0x10
>> [  322.660349]  ret_from_fork_asm+0x1b/0x30
>> [  322.660358]  </TASK>
>> [  322.660818]     ok 8 test_early_put
>>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Boris Brezillon <boris.brezillon@collabora.com>
>> Cc: Danilo Krummrich <dakr@redhat.com>
>> Cc: dri-devel@lists.freedesktop.org
>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> ---
>>   drivers/gpu/drm/drm_exec.c |  2 +-
>>   include/drm/drm_exec.h     | 35 +++++++++++++++++++++++++++++++----
>>   2 files changed, 32 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c
>> index ff69cf0fb42a..5d2809de4517 100644
>> --- a/drivers/gpu/drm/drm_exec.c
>> +++ b/drivers/gpu/drm/drm_exec.c
>> @@ -56,7 +56,7 @@ static void drm_exec_unlock_all(struct drm_exec *exec)
>>       struct drm_gem_object *obj;
>>       unsigned long index;
>>   -    drm_exec_for_each_locked_object(exec, index, obj) {
>> +    drm_exec_for_each_locked_object_reverse(exec, index, obj) {
>
> Well that's a really good catch, just one more additional thought below.
>
>>           dma_resv_unlock(obj->resv);
>>           drm_gem_object_put(obj);
>>       }
>> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
>> index e0462361adf9..55764cf7c374 100644
>> --- a/include/drm/drm_exec.h
>> +++ b/include/drm/drm_exec.h
>> @@ -51,6 +51,20 @@ struct drm_exec {
>>       struct drm_gem_object *prelocked;
>>   };
>>   +/**
>> + * drm_exec_obj() - Return the object for a give drm_exec index
>> + * @exec: Pointer to the drm_exec context
>> + * @index: The index.
>> + *
>> + * Return: Pointer to the locked object corresponding to @index if
>> + * index is within the number of locked objects. NULL otherwise.
>> + */
>> +static inline struct drm_gem_object *
>> +drm_exec_obj(struct drm_exec *exec, unsigned long index)
>> +{
>> +    return index < exec->num_objects ? exec->objects[index] : NULL;
>> +}
>> +
>>   /**
>>    * drm_exec_for_each_locked_object - iterate over all the locked 
>> objects
>>    * @exec: drm_exec object
>> @@ -59,10 +73,23 @@ struct drm_exec {
>>    *
>>    * Iterate over all the locked GEM objects inside the drm_exec object.
>>    */
>> -#define drm_exec_for_each_locked_object(exec, index, obj)    \
>> -    for (index = 0, obj = (exec)->objects[0];        \
>> -         index < (exec)->num_objects;            \
>> -         ++index, obj = (exec)->objects[index])
>> +#define drm_exec_for_each_locked_object(exec, index, obj)        \
>> +    for ((index) = 0; ((obj) = drm_exec_obj(exec, index)); ++(index))
>
> Mhm, that makes it possible to modify the number of objects while 
> inside the loop, doesn't it?

Sorry, you lost me a bit there. Isn't that possible with the previous 
code as well?

/Thanks,

Thomas



>
> I'm not sure if that's a good idea or not.
>
> Regards,
> Christian.
>
>> +
>> +/**
>> + * drm_exec_for_each_locked_object_reverse - iterate over all the 
>> locked
>> + * objects in reverse locking order
>> + * @exec: drm_exec object
>> + * @index: unsigned long index for the iteration
>> + * @obj: the current GEM object
>> + *
>> + * Iterate over all the locked GEM objects inside the drm_exec 
>> object in
>> + * reverse locking order. Note that @index may go below zero and wrap,
>> + * but that will be caught by drm_exec_object(), returning a NULL 
>> object.
>> + */
>> +#define drm_exec_for_each_locked_object_reverse(exec, index, obj)    \
>> +    for ((index) = (exec)->num_objects - 1;                \
>> +         ((obj) = drm_exec_obj(exec, index)); --(index))
>>     /**
>>    * drm_exec_until_all_locked - loop until all GEM objects are locked
>

Re: [PATCH 3/3] drm/drm_exec: Work around a WW mutex lockdep oddity

[Christian König]

Am 05.09.23 um 16:29 schrieb Thomas Hellström:
> Hi, Christian
>
> On 9/5/23 15:14, Christian König wrote:
>> Am 05.09.23 um 10:58 schrieb Thomas Hellström:
>>> If *any* object of a certain WW mutex class is locked, lockdep will
>>> consider *all* mutexes of that class as locked. Also the lock 
>>> allocation
>>> tracking code will apparently register only the address of the first
>>> mutex locked in a sequence.
>>> This has the odd consequence that if that first mutex is unlocked and
>>> its memory then freed, the lock alloc tracking code will assume that 
>>> memory
>>> is freed with a held lock in there.
>>>
>>> For now, work around that for drm_exec by releasing the first grabbed
>>> object lock last.
>>>
>>> Related lock alloc tracking warning:
>>> [  322.660067] =========================
>>> [  322.660070] WARNING: held lock freed!
>>> [  322.660074] 6.5.0-rc7+ #155 Tainted: G     U           N
>>> [  322.660078] -------------------------
>>> [  322.660081] kunit_try_catch/4981 is freeing memory 
>>> ffff888112adc000-ffff888112adc3ff, with a lock still held there!
>>> [  322.660089] ffff888112adc1a0 
>>> (reservation_ww_class_mutex){+.+.}-{3:3}, at: 
>>> drm_exec_lock_obj+0x11a/0x600 [drm_exec]
>>> [  322.660104] 2 locks held by kunit_try_catch/4981:
>>> [  322.660108]  #0: ffffc9000343fe18 
>>> (reservation_ww_class_acquire){+.+.}-{0:0}, at: 
>>> test_early_put+0x22f/0x490 [drm_exec_test]
>>> [  322.660123]  #1: ffff888112adc1a0 
>>> (reservation_ww_class_mutex){+.+.}-{3:3}, at: 
>>> drm_exec_lock_obj+0x11a/0x600 [drm_exec]
>>> [  322.660135]
>>>                 stack backtrace:
>>> [  322.660139] CPU: 7 PID: 4981 Comm: kunit_try_catch Tainted: G     
>>> U           N 6.5.0-rc7+ #155
>>> [  322.660146] Hardware name: ASUS System Product Name/PRIME B560M-A 
>>> AC, BIOS 0403 01/26/2021
>>> [  322.660152] Call Trace:
>>> [  322.660155]  <TASK>
>>> [  322.660158]  dump_stack_lvl+0x57/0x90
>>> [  322.660164]  debug_check_no_locks_freed+0x20b/0x2b0
>>> [  322.660172]  slab_free_freelist_hook+0xa1/0x160
>>> [  322.660179]  ? drm_exec_unlock_all+0x168/0x2a0 [drm_exec]
>>> [  322.660186]  __kmem_cache_free+0xb2/0x290
>>> [  322.660192]  drm_exec_unlock_all+0x168/0x2a0 [drm_exec]
>>> [  322.660200]  drm_exec_fini+0xf/0x1c0 [drm_exec]
>>> [  322.660206]  test_early_put+0x289/0x490 [drm_exec_test]
>>> [  322.660215]  ? __pfx_test_early_put+0x10/0x10 [drm_exec_test]
>>> [  322.660222]  ? __kasan_check_byte+0xf/0x40
>>> [  322.660227]  ? __ksize+0x63/0x140
>>> [  322.660233]  ? drmm_add_final_kfree+0x3e/0xa0 [drm]
>>> [  322.660289]  ? _raw_spin_unlock_irqrestore+0x30/0x60
>>> [  322.660294]  ? lockdep_hardirqs_on+0x7d/0x100
>>> [  322.660301]  ? __pfx_kunit_try_run_case+0x10/0x10 [kunit]
>>> [  322.660310]  ? __pfx_kunit_generic_run_threadfn_adapter+0x10/0x10 
>>> [kunit]
>>> [  322.660319]  kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit]
>>> [  322.660328]  kthread+0x2e7/0x3c0
>>> [  322.660334]  ? __pfx_kthread+0x10/0x10
>>> [  322.660339]  ret_from_fork+0x2d/0x70
>>> [  322.660345]  ? __pfx_kthread+0x10/0x10
>>> [  322.660349]  ret_from_fork_asm+0x1b/0x30
>>> [  322.660358]  </TASK>
>>> [  322.660818]     ok 8 test_early_put
>>>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Cc: Boris Brezillon <boris.brezillon@collabora.com>
>>> Cc: Danilo Krummrich <dakr@redhat.com>
>>> Cc: dri-devel@lists.freedesktop.org
>>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>> ---
>>>   drivers/gpu/drm/drm_exec.c |  2 +-
>>>   include/drm/drm_exec.h     | 35 +++++++++++++++++++++++++++++++----
>>>   2 files changed, 32 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c
>>> index ff69cf0fb42a..5d2809de4517 100644
>>> --- a/drivers/gpu/drm/drm_exec.c
>>> +++ b/drivers/gpu/drm/drm_exec.c
>>> @@ -56,7 +56,7 @@ static void drm_exec_unlock_all(struct drm_exec 
>>> *exec)
>>>       struct drm_gem_object *obj;
>>>       unsigned long index;
>>>   -    drm_exec_for_each_locked_object(exec, index, obj) {
>>> +    drm_exec_for_each_locked_object_reverse(exec, index, obj) {
>>
>> Well that's a really good catch, just one more additional thought below.
>>
>>>           dma_resv_unlock(obj->resv);
>>>           drm_gem_object_put(obj);
>>>       }
>>> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
>>> index e0462361adf9..55764cf7c374 100644
>>> --- a/include/drm/drm_exec.h
>>> +++ b/include/drm/drm_exec.h
>>> @@ -51,6 +51,20 @@ struct drm_exec {
>>>       struct drm_gem_object *prelocked;
>>>   };
>>>   +/**
>>> + * drm_exec_obj() - Return the object for a give drm_exec index
>>> + * @exec: Pointer to the drm_exec context
>>> + * @index: The index.
>>> + *
>>> + * Return: Pointer to the locked object corresponding to @index if
>>> + * index is within the number of locked objects. NULL otherwise.
>>> + */
>>> +static inline struct drm_gem_object *
>>> +drm_exec_obj(struct drm_exec *exec, unsigned long index)
>>> +{
>>> +    return index < exec->num_objects ? exec->objects[index] : NULL;
>>> +}
>>> +
>>>   /**
>>>    * drm_exec_for_each_locked_object - iterate over all the locked 
>>> objects
>>>    * @exec: drm_exec object
>>> @@ -59,10 +73,23 @@ struct drm_exec {
>>>    *
>>>    * Iterate over all the locked GEM objects inside the drm_exec 
>>> object.
>>>    */
>>> -#define drm_exec_for_each_locked_object(exec, index, obj) \
>>> -    for (index = 0, obj = (exec)->objects[0];        \
>>> -         index < (exec)->num_objects;            \
>>> -         ++index, obj = (exec)->objects[index])
>>> +#define drm_exec_for_each_locked_object(exec, index, obj)        \
>>> +    for ((index) = 0; ((obj) = drm_exec_obj(exec, index)); ++(index))
>>
>> Mhm, that makes it possible to modify the number of objects while 
>> inside the loop, doesn't it?
>
> Sorry, you lost me a bit there. Isn't that possible with the previous 
> code as well?

Yeah, indeed. Reviewed-by: Christian König <christian.koenig@amd.com>

Regards,
Christian.

>
> /Thanks,
>
> Thomas
>
>
>
>>
>> I'm not sure if that's a good idea or not.
>>
>> Regards,
>> Christian.
>>
>>> +
>>> +/**
>>> + * drm_exec_for_each_locked_object_reverse - iterate over all the 
>>> locked
>>> + * objects in reverse locking order
>>> + * @exec: drm_exec object
>>> + * @index: unsigned long index for the iteration
>>> + * @obj: the current GEM object
>>> + *
>>> + * Iterate over all the locked GEM objects inside the drm_exec 
>>> object in
>>> + * reverse locking order. Note that @index may go below zero and wrap,
>>> + * but that will be caught by drm_exec_object(), returning a NULL 
>>> object.
>>> + */
>>> +#define drm_exec_for_each_locked_object_reverse(exec, index, obj)    \
>>> +    for ((index) = (exec)->num_objects - 1; \
>>> +         ((obj) = drm_exec_obj(exec, index)); --(index))
>>>     /**
>>>    * drm_exec_until_all_locked - loop until all GEM objects are locked
>>

Re: [Intel-xe] [PATCH 2/3] drm/tests/drm_exec: Add a test for object freeing within drm_exec_fini()

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4601 bytes --]

On Tue, Sep 05, 2023 at 03:42:58PM +0200, Thomas Hellström wrote:
> Hi, Maxime
> 
> On 9/5/23 15:16, Maxime Ripard wrote:
> > On Tue, Sep 05, 2023 at 02:32:38PM +0200, Thomas Hellström wrote:
> > > Hi,
> > > 
> > > On 9/5/23 14:05, Maxime Ripard wrote:
> > > > Hi,
> > > > 
> > > > On Tue, Sep 05, 2023 at 10:58:31AM +0200, Thomas Hellström wrote:
> > > > > Check that object freeing from within drm_exec_fini() works as expected
> > > > > and doesn't generate any warnings.
> > > > > 
> > > > > Cc: Christian König <christian.koenig@amd.com>
> > > > > Cc: dri-devel@lists.freedesktop.org
> > > > > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > > > ---
> > > > >    drivers/gpu/drm/tests/drm_exec_test.c | 47 +++++++++++++++++++++++++++
> > > > >    1 file changed, 47 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/tests/drm_exec_test.c b/drivers/gpu/drm/tests/drm_exec_test.c
> > > > > index 563949d777dd..294c25f49cc7 100644
> > > > > --- a/drivers/gpu/drm/tests/drm_exec_test.c
> > > > > +++ b/drivers/gpu/drm/tests/drm_exec_test.c
> > > > > @@ -170,6 +170,52 @@ static void test_prepare_array(struct kunit *test)
> > > > >    	drm_gem_private_object_fini(&gobj2);
> > > > >    }
> > > > > +static const struct drm_gem_object_funcs put_funcs = {
> > > > > +	.free = (void *)kfree,
> > > > > +};
> > > > > +
> > > > > +/*
> > > > > + * Check that freeing objects from within drm_exec_fini()
> > > > > + * behaves as expected.
> > > > > + */
> > > > > +static void test_early_put(struct kunit *test)
> > > > > +{
> > > > > +	struct drm_exec_priv *priv = test->priv;
> > > > > +	struct drm_gem_object *gobj1;
> > > > > +	struct drm_gem_object *gobj2;
> > > > > +	struct drm_gem_object *array[2];
> > > > > +	struct drm_exec exec;
> > > > > +	int ret;
> > > > > +
> > > > > +	gobj1 = kzalloc(sizeof(*gobj1), GFP_KERNEL);
> > > > > +	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj1);
> > > > > +	if (!gobj1)
> > > > > +		return;
> > > > > +
> > > > > +	gobj2 = kzalloc(sizeof(*gobj2), GFP_KERNEL);
> > > > > +	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj2);
> > > > > +	if (!gobj2) {
> > > > > +		kfree(gobj1);
> > > > > +		return;
> > > > > +	}
> > > > > +
> > > > > +	gobj1->funcs = &put_funcs;
> > > > > +	gobj2->funcs = &put_funcs;
> > > > > +	array[0] = gobj1;
> > > > > +	array[1] = gobj2;
> > > > > +	drm_gem_private_object_init(priv->drm, gobj1, PAGE_SIZE);
> > > > > +	drm_gem_private_object_init(priv->drm, gobj2, PAGE_SIZE);
> > > > > +
> > > > > +	drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT);
> > > > > +	drm_exec_until_all_locked(&exec)
> > > > > +		ret = drm_exec_prepare_array(&exec, array, ARRAY_SIZE(array),
> > > > > +					     1);
> > > > > +	KUNIT_EXPECT_EQ(test, ret, 0);
> > > > > +	drm_gem_object_put(gobj1);
> > > > > +	drm_gem_object_put(gobj2);
> > > > > +	drm_exec_fini(&exec);
> > > > It doesn't look like you actually check that "freeing objects from
> > > > within drm_exec_fini() behaves as expected." What is the expectation
> > > > here, and how is it checked?
> > > Hm. Good question, I've been manually checking dmesg for lockdep splats. Is
> > > there a way to automate that?
> > I'm not familiar with the drm_exec API, but judging by the code I'd
> > assume you want to check that gobj1 and gobj2 are actually freed using
> > kfree?
> 
> Actually not. What's important here is that the call to drm_exec_fini(),
> which puts the last references to gobj1 and gobj2 doesn't trigger any
> lockdep splats, like the one in the commit message of patch 3/3. So to make
> more sense, the test could perhaps be conditioned on
> CONFIG_DEBUG_LOCK_ALLOC. Still it would require manual checking of dmesg()
> after being run.

I'm not aware of something to check on lockdep's status when running a
kunit test, but I'm not sure anyone is expected to look at the dmesg
trace when running kunit to find out whether the test succeeded or not.

It looks like there was an attempt at some point to fail the test if
there was a lockdep error:
https://lore.kernel.org/all/20200814205527.1833459-1-urielguajardojr@gmail.com/

It doesn't look like it's been merged though. David, Brendan, do you
know why it wasn't merged or if there is a good option for us there?

At the very least, I think a comment after the call to drm_exec_fini to
make it clear that the error would be in the kernel logs, and a better
one on the test definition to explicitly say what you want to make sure
of, and how one can check it's been done would be great.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [Intel-xe] [PATCH 1/3] drm/kunit: Avoid a driver uaf

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1226 bytes --]

On Tue, Sep 05, 2023 at 02:43:00PM +0200, Thomas Hellström wrote:
> Hi maxime,
> 
> On 9/5/23 14:06, Maxime Ripard wrote:
> > On Tue, Sep 05, 2023 at 10:58:30AM +0200, Thomas Hellström wrote:
> > > when using __drm_kunit_helper_alloc_drm_device() the driver may be
> > > dereferenced by device-managed resources up until the device is
> > > freed, which is typically later than the kunit-managed resource code
> > > frees it.
> > I'd like to have a bit more context on how a driver can end up in that
> > situation?
> 
> I interpret the attached traces as follows.
> 
> INIT:
> 
> Code allocates a struct device as a kunit-managed resource.
> Code allocates a drm driver as a kunit-managed resource.
> Code allocates a drm device as a device-managed resource.
> 
> EXIT:
> 
> Kunit resource cleanup frees the drm driver
> Kunit resource cleanup frees the struct device, which starts a
> device-managed resource cleanup
> device-managed cleanup calls drm_dev_put()
> drm_dev_put() dereferences the (now freed) drm driver -> Boom.
> 
> It should be sufficient to enable KASAN and run the drm_exec_test kunit test
> to trigger this.

Ack. Can you put this into your commit log?

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/3] drm/drm_exec: Work around a WW mutex lockdep oddity

[Thomas Hellström]

Hi,

On 9/6/23 10:34, Christian König wrote:
> Am 05.09.23 um 16:29 schrieb Thomas Hellström:
>> Hi, Christian
>>
>> On 9/5/23 15:14, Christian König wrote:
>>> Am 05.09.23 um 10:58 schrieb Thomas Hellström:
>>>> If *any* object of a certain WW mutex class is locked, lockdep will
>>>> consider *all* mutexes of that class as locked. Also the lock 
>>>> allocation
>>>> tracking code will apparently register only the address of the first
>>>> mutex locked in a sequence.
>>>> This has the odd consequence that if that first mutex is unlocked and
>>>> its memory then freed, the lock alloc tracking code will assume 
>>>> that memory
>>>> is freed with a held lock in there.
>>>>
>>>> For now, work around that for drm_exec by releasing the first grabbed
>>>> object lock last.
>>>>
>>>> Related lock alloc tracking warning:
>>>> [  322.660067] =========================
>>>> [  322.660070] WARNING: held lock freed!
>>>> [  322.660074] 6.5.0-rc7+ #155 Tainted: G     U           N
>>>> [  322.660078] -------------------------
>>>> [  322.660081] kunit_try_catch/4981 is freeing memory 
>>>> ffff888112adc000-ffff888112adc3ff, with a lock still held there!
>>>> [  322.660089] ffff888112adc1a0 
>>>> (reservation_ww_class_mutex){+.+.}-{3:3}, at: 
>>>> drm_exec_lock_obj+0x11a/0x600 [drm_exec]
>>>> [  322.660104] 2 locks held by kunit_try_catch/4981:
>>>> [  322.660108]  #0: ffffc9000343fe18 
>>>> (reservation_ww_class_acquire){+.+.}-{0:0}, at: 
>>>> test_early_put+0x22f/0x490 [drm_exec_test]
>>>> [  322.660123]  #1: ffff888112adc1a0 
>>>> (reservation_ww_class_mutex){+.+.}-{3:3}, at: 
>>>> drm_exec_lock_obj+0x11a/0x600 [drm_exec]
>>>> [  322.660135]
>>>>                 stack backtrace:
>>>> [  322.660139] CPU: 7 PID: 4981 Comm: kunit_try_catch Tainted: 
>>>> G     U           N 6.5.0-rc7+ #155
>>>> [  322.660146] Hardware name: ASUS System Product Name/PRIME 
>>>> B560M-A AC, BIOS 0403 01/26/2021
>>>> [  322.660152] Call Trace:
>>>> [  322.660155]  <TASK>
>>>> [  322.660158]  dump_stack_lvl+0x57/0x90
>>>> [  322.660164]  debug_check_no_locks_freed+0x20b/0x2b0
>>>> [  322.660172]  slab_free_freelist_hook+0xa1/0x160
>>>> [  322.660179]  ? drm_exec_unlock_all+0x168/0x2a0 [drm_exec]
>>>> [  322.660186]  __kmem_cache_free+0xb2/0x290
>>>> [  322.660192]  drm_exec_unlock_all+0x168/0x2a0 [drm_exec]
>>>> [  322.660200]  drm_exec_fini+0xf/0x1c0 [drm_exec]
>>>> [  322.660206]  test_early_put+0x289/0x490 [drm_exec_test]
>>>> [  322.660215]  ? __pfx_test_early_put+0x10/0x10 [drm_exec_test]
>>>> [  322.660222]  ? __kasan_check_byte+0xf/0x40
>>>> [  322.660227]  ? __ksize+0x63/0x140
>>>> [  322.660233]  ? drmm_add_final_kfree+0x3e/0xa0 [drm]
>>>> [  322.660289]  ? _raw_spin_unlock_irqrestore+0x30/0x60
>>>> [  322.660294]  ? lockdep_hardirqs_on+0x7d/0x100
>>>> [  322.660301]  ? __pfx_kunit_try_run_case+0x10/0x10 [kunit]
>>>> [  322.660310]  ? 
>>>> __pfx_kunit_generic_run_threadfn_adapter+0x10/0x10 [kunit]
>>>> [  322.660319]  kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit]
>>>> [  322.660328]  kthread+0x2e7/0x3c0
>>>> [  322.660334]  ? __pfx_kthread+0x10/0x10
>>>> [  322.660339]  ret_from_fork+0x2d/0x70
>>>> [  322.660345]  ? __pfx_kthread+0x10/0x10
>>>> [  322.660349]  ret_from_fork_asm+0x1b/0x30
>>>> [  322.660358]  </TASK>
>>>> [  322.660818]     ok 8 test_early_put
>>>>
>>>> Cc: Christian König <christian.koenig@amd.com>
>>>> Cc: Boris Brezillon <boris.brezillon@collabora.com>
>>>> Cc: Danilo Krummrich <dakr@redhat.com>
>>>> Cc: dri-devel@lists.freedesktop.org
>>>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>>> ---
>>>>   drivers/gpu/drm/drm_exec.c |  2 +-
>>>>   include/drm/drm_exec.h     | 35 +++++++++++++++++++++++++++++++----
>>>>   2 files changed, 32 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c
>>>> index ff69cf0fb42a..5d2809de4517 100644
>>>> --- a/drivers/gpu/drm/drm_exec.c
>>>> +++ b/drivers/gpu/drm/drm_exec.c
>>>> @@ -56,7 +56,7 @@ static void drm_exec_unlock_all(struct drm_exec 
>>>> *exec)
>>>>       struct drm_gem_object *obj;
>>>>       unsigned long index;
>>>>   -    drm_exec_for_each_locked_object(exec, index, obj) {
>>>> +    drm_exec_for_each_locked_object_reverse(exec, index, obj) {
>>>
>>> Well that's a really good catch, just one more additional thought 
>>> below.
>>>
>>>> dma_resv_unlock(obj->resv);
>>>>           drm_gem_object_put(obj);
>>>>       }
>>>> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
>>>> index e0462361adf9..55764cf7c374 100644
>>>> --- a/include/drm/drm_exec.h
>>>> +++ b/include/drm/drm_exec.h
>>>> @@ -51,6 +51,20 @@ struct drm_exec {
>>>>       struct drm_gem_object *prelocked;
>>>>   };
>>>>   +/**
>>>> + * drm_exec_obj() - Return the object for a give drm_exec index
>>>> + * @exec: Pointer to the drm_exec context
>>>> + * @index: The index.
>>>> + *
>>>> + * Return: Pointer to the locked object corresponding to @index if
>>>> + * index is within the number of locked objects. NULL otherwise.
>>>> + */
>>>> +static inline struct drm_gem_object *
>>>> +drm_exec_obj(struct drm_exec *exec, unsigned long index)
>>>> +{
>>>> +    return index < exec->num_objects ? exec->objects[index] : NULL;
>>>> +}
>>>> +
>>>>   /**
>>>>    * drm_exec_for_each_locked_object - iterate over all the locked 
>>>> objects
>>>>    * @exec: drm_exec object
>>>> @@ -59,10 +73,23 @@ struct drm_exec {
>>>>    *
>>>>    * Iterate over all the locked GEM objects inside the drm_exec 
>>>> object.
>>>>    */
>>>> -#define drm_exec_for_each_locked_object(exec, index, obj) \
>>>> -    for (index = 0, obj = (exec)->objects[0];        \
>>>> -         index < (exec)->num_objects;            \
>>>> -         ++index, obj = (exec)->objects[index])
>>>> +#define drm_exec_for_each_locked_object(exec, index, obj)        \
>>>> +    for ((index) = 0; ((obj) = drm_exec_obj(exec, index)); ++(index))
>>>
>>> Mhm, that makes it possible to modify the number of objects while 
>>> inside the loop, doesn't it?
>>
>> Sorry, you lost me a bit there. Isn't that possible with the previous 
>> code as well?
>
> Yeah, indeed. Reviewed-by: Christian König <christian.koenig@amd.com>
>
> Regards,
> Christian.

Thanks Boris, Danilo and Christian for review. I pushed this one to 
drm-misc-next-fixes.

/Thomas


>
>>
>> /Thanks,
>>
>> Thomas
>>
>>
>>
>>>
>>> I'm not sure if that's a good idea or not.
>>>
>>> Regards,
>>> Christian.
>>>
>>>> +
>>>> +/**
>>>> + * drm_exec_for_each_locked_object_reverse - iterate over all the 
>>>> locked
>>>> + * objects in reverse locking order
>>>> + * @exec: drm_exec object
>>>> + * @index: unsigned long index for the iteration
>>>> + * @obj: the current GEM object
>>>> + *
>>>> + * Iterate over all the locked GEM objects inside the drm_exec 
>>>> object in
>>>> + * reverse locking order. Note that @index may go below zero and 
>>>> wrap,
>>>> + * but that will be caught by drm_exec_object(), returning a NULL 
>>>> object.
>>>> + */
>>>> +#define drm_exec_for_each_locked_object_reverse(exec, index, 
>>>> obj)    \
>>>> +    for ((index) = (exec)->num_objects - 1; \
>>>> +         ((obj) = drm_exec_obj(exec, index)); --(index))
>>>>     /**
>>>>    * drm_exec_until_all_locked - loop until all GEM objects are locked
>>>
>

Re: [Intel-xe] [PATCH 1/3] drm/kunit: Avoid a driver uaf

[Thomas Hellström]

Hi, Maxime,

On 9/6/23 12:08, Maxime Ripard wrote:
> On Tue, Sep 05, 2023 at 02:43:00PM +0200, Thomas Hellström wrote:
>> Hi maxime,
>>
>> On 9/5/23 14:06, Maxime Ripard wrote:
>>> On Tue, Sep 05, 2023 at 10:58:30AM +0200, Thomas Hellström wrote:
>>>> when using __drm_kunit_helper_alloc_drm_device() the driver may be
>>>> dereferenced by device-managed resources up until the device is
>>>> freed, which is typically later than the kunit-managed resource code
>>>> frees it.
>>> I'd like to have a bit more context on how a driver can end up in that
>>> situation?
>> I interpret the attached traces as follows.
>>
>> INIT:
>>
>> Code allocates a struct device as a kunit-managed resource.
>> Code allocates a drm driver as a kunit-managed resource.
>> Code allocates a drm device as a device-managed resource.
>>
>> EXIT:
>>
>> Kunit resource cleanup frees the drm driver
>> Kunit resource cleanup frees the struct device, which starts a
>> device-managed resource cleanup
>> device-managed cleanup calls drm_dev_put()
>> drm_dev_put() dereferences the (now freed) drm driver -> Boom.
>>
>> It should be sufficient to enable KASAN and run the drm_exec_test kunit test
>> to trigger this.
> Ack. Can you put this into your commit log?
>
> Thanks!
> Maxime

Thanks for reviewing. I'll update this and the other patch with your 
comments.

Thanks,

Thomas