Re: [PATCH v2 3/3] drm/panfrost: show device-wide list of DRM GEM objects over DebugFS

[Steven Price <steven.price@arm.com>]

On 15/05/2025 19:04, Daniel Stone wrote:
> Hi Steven,
> 
> On Thu, 8 May 2025 at 11:42, Steven Price <steven.price@arm.com> wrote:
>> I'm also seeing a splat when running this, see below. I haven't got my
>> head around how this is happening, but I see it when glmark quits at the
>> end of the test.
>>
>> [  399.505066] Unable to handle kernel NULL pointer dereference at virtual address 00000004 when write
>> [...]
>> [  399.882216] Call trace:
>> [  399.882222]  panfrost_gem_free_object [panfrost] from drm_gem_handle_delete+0x84/0xb0
>> [  399.893813]  drm_gem_handle_delete from drm_ioctl+0x2b8/0x4f4
>> [  399.900237]  drm_ioctl from sys_ioctl+0x428/0xe30
>> [  399.905496]  sys_ioctl from ret_fast_syscall+0x0/0x1c
> 
> Soooo. Let's assume it has to actually occur in
> panfrost_gem_debugfs_bo_rm(), since that's all that's changed here.
> 
> I don't think pfdev can be NULL here, because we've already
> dereferenced ptdev and written to a structure member earlier in
> panfrost_gem_free_object(). I don't think it can be the debugfs mutex,
> because a) that's initialised with the device, and b) wouldn't be
> offset 0x4.
> 
> I'm looking then at list_del_init(&bo->debugfs.node), which would
> effectively execute bo->debugfs.node->next->prev =
> bo->debugfs.node->prev. So if bo->debugfs.node->next was NULL, that
> would explain a write to 0x4 on 32-bit systems.

So I finally got some time to do some debugging on this. And you are
absolutely correct on where the fault is triggered.

The cause of it is that panfrost_gem_debugfs_bo_add() is called from
panfrost_gem_create(), but that isn't the only place that Panfrost GEM
objects are created - it turns out panfrost_perfcnt_enable_locked() also
calls drm_gem_shmem_create(). And in that case the list next/prev
pointers are left set to NULL, causing things to blow up when the GEM
object is freed.

The below patch gets things working, or alternatively just init the list
in panfrost_gem_create_object() if we don't want to include the perfcnt
buffer in the list.

Steve

---8<--
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c
b/drivers/gpu/drm/panfrost/panfrost_gem.c
index fe2cdbe8baf0..51da13cd81f0 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -297,13 +297,14 @@ struct drm_gem_object
*panfrost_gem_create_object(struct drm_device *dev, size_t
        obj->base.map_wc = !pfdev->coherent;
        mutex_init(&obj->label.lock);

+       panfrost_gem_debugfs_bo_add(pfdev, obj);
+
        return &obj->base.base;
 }

 struct panfrost_gem_object *
 panfrost_gem_create(struct drm_device *dev, size_t size, u32 flags)
 {
-       struct panfrost_device *pfdev = dev->dev_private;
        struct drm_gem_shmem_object *shmem;
        struct panfrost_gem_object *bo;

@@ -319,8 +320,6 @@ panfrost_gem_create(struct drm_device *dev, size_t
size, u32 flags)
        bo->noexec = !!(flags & PANFROST_BO_NOEXEC);
        bo->is_heap = !!(flags & PANFROST_BO_HEAP);

-       panfrost_gem_debugfs_bo_add(pfdev, bo);
-
        return bo;
 }