Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] Asymmetric SVM range refcounting leading to potential GPU Use-After-Free via SDMA queues. - [High] The gang_ctx_bo and wptr_bo_gart allocations in init_user_queue() are leaked when pqm_create_queue() fails during dev->dqm->ops.create_queue(). -- commit d34532449fcac18c405929eee012b2621c7a7eec Author: David Francis <[email protected]> drm/amdkfd: Properly acquire queue buffers in CRIU restore When kfd_queue_acquire_buffers() was split off from set_queue_properties_from_user(), set_queue_properties_from_criu() was missed. Thus, set_queue_properties_from_criu() is not filling out the buffer fields of queue_properties, which can come up when subsequent code expects them to be non-null. > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c > b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c > index 7fbb5c274ccc4..b32bd79e0d41f 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c [ ... ] > @@ -1053,10 +1053,18 @@ int kfd_criu_restore_queue(struct kfd_process *p, > memset(&qp, 0, sizeof(qp)); > set_queue_properties_from_criu(&qp, q_data, > NUM_XCC(pdd->dev->adev->gfx.xcc_mask)); > > + ret = kfd_queue_acquire_buffers(pdd, &qp); [Severity: Critical] This is a pre-existing issue, but does the SVM range refcounting asymmetry in kfd_queue_acquire_buffers() and kfd_queue_release_buffers() lead to a potential use-after-free for SDMA queues? If a local user creates a COMPUTE queue using an SVM range as the CWSR area, it increments the queue_refcount. If the user then creates an SDMA queue passing the same SVM range address, kfd_queue_acquire_buffers() skips kfd_queue_buffer_svm_get() because it's not a compute queue: drivers/gpu/drm/amd/amdkfd/kfd_queue.c:kfd_queue_acquire_buffers() { ... /* only compute queue requires EOP buffer and CWSR area */ if (properties->type != KFD_QUEUE_TYPE_COMPUTE) goto out_unreserve; ... } However, when the SDMA queue is destroyed, kfd_queue_release_buffers() unconditionally drops the refcount: drivers/gpu/drm/amd/amdkfd/kfd_queue.c:kfd_queue_release_buffers() { ... kfd_queue_buffer_svm_put(pdd, properties->ctx_save_restore_area_address, total_cwsr_size); return 0; } When the memory is unmapped, svm_range_unmap_from_gpus() will see a refcount of 0 and skip quiescing the COMPUTE queue, potentially allowing it to continue writing to physical pages after they are returned to the OS. > + if (ret) { > + pr_debug("failed to acquire user queue buffers for CRIU\n"); > + goto exit; > + } > + > print_queue_properties(&qp); > > ret = pqm_create_queue(&p->pqm, pdd->dev, &qp, &queue_id, q_data, mqd, > ctl_stack, NULL); [Severity: High] This isn't a bug introduced by this patch, but does pqm_create_queue() leak the gang_ctx_bo and wptr_bo_gart allocations if dev->dqm->ops.create_queue() fails? In the error handling path: drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c:pqm_create_queue() { ... err_create_queue: uninit_queue(q); if (kq) kernel_queue_uninit(kq); kfree(pqn); ... } The err_create_queue path calls uninit_queue(q), which kfrees the queue structure, but it fails to call pqm_clean_queue_resource(). This leaks the GTT allocations made earlier in init_user_queue(). A local user could repeatedly exhaust the queue limit to trigger this error path and cause a GTT memory leak. > if (ret) { > + kfd_queue_unref_bo_vas(pdd, &qp); > + kfd_queue_release_buffers(pdd, &qp); > pr_err("Failed to create new queue err:%d\n", ret); > goto exit; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
