Am 30.10.2017 um 17:23 schrieb Felix Kuehling: > On 2017-10-28 10:44 AM, Christian König wrote: >> Am 28.10.2017 um 01:35 schrieb Felix Kuehling: >>> The kfd_process doesn't own a reference to the mm_struct, so it can >>> disappear without warning even while the kfd_process still exists. >>> >>> Therefore, avoid dereferencing the kfd_process.mm pointer and make >>> it opaque. Use get_task_mm to get a temporary reference to the mm >>> when it's needed. >>> >>> v2: removed unnecessary WARN_ON >>> >>> Signed-off-by: Felix Kuehling <Felix.Kuehling at amd.com> >>> Reviewed-by: Oded Gabbay <oded.gabbay at gmail.com> (v1) >>> --- >>>  drivers/gpu/drm/amd/amdkfd/kfd_events.c | 19 +++++++++++++++---- >>>  drivers/gpu/drm/amd/amdkfd/kfd_priv.h   | 7 ++++++- >>>  drivers/gpu/drm/amd/amdkfd/kfd_process.c | 1 - >>>  3 files changed, 21 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c >>> b/drivers/gpu/drm/amd/amdkfd/kfd_events.c >>> index 944abfa..61ce547 100644 >>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c >>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c >>> @@ -24,8 +24,8 @@ >>>  #include <linux/slab.h> >>>  #include <linux/types.h> >>>  #include <linux/sched/signal.h> >>> +#include <linux/sched/mm.h> >>>  #include <linux/uaccess.h> >>> -#include <linux/mm.h> >>>  #include <linux/mman.h> >>>  #include <linux/memory.h> >>>  #include "kfd_priv.h" >>> @@ -904,14 +904,24 @@ void kfd_signal_iommu_event(struct kfd_dev >>> *dev, unsigned int pasid, >>>       * running so the lookup function returns a locked process. >>>       */ >>>      struct kfd_process *p = kfd_lookup_process_by_pasid(pasid); >>> +   struct mm_struct *mm; >>>       if (!p) >>>          return; /* Presumably process exited. */ >>>  +   /* Take a safe reference to the mm_struct, which may otherwise >>> +    * disappear even while the kfd_process is still referenced. >>> +    */ >>> +   mm = get_task_mm(p->lead_thread); >>> +   if (!mm) { >>> +       mutex_unlock(&p->mutex); >>> +       return; /* Process is exiting */ >>> +   } >>> + >>>      memset(&memory_exception_data, 0, sizeof(memory_exception_data)); >>>  -   down_read(&p->mm->mmap_sem); >>> -   vma = find_vma(p->mm, address); >>> +   down_read(&mm->mmap_sem); >>> +   vma = find_vma(mm, address); >>>       memory_exception_data.gpu_id = dev->id; >>>      memory_exception_data.va = address; >>> @@ -937,7 +947,8 @@ void kfd_signal_iommu_event(struct kfd_dev *dev, >>> unsigned int pasid, >>>          } >>>      } >>>  -   up_read(&p->mm->mmap_sem); >>> +   up_read(&mm->mmap_sem); >>> +   mmput(mm); >>>       mutex_lock(&p->event_mutex); >>>  diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h >>> b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h >>> index 7d86ec9..1a483a7 100644 >>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h >>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h >>> @@ -494,7 +494,12 @@ struct kfd_process { >>>       */ >>>      struct hlist_node kfd_processes; >>>  -   struct mm_struct *mm; >>> +   /* >>> +    * Opaque pointer to mm_struct. We don't hold a reference to >>> +    * it so it should never be dereferenced from here. This is >>> +    * only used for looking up processes by their mm. >>> +    */ >>> +   void *mm; >> Using a void* here still doesn't look like the right thing to do. > For added context, this is how it's used: > > static struct kfd_process *create_process(const struct task_struct *thread) > { > ... > process->mm = thread->mm; > ... > hash_add_rcu(kfd_processes_table, &process->kfd_processes, > (uintptr_t)process->mm); > ... > } > > static struct kfd_process *find_process_by_mm(const struct mm_struct *mm) > { > struct kfd_process *process; > > hash_for_each_possible_rcu(kfd_processes_table, process, > kfd_processes, (uintptr_t)mm) > if (process->mm == mm) > return process; > > return NULL; > } In this case I would make p->mm a uintptr_t and not void*. I would use void* only if you really don't know what pointer it is. Anyway the patch is fine with me as it is, just wanted to understand what's happening here. Regards, Christian. > > >>>       struct mutex mutex; >>>  diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c >>> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c >>> index 3ccb3b5..695fa2a 100644 >>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c >>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c >>> @@ -200,7 +200,6 @@ static void kfd_process_destroy_delayed(struct >>> rcu_head *rcu) >>>      struct kfd_process *p; >>>       p = container_of(rcu, struct kfd_process, rcu); >>> -   WARN_ON(atomic_read(&p->mm->mm_count) <= 0); >>>       mmdrop(p->mm); >> If you are concerned that p->mm is used after this just set it to NULL. > I'm not concerned about it. > > Regards, >  Felix > >> Regards, >> Christian. >> >>> >>