Hi Tvrtko,
On 06/04/26 12:00, Tvrtko Ursulin wrote:
On 02/04/2026 19:36, Maíra Canal wrote:
This is a preparation commit for per-file descriptor seqno tracking,
which will require the struct vc4_render_job to store a pointer to
vc4_file. Since the scheduler's free_job callback runs asynchronously,
it can execute after vc4_close() has already freed the vc4_file, causing
a use-after-free.
Add kref-based reference counting to vc4_file so that jobs can safely
take a reference at submit time via vc4_file_get() and drop it in their
free callback via vc4_file_put().
Signed-off-by: Maíra Canal <[email protected]>
---
drivers/gpu/drm/vc4/vc4_drv.c | 23 +++++++++++++++++++++--
drivers/gpu/drm/vc4/vc4_drv.h | 4 ++++
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/
vc4_drv.c
index
cacd4cb942593b44f2210b7738bd964ebc34522d..d0e515c900af6e7400a96e42a9141dab5d5bca2d 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -167,6 +167,7 @@ static int vc4_open(struct drm_device *dev, struct
drm_file *file)
goto err_sched;
}
+ kref_init(&vc4file->refcount);
vc4_perfmon_open_file(vc4file);
file->driver_priv = vc4file;
@@ -180,6 +181,25 @@ static int vc4_open(struct drm_device *dev,
struct drm_file *file)
}
+static void vc4_file_release(struct kref *ref)
+{
+ struct vc4_file *vc4file = container_of(ref, struct vc4_file,
refcount);
+
+ vc4_perfmon_close_file(vc4file);
Access of vc4->active_perfmon is not clear to me. When I was looking at
the earlier patches I assumed it is protected by the run_job /
workqueue. But from the angle of a delayed vc4_file release I am not
sure why it is safe.
vc4_perfmon_close_file
-> vc4_perfmon_delete
/* If the active perfmon is being destroyed, stop it first */
if (perfmon == vc4->active_perfmon)
vc4_perfmon_stop(vc4, perfmon, false);
Why is vc4->active_perfmon stable here? Something guarantees the
perfmons belonging to this file (vc4file->perfmons) will not be in the
xarray any longer if it was the active one?
Yeah, this is a pre-existing problem that also exists in v3d. I'm not
sure if it's worth it to fix it in this series, but it's something that
has been in my TODO list for a while. To fix it, I'd prefer to examine
this problem separately and check if I can simplify the management
somehow instead of just adding a lock to protect active_perfmon. That's
why I believe it deserves a separate series.
If you prefer to have this fixed in this series, let me know and I can
work on it.
Thanks for the review!
Best regards,
- Maíra
