Re: [PATCH] drm/amdkfd: fix use-after-free in create_process() error path

["Kuehling, Felix" <felix.kuehling@amd.com>]

On 2026-11-08 05:56, Hongyan Xu wrote:
> create_process() publishes the process in kfd_processes_table before its
> last failing call (alloc_notifier() must find it there), and its error path
> frees it with a bare kfree(), ignoring the refcount.  find_process() takes
> its reference inside the sRCU critical section but callers use the process
> after leaving it, so synchronize_srcu() misses them, and the error path
> cannot unref once either: it holds two (kref_init() and the one keeping
> free_notifier() from starting kfd_process_wq_release()).
>
> Release the process as soon as it is visible: take the lead thread
> reference right after lead_thread is assigned, run ida_init() before
> mmu_notifier_get(), so the release work is safe on a half-built object.
> The published error paths unlink with hash_del_rcu(), synchronize_srcu()
> and drop both references, so the last one runs kfd_process_wq_release().
> Both labels hold exactly two references, so nothing is freed early and no
> path falls into the unwind the unpublished labels keep, now with
> put_task_struct()).  Restore pqm_uninit() (kfd_process_queue_manager.c):
> pqm_init()'s 128 byte queue slot bitmap leaked per failure.  Match
> kfd_debugfs_remove_process() by process, not pid.
>
> Fixes: 3248b6d3cbcd ("drm/amdkfd: Use mmu_notifier_get")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hongyan Xu <getshell@seu.edu.cn>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_debugfs.c | 12 ++++++-
>   drivers/gpu/drm/amd/amdkfd/kfd_process.c | 46 ++++++++++++++++++++++--
>   2 files changed, 54 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_debugfs.c b/drivers/gpu/drm/amd/amdkfd/kfd_debugfs.c
> index 7d4e07452cdb..a17a3a307266 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_debugfs.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_debugfs.c
> @@ -33,6 +33,7 @@ static struct list_head procs;
>   struct debugfs_proc_entry {
>   	struct list_head list;
>   	struct dentry *proc_dentry;
> +	struct kfd_process *process;

Hi Hongyan,

Thank you for working on this. It looks like your patch is based on an 
outdated version of the branch. Current master branch already has a fix 
for this part of your patch:

commit 325de6bf3328efd5acf5072337fc196928753de8
Author: Zhu Lingshan <lingshan.zhu@amd.com>
Date:   Wed Jul 1 11:03:09 2026 +0800

     drm/amdkfd: expose pasid of secondary contexts by debugfs

     Current kfd debugfs interfaces only expose pasid
     of the primary process, this commit exposes
     pasid of secondary contexts by debugfs

     Just like entries under sysfs,
     the secondary contexts are named as
     context_<id> under its primary kfd process.

     The layout:
     /sys/kernel/debug/kfd/proc# tree
     .
     └── 5802
         ├── context_0
         │   ├── pasid_1025
         │   └── pasid_63266
         ├── context_1
         │   ├── pasid_1025
         │   └── pasid_63266
         ├── pasid_1025
         └── pasid_63266

     Another fix is, kfd_debugfs_add_process may fail,
     this commit change it to return a meaningful
     value other than void

     Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
     Reviewed-by: Felix Kuehling <felix.kuehling@amd.com>
     Signed-off-by: Alex Deucher <alexander.deucher@amd.com>


>   	pid_t pid;
>   };
>   
> @@ -151,6 +152,7 @@ void kfd_debugfs_add_process(struct kfd_process *p)
>   		return;
>   
>   	list_add(&entry->list, &procs);
> +	entry->process = p;
>   	entry->pid = p->lead_thread->pid;
>   	snprintf(name, MAX_DEBUGFS_FILENAME_LEN, "%d",
>   		 (int)entry->pid);
> @@ -174,9 +176,17 @@ void kfd_debugfs_remove_process(struct kfd_process *p)
>   {
>   	struct debugfs_proc_entry *entry, *next;
>   
> +	/*
> +	 * Match the entry that was added for this process and not one that
> +	 * merely has the same pid: kfd_process_wq_release() also runs for a
> +	 * process that failed in create_process() and never got an entry,
> +	 * and after an exec(3) the old and the new primary kfd_process of a
> +	 * process have the same lead thread while a file descriptor keeps
> +	 * the old one alive.
> +	 */
>   	mutex_lock(&kfd_processes_mutex);
>   	list_for_each_entry_safe(entry, next, &procs, list) {
> -		if (entry->pid != p->lead_thread->pid)
> +		if (entry->process != p)
>   			continue;
>   
>   		debugfs_remove_recursive(entry->proc_dentry);
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index 12e24fbf8c46..f118f7f03de7 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -1600,6 +1600,11 @@ struct kfd_process *create_process(const struct task_struct *thread, bool primar
>   	mutex_init(&process->mutex);
>   	process->mm = thread->mm;
>   	process->lead_thread = thread->group_leader;
> +	/*
> +	 * Take the reference on the lead thread before the process can be
> +	 * released through kfd_process_wq_release(), which drops it again.
> +	 */
> +	get_task_struct(process->lead_thread);
>   	process->n_pdds = 0;
>   	process->queues_paused = false;
>   
> @@ -1648,13 +1653,17 @@ struct kfd_process *create_process(const struct task_struct *thread, bool primar
>   	 */
>   	if (primary) {
>   		process->context_id = KFD_CONTEXT_ID_PRIMARY;
> +		/*
> +		 * Must be initialized before the process can be given to
> +		 * kfd_process_wq_release(), which destroys it.
> +		 */
> +		ida_init(&process->id_table);
>   		mn = mmu_notifier_get(&kfd_process_mmu_notifier_ops, process->mm);
>   		if (IS_ERR(mn)) {
>   			err = PTR_ERR(mn);
>   			goto err_register_notifier;
>   		}
>   		BUG_ON(mn != &process->mmu_notifier);
> -		ida_init(&process->id_table);
>   	}
>   
>   	err = kfd_process_alloc_id(process);
> @@ -1664,7 +1673,6 @@ struct kfd_process *create_process(const struct task_struct *thread, bool primar
>   	}
>   
>   	kfd_unref_process(process);
> -	get_task_struct(process->lead_thread);
>   
>   	INIT_WORK(&process->debug_event_workarea, debug_event_write_work_handler);
>   
> @@ -1673,8 +1681,39 @@ struct kfd_process *create_process(const struct task_struct *thread, bool primar
>   err_alloc_id:
>   	kfd_process_free_id(process);
>   err_register_notifier:
> +	/*
> +	 * The process is already in kfd_processes_table, so find_process()
> +	 * may have handed it out with a reference taken on it.  Unlink it
> +	 * and wait for the readers that are still walking the table before
> +	 * dropping the references held here: afterwards no new reference
> +	 * can be taken and the ones already taken keep the process alive.
> +	 * The memory is then freed by kfd_process_wq_release() when the
> +	 * last reference is dropped.
> +	 */
>   	hash_del_rcu(&process->kfd_processes);
> -	svm_range_list_fini(process);
> +	synchronize_srcu(&kfd_processes_srcu);
> +
> +	/*
> +	 * kfd_process_wq_release() frees the memory, but it does not
> +	 * destroy the queue manager: on the normal path that is done by
> +	 * kfd_process_notifier_release_internal(), which neither of these
> +	 * two error paths can reach, because no mmu notifier was
> +	 * registered for the process.  The process is unlinked and no
> +	 * reader can reach the pqm any more, so destroy it here, before
> +	 * the references are dropped.
> +	 */
> +	pqm_uninit(&process->pqm);
> +
> +	/*
> +	 * Drop the reference that keeps a failing free_notifier() from
> +	 * starting kfd_process_wq_release() and the one taken by
> +	 * kref_init().
> +	 */
> +	kfd_unref_process(process);
> +	kfd_unref_process(process);
> +
> +	return ERR_PTR(err);
> +
>   err_init_svm_range_list:
>   	kfd_process_free_outstanding_kfd_bos(process);
>   	kfd_process_destroy_pdds(process);
> @@ -1684,6 +1723,7 @@ struct kfd_process *create_process(const struct task_struct *thread, bool primar
>   	kfd_event_free_process(process);
>   err_event_init:
>   	mutex_destroy(&process->mutex);
> +	put_task_struct(process->lead_thread);

The changes in kfd_process.c look reasonable. I'll try to apply them 
without the changes in kfd_debugfs.c.

Regards,
   Felix


>   	kfree(process);
>   err_alloc_process:
>   	return ERR_PTR(err);