drm/amdkfd: fix a use after free race with mmu_notififer unregister

When using mmu_notififer_unregister_no_release() the caller must ensure there is a SRCU synchronize before the mn memory is freed, otherwise use after free races are possible, for instance:

      CPU0                                      CPU1
                                       invalidate_range_start
                                          hlist_for_each_entry_rcu(..)
  mmu_notifier_unregister_no_release(&p->mn)
  kfree(mn)
                                       if (mn->ops->invalidate_range_end)

The error unwind in amdkfd misses the SRCU synchronization.

amdkfd keeps the kfd_process around until the mm is released, so split the flow to fully initialize the kfd_process and register it for find_process, and with the notifier. Past this point the kfd_process does not need to be cleaned up as it is fully ready.

The final failable step does a vm_mmap() and does not seem to impact the kfd_process global state. Since it also cannot be undone (and already has problems with undo if it internally fails), it has to be last.

This way we don't have to try to unwind the mmu_notifier_register() and avoid the problem with the SRCU.

Along the way this also fixes various other error unwind bugs in the flow.

Fixes 45102048f77e ("amdkfd: Add process queue manager module")

Signed-off-by Jason Gunthorpe <[email protected]>

---
  drivers/gpu/drm/amd/amdkfd/kfd_process.c | 74 +++++++++++-------------
  1 file changed, 35 insertions(+), 39 deletions(-)

amdkfd folks, this little bug is blocking some rework I have for the mmu notifiers (ie mm/mmu_notifiers: remove unregister_no_release)

Can I get your help to review and if needed polish this change? I'd like to send this patch through the hmm tree along with the rework, thanks

You can see the larger series here:

https://github.com/jgunthorpe/linux/commits/mmu_notifier

Jason

drivers/gpu/drm/amd/amdkfd/kfd_process.c +27 -29

index 8f1076c0c88a25..81e3ee3f1813bf 100644
@@ -62,8 +62,8 @@ static struct workqueue_struct *kfd_restore_wq;
 
  static struct kfd_process *find_process(const struct task_struct *thread);
  static void kfd_process_ref_release(struct kref *ref);
-static struct kfd_process *create_process(const struct task_struct *thread,
-					struct file *filep);
+static struct kfd_process *create_process(const struct task_struct *thread);
+static int kfd_process_init_cwsr_apu(struct kfd_process *p, struct file *filep);
 
  static void evict_process_worker(struct work_struct *work);
  static void restore_process_worker(struct work_struct *work);

@@ -289,7 +289,15 @@ struct kfd_process *kfd_create_process(struct file *filep)
  	if (process) {
  		pr_debug("Process already found\n");
  	} else {
-		process = create_process(thread, filep);
+		process = create_process(thread);
+		if (IS_ERR(process))
+			goto out;
+
+		ret = kfd_process_init_cwsr_apu(process, filep);
+		if (ret) {
+			process = ERR_PTR(ret);
+			goto out;
+		}
 
  		if (!procfs.kobj)
  			goto out;

@@ -609,64 +617,56 @@ static int kfd_process_device_init_cwsr_dgpu(struct kfd_process_device *pdd)
  	return 0;
  }
 
-static struct kfd_process *create_process(const struct task_struct *thread,
-					struct file *filep)
+/*
+ * On return the kfd_process is fully operational and will be freed when the
+ * mm is released
+ */
+static struct kfd_process *create_process(const struct task_struct *thread)
  {
  	struct kfd_process *process;
  	int err = -ENOMEM;
 
  	process = kzalloc(sizeof(*process), GFP_KERNEL);
-
  	if (!process)
  		goto err_alloc_process;
 
-	process->pasid = kfd_pasid_alloc();
-	if (process->pasid == 0)
-		goto err_alloc_pasid;
-
-	if (kfd_alloc_process_doorbells(process) < 0)
-		goto err_alloc_doorbells;
-
  	kref_init(&process->ref);
-
  	mutex_init(&process->mutex);
-
  	process->mm = thread->mm;
-
-	/* register notifier */
-	process->mmu_notifier.ops = &kfd_process_mmu_notifier_ops;
-	err = mmu_notifier_register(&process->mmu_notifier, process->mm);
-	if (err)
-		goto err_mmu_notifier;
-
-	hash_add_rcu(kfd_processes_table, &process->kfd_processes,
-			(uintptr_t)process->mm);
-
  	process->lead_thread = thread->group_leader;
-	get_task_struct(process->lead_thread);
-
  	INIT_LIST_HEAD(&process->per_device_data);
-
+	INIT_DELAYED_WORK(&process->eviction_work, evict_process_worker);
+	INIT_DELAYED_WORK(&process->restore_work, restore_process_worker);
+	process->last_restore_timestamp = get_jiffies_64();
  	kfd_event_init_process(process);
+	process->is_32bit_user_mode = in_compat_syscall();
+
+	process->pasid = kfd_pasid_alloc();
+	if (process->pasid == 0)
+		goto err_alloc_pasid;
+
+	if (kfd_alloc_process_doorbells(process) < 0)
+		goto err_alloc_doorbells;
 
  	err = pqm_init(&process->pqm, process);
  	if (err != 0)
  		goto err_process_pqm_init;
 
  	/* init process apertures*/
-	process->is_32bit_user_mode = in_compat_syscall();
  	err = kfd_init_apertures(process);
  	if (err != 0)
  		goto err_init_apertures;
 

drivers/gpu/drm/amd/amdkfd/kfd_process.c +4 -3

+	get_task_struct(process->lead_thread);
+	hash_add_rcu(kfd_processes_table, &process->kfd_processes,
+			(uintptr_t)process->mm);
+
  	return process;
 
  err_init_cwsr:

@@ -675,15 +675,11 @@ static struct kfd_process *create_process(const struct task_struct *thread,
  err_init_apertures:
  	pqm_uninit(&process->pqm);
  err_process_pqm_init:
-	hash_del_rcu(&process->kfd_processes);
-	synchronize_rcu();
-	mmu_notifier_unregister_no_release(&process->mmu_notifier, process->mm);

Btw, who maintains amkfd these days? MAINTAINERS still lists Oded, but he seems to have moved on to Habanalabs and maintains that drivers now while not having any action on amdkfd for over a year.