linux-kernel - Re: [PATCH v4 4/4] drm/panthor: show device-wide list of DRM GEM objects over DebugFS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250408154755.0d45b54b@collabora.com>
Date: Tue, 8 Apr 2025 15:47:55 +0200
From: Boris Brezillon <boris.brezillon@...labora.com>
To: Adrián Larumbe <adrian.larumbe@...labora.com>
Cc: Steven Price <steven.price@....com>, Liviu Dudau <liviu.dudau@....com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard
 <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, David Airlie
 <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Sumit Semwal
 <sumit.semwal@...aro.org>, Christian König
 <christian.koenig@....com>, kernel@...labora.com,
 dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
 linux-media@...r.kernel.org, linaro-mm-sig@...ts.linaro.org
Subject: Re: [PATCH v4 4/4] drm/panthor: show device-wide list of DRM GEM
 objects over DebugFS

On Tue, 8 Apr 2025 14:38:44 +0100
Adrián Larumbe <adrian.larumbe@...labora.com> wrote:

> > > diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
> > > index 44d027e6d664..2fc87be9b700 100644
> > > --- a/drivers/gpu/drm/panthor/panthor_gem.c
> > > +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> > > @@ -2,6 +2,7 @@
> > >  /* Copyright 2019 Linaro, Ltd, Rob Herring <robh@...nel.org> */
> > >  /* Copyright 2023 Collabora ltd. */
> > >
> > > +#include <linux/cleanup.h>
> > >  #include <linux/dma-buf.h>
> > >  #include <linux/dma-mapping.h>
> > >  #include <linux/err.h>
> > > @@ -10,14 +11,65 @@
> > >  #include <drm/panthor_drm.h>
> > >
> > >  #include "panthor_device.h"
> > > +#include "panthor_fw.h"
> > >  #include "panthor_gem.h"
> > >  #include "panthor_mmu.h"
> > >
> > > +#ifdef CONFIG_DEBUG_FS
> > > +static void panthor_gem_debugfs_bo_init(struct panthor_gem_object *bo, u32 type_mask)
> > > +{
> > > +	INIT_LIST_HEAD(&bo->debugfs.node);  
> >
> > This should be called when the GEM object is created, otherwise the
> > list_empty() test done in panthor_gem_debugfs_bo_rm() will only work if
> > panthor_gem_debugfs_bo_add() is called, and depending on when this
> > happens, or whether it happens at all, the error path will do a NULL
> > deref.  
> 
> I'll be moving panthor_gem_debugfs_bo_add() back into panthor_gem_create_object() and
> inline panthor_gem_debugfs_bo_init() into it.

You mean moving the panthor_gem_debugfs_bo_add() call to
panthor_gem_create_object(), not inlining its content, right?

> > > +	} else {
> > > +		bo->debugfs.creator.tgid = 0;
> > > +		snprintf(bo->debugfs.creator.process_name,
> > > +			 sizeof(bo->debugfs.creator.process_name),
> > > +			 "kernel");
> > > +	}
> > > +
> > > +	bo->debugfs.bo_mask = type_mask;  
> >
> > Why not do that directly in panthor_gem_debugfs_bo_add()? The only bits
> > that might be useful to do early is the INIT_LIST_HEAD(), and I think
> > it can be inlined in panthor_gem_create_object().  
> 
> I'll be doing in this in the next revision, but because I've no access to the BO
> type mask from inside Panthor's drm_driver::gem_create_object() binding, then
> I'll have to assign the mask right after the object has been created.
> 
> I think this means there might be a short window after the object's been added to
> the DebugFS GEMs list in which it could be shown with the kernel mask field still
> set to 0, but I guess that's not too important either.

I think it's okay, as long as you don't crash when printing partially
initialized objects. Another solution would be to have a flag encoding
when the obj is initialized, so you can skip objects that don't have
this flag set yet.