On 2026-06-10 23:01, Geoffrey McRae wrote:
Move kfd_process_remove_sysfs() earlier in
kfd_process_wq_release() so that all sysfs/procfs entries are
removed before tearing down PDDs and dropping lead_thread.
The per-process sysfs attributes are backed by struct
kfd_process_device, and their show/store callbacks dereference
PDD fields. Since sysfs removal waits for active callbacks to
complete, removing these entries first closes a race where
userspace reads sdma_* and stats_* files after PDD teardown.
This race caused NULL pointer dereferences observed in
kfd_sdma_activity_worker and kfd_procfs_stats_show.
Also harden kfd_process_remove_sysfs() against partially
initialized or already-freed objects:
- Check kobj_queues before removing PASID and deleting it
- Skip NULL pdd entries
- Guard kobj_stats and kobj_counters before use
These checks prevent invalid dereferences during cleanup.
Fixes: NULL pointer dereference in KFD sysfs/procfs stats paths
Change-Id: I405b8fb95d3c5e163dfc45928da54f31546d92cc
Cc: Felix Kuehling <[email protected]>
Cc: Alex Deucher <[email protected]>
Signed-off-by: Geoffrey McRae <[email protected]>
---
drivers/gpu/drm/amd/amdkfd/kfd_process.c | 44 +++++++++++++++---------
1 file changed, 28 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index d28ca581cad0..b47e7dac8b2d 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -1160,28 +1160,36 @@ static void kfd_process_remove_sysfs(struct kfd_process
*p)
if (!p->kobj)
return;
- sysfs_remove_file(p->kobj, &p->attr_pasid);
- kobject_del(p->kobj_queues);
- kobject_put(p->kobj_queues);
- p->kobj_queues = NULL;
+ if (p->kobj_queues) {
+ sysfs_remove_file(p->kobj, &p->attr_pasid);
+ kobject_del(p->kobj_queues);
+ kobject_put(p->kobj_queues);
+ p->kobj_queues = NULL;
+ }
for (i = 0; i < p->n_pdds; i++) {
pdd = p->pdds[i];
+ if (!pdd)
+ continue;
I agree with Kent that this is probably overly paranoid since you are
fixing the sequence. kfd_create_process_device_data initializes the pdds
in such a way that it should be impossible to find a NULL-pointer in
this array.
sysfs_remove_file(p->kobj, &pdd->attr_vram);
sysfs_remove_file(p->kobj, &pdd->attr_sdma);
- sysfs_remove_file(pdd->kobj_stats, &pdd->attr_evict);
- if (pdd->dev->kfd2kgd->get_cu_occupancy)
- sysfs_remove_file(pdd->kobj_stats,
- &pdd->attr_cu_occupancy);
- kobject_del(pdd->kobj_stats);
- kobject_put(pdd->kobj_stats);
- pdd->kobj_stats = NULL;
+ if (pdd->kobj_stats) {
+ sysfs_remove_file(pdd->kobj_stats, &pdd->attr_evict);
+ if (pdd->dev->kfd2kgd->get_cu_occupancy)
+ sysfs_remove_file(pdd->kobj_stats,
+ &pdd->attr_cu_occupancy);
+ kobject_del(pdd->kobj_stats);
+ kobject_put(pdd->kobj_stats);
+ pdd->kobj_stats = NULL;
+ }
}
for_each_set_bit(i, p->svms.bitmap_supported, p->n_pdds) {
pdd = p->pdds[i];
+ if (!pdd || !pdd->kobj_counters)
+ continue;
Same here. pdd cannot be NULL. Checking pdd->kobj_counters is necessary,
though.
sysfs_remove_file(pdd->kobj_counters, &pdd->attr_faults);
sysfs_remove_file(pdd->kobj_counters, &pdd->attr_page_in);
@@ -1239,6 +1247,15 @@ static void kfd_process_wq_release(struct work_struct
*work)
kfd_debugfs_remove_process(p);
+ /*
+ * Remove proc/sysfs entries before tearing down PDDs or dropping
+ * lead_thread. The per-process sysfs attributes are embedded in
+ * struct kfd_process_device and the show callbacks dereference PDD
+ * fields. sysfs removal waits for active show/store callbacks, so this
+ * closes a race with userspace reading sdma_*/stats_* files.
It's actually worse because kfd_process_remove_sysfs itself is
dereferencing pdd pointers. I guess the only reason that doesn't blow up
is because kfd_process_destroy_pdds also sets p->n_pdds = 0. So the
sysfs cleanup never worked as intended. You may want to update the patch
description and this commend with that in mind. Anyway, the patch looks
good. With the two nit-picks above fixed, the patch is
Reviewed-by: Felix Kuehling <[email protected]>
+ */
+ kfd_process_remove_sysfs(p);
+
kfd_process_kunmap_signal_bo(p);
kfd_process_free_outstanding_kfd_bos(p);
svm_range_list_fini(p);
@@ -1252,11 +1269,6 @@ static void kfd_process_wq_release(struct work_struct
*work)
put_task_struct(p->lead_thread);
- /* the last step is removing process entries under /sys
- * to indicate the process has been terminated.
- */
- kfd_process_remove_sysfs(p);
-
kfree(p);
}
--
2.43.0
