Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Kernel panic due to NULL pointer dereferences in amdgpu_vm_get_task_info_vm() and amdgpu_vm_fini() when vm->task_info allocation fails. - [High] Potential Use-After-Free of amdgpu_vm in amdgpu_vm_get_task_info_pasid. -- commit ac56a0fa84d860ce75aeb92f4bc1e57cf68cb0d2 Author: Yongqiang Sun <[email protected]> drm/amdkfd: fix SMI event cross-process information leak This commit fixes a cross-process information leak by passing the target process PID into kfd_smi_event_add() for system-wide events. This ensures that the existing per-client filter properly restricts delivery to the owning process instead of bypassing the filter with a PID of 0. > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c > b/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c > index a499449fcb068..d6e737ab10076 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c > @@ -254,8 +254,10 @@ void kfd_smi_event_update_vmfault(struct kfd_node *dev, > uint16_t pasid) > if (task_info) { [Severity: High] This isn't a regression introduced by this patch, but can the retrieval of task_info just above this line lead to a Use-After-Free of the amdgpu_vm pointer? In the call to amdgpu_vm_get_task_info_pasid(), it retrieves the vm pointer from the pasids xarray and immediately drops the lock before passing the bare pointer to amdgpu_vm_get_task_info_vm(): amdgpu_vm_get_task_info_pasid() { ... return amdgpu_vm_get_task_info_vm( amdgpu_vm_get_vm_from_pasid(adev, pasid)); } If the owning process concurrently exits and amdgpu_vm_fini() is called, the vm could be freed between dropping the xarray lock and dereferencing the pointer in amdgpu_vm_get_task_info_vm(). This could be triggered via concurrent VM fault interrupts and process teardown. > /* Report VM faults from user applications, not retry from > kernel */ > if (task_info->task.pid) > - kfd_smi_event_add(0, dev, KFD_SMI_EVENT_VMFAULT, > KFD_EVENT_FMT_VMFAULT( > - task_info->task.pid, > task_info->task.comm)); > + kfd_smi_event_add(task_info->tgid, dev, > + KFD_SMI_EVENT_VMFAULT, > + > KFD_EVENT_FMT_VMFAULT(task_info->task.pid, > + > task_info->task.comm)); > amdgpu_vm_put_task_info(task_info); > } > } [ ... ] > @@ -356,7 +358,7 @@ void kfd_smi_event_process(struct kfd_process_device > *pdd, bool start) > task_info = amdgpu_vm_get_task_info_vm(avm); [Severity: High] This is a pre-existing regression, but can this call result in a kernel panic due to a NULL pointer dereference? Looking at amdgpu_vm_get_task_info_vm(), it checks if the vm pointer is valid but unconditionally accesses vm->task_info: amdgpu_vm_get_task_info_vm() { ... if (vm) { ti = vm->task_info; kref_get(&vm->task_info->refcount); } return ti; } If the memory allocation fails during initialization in amdgpu_vm_init(), the error is logged but execution continues: amdgpu_vm_init() { ... r = amdgpu_vm_create_task_info(vm); if (r) dev_dbg(adev->dev, "Failed to create task info for VM\n"); ... } This leaves the vm->task_info pointer as NULL, which will cause a crash when kref_get() is called here. Additionally, this missing NULL check can cause another dereference during VM destruction in amdgpu_vm_fini() when formatting the warning message: amdgpu_vm_fini() { ... dev_warn(adev->dev, "VM memory stats for proc %s(%d) task %s(%d) is non-zero when fini\n", ti->process_name, ti->task.pid, ti->task.comm, ti->tgid); ... } > > if (task_info) { > - kfd_smi_event_add(0, pdd->dev, > + kfd_smi_event_add(task_info->tgid, pdd->dev, > start ? KFD_SMI_EVENT_PROCESS_START : > KFD_SMI_EVENT_PROCESS_END, > KFD_EVENT_FMT_PROCESS(task_info->task.pid, -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
