AMD General Comment inline
> -----Original Message----- > From: amd-gfx <[email protected]> On Behalf Of Geoffrey > McRae > Sent: June 10, 2026 11:02 PM > To: Kuehling, Felix <[email protected]> > Cc: Deucher, Alexander <[email protected]>; amd- > [email protected]; McRae, Geoffrey <[email protected]> > Subject: [PATCH] drm/amdkfd: Fix NULL deref during sysfs teardown > > 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'm not sure about this check. Did you see a case where pdd was NULL? I think the rest of the patch should cover the case you saw. Kent > > 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; > > 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. > + */ > + 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
