Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
New issues: - [Medium] The commit message describes a factually incorrect premise regarding `domain` initialization. Pre-existing issues: - [Critical] GEM objects exported via PRIME can outlive their originating DRM file descriptor, causing a use-after-free in `rocket_gem_bo_free`. - [Critical] Partial IOMMU mappings bypass the unmap error path in `rocket_ioctl_create_bo()`, leaking the mapping. - [High] An IOMMU group reference is leaked on every interrupt handling due to an unbalanced `iommu_group_get()` call. - [High] Calling `drm_sched_job_cleanup()` on an armed job corrupts the GPU scheduler timeline. - [High] The loop in `rocket_ioctl_submit()` completely ignores the return value of `rocket_ioctl_submit_job()`. - [High] Calling `memcpy` with a NULL pointer as the source when `job->in_bo_count` is 0. - [High] The `scheds` array is leaked in `rocket_job_open`. - [High] Incorrect teardown order in `rocket_job_fini` leads to Use-After-Free of the scheduler. - [High] Incorrect teardown order in `rocket_core_fini` causes NULL pointer dereferences for concurrent jobs. -- --- Patch [2]: [PATCH 2/4] accel/rocket: Fix the panic in rocket_iommu_domain_put when the domain is NULL --- commit 30af1c047db9952f07dbaa552d86ca1a67f4701b Author: Chaoyi Chen <[email protected]> accel/rocket: Fix the panic in rocket_iommu_domain_put when the domain is NULL This commit adds a null check in rocket_iommu_domain_put() to prevent a panic when cleaning up a job that failed early in submission. The commit message states this is necessary because the domain is assigned last in rocket_ioctl_submit_job(). > In rocket_ioctl_submit_job(), since the domain is assigned last, an > error before that triggers rocket_job_put() -> rocket_job_cleanup() > -> rocket_iommu_domain_put() with the domain still NULL, > causing a panic in that function. [Severity: Medium] This isn't a bug introduced by this patch, but the commit message describes a premise that doesn't match the code. Looking at rocket_ioctl_submit_job(), the domain is actually assigned immediately after allocation: drivers/accel/rocket/rocket_job.c:rocket_ioctl_submit_job() { ... rjob = kzalloc_obj(*rjob); if (!rjob) return -ENOMEM; kref_init(&rjob->refcount); rjob->rdev = rdev; rjob->domain = rocket_iommu_domain_get(file_priv); ... } Because the domain is assigned before any operations that could fail, the panic described in the commit message appears impossible in this specific path. Could the commit message be updated to clarify the exact failure path being fixed? > diff --git a/drivers/accel/rocket/rocket_drv.c > b/drivers/accel/rocket/rocket_drv.c > index 8bbbce594883e..77cd2ecce001e 100644 > --- a/drivers/accel/rocket/rocket_drv.c > +++ b/drivers/accel/rocket/rocket_drv.c > @@ -65,7 +65,8 @@ rocket_iommu_domain_get(struct rocket_file_priv > *rocket_priv) > void > rocket_iommu_domain_put(struct rocket_iommu_domain *domain) > { > - kref_put(&domain->kref, rocket_iommu_domain_destroy); > + if (domain) > + kref_put(&domain->kref, rocket_iommu_domain_destroy); > } > > static int [Severity: Critical] This is a pre-existing issue, but I noticed a potential use-after-free with GEM objects and PRIME. If a user exports a GEM buffer via dma-buf and closes the DRM file descriptor, rocket_postclose() unconditionally frees rocket_priv: drivers/accel/rocket/rocket_drv.c:rocket_postclose() { ... rocket_iommu_domain_put(rocket_priv->domain); kfree(rocket_priv); ... } Later, when the dma-buf is finally closed, rocket_gem_bo_free() will dereference the freed rocket_priv: drivers/accel/rocket/rocket_gem.c:rocket_gem_bo_free() { ... struct rocket_file_priv *rocket_priv = bo->driver_priv; ... mutex_lock(&rocket_priv->mm_lock); ... } Does this code need to reference count rocket_priv or tie its lifetime to the GEM objects rather than the DRM file? [Severity: Critical] This is a pre-existing issue, but there is a potential IOMMU mapping leak in the BO creation path: drivers/accel/rocket/rocket_gem.c:rocket_ioctl_create_bo() { ... ret = iommu_map_sgtable(rocket_priv->domain->domain, rkt_obj->mm.start, shmem_obj->sgt, IOMMU_READ | IOMMU_WRITE); if (ret < 0 || ret < args->size) { drm_err(dev, "failed to map buffer: size=%d request_size=%u\n", ret, args->size); ret = -ENOMEM; goto err_remove_node; } ... } If iommu_map_sgtable() returns a partial mapping where ret > 0 but ret < args->size, the code jumps directly to err_remove_node, bypassing err_unmap. Won't the successfully mapped pages remain in the IOMMU while the GEM pages are freed, allowing potential use-after-free corruption? [Severity: High] This is a pre-existing issue, but calling drm_sched_job_cleanup() on an armed job might corrupt the scheduler timeline. drivers/accel/rocket/rocket_job.c:rocket_job_push() { ... drm_sched_job_arm(&job->base); ... ret = rocket_acquire_object_fences(job->in_bos, job->in_bo_count, &job->base, false); if (ret) goto err_unlock; ... } If an error occurs after drm_sched_job_arm(), the code jumps to out_cleanup_job and cleans it up: drivers/accel/rocket/rocket_job.c:rocket_ioctl_submit_job() { ... out_cleanup_job: if (ret) drm_sched_job_cleanup(&rjob->base); ... } According to DRM scheduler rules, once a job is armed, it must be pushed to the entity. Doesn't cleaning it up directly leave a missing sequence number in the fence timeline? [Severity: High] This is a pre-existing issue, but there appears to be a reference leak in the interrupt handler: drivers/accel/rocket/rocket_job.c:rocket_job_handle_irq() { ... iommu_detach_group(NULL, iommu_group_get(core->dev)); ... } The call to iommu_group_get() increments the reference count, but iommu_detach_group() does not consume it. Won't this leak a reference on every handled interrupt, eventually pinning the group permanently? [Severity: High] This is a pre-existing issue, but the job submission loop ignores errors: drivers/accel/rocket/rocket_job.c:rocket_ioctl_submit() { ... for (i = 0; i < args->job_count; i++) rocket_ioctl_submit_job(dev, file, &jobs[i]); exit: kvfree(jobs); return ret; } If rocket_ioctl_submit_job() fails with an error like -ENOMEM, the loop ignores the return value and continues. The ioctl will eventually return 0 for success. Should this code check the return value and notify userspace of failures to prevent hangs when waiting on fences that will never be signaled? [Severity: High] This is a pre-existing issue, but there is a risk of undefined behavior when copying job buffers: drivers/accel/rocket/rocket_job.c:rocket_job_push() { ... bos = kvmalloc_array(bo_count, sizeof(*bos), GFP_KERNEL); if (!bos) return -ENOMEM; memcpy(bos, job->in_bos, job->in_bo_count * sizeof(void *)); ... } If a job is submitted with in_bo_count = 0 but out_bo_count > 0, job->in_bos remains null. This leads to memcpy() being called with a null source pointer, which is undefined behavior. Should this code check if in_bo_count is greater than 0 before the copy? [Severity: High] This is a pre-existing issue, but the scheduler array appears to be leaked: drivers/accel/rocket/rocket_job.c:rocket_job_open() { ... struct drm_gpu_scheduler **scheds = kmalloc_objs(*scheds, rdev->num_cores); ... ret = drm_sched_entity_init(&rocket_priv->sched_entity, DRM_SCHED_PRIORITY_NORMAL, scheds, rdev->num_cores, NULL); ... } If drm_sched_entity_init() fails, it returns early without freeing scheds. Additionally, if rdev->num_cores is 1, drm_sched_entity_init() might not take ownership of the array, meaning the allocation is orphaned and cannot be freed in rocket_job_close(). Does this array need to be freed if it's not consumed or if an error occurs? [Severity: High] This is a pre-existing issue, but there seems to be a teardown ordering problem in the job destruction path: drivers/accel/rocket/rocket_job.c:rocket_job_fini() { drm_sched_fini(&core->sched); cancel_work_sync(&core->reset.work); ... } drm_sched_fini() is called before the background reset work is canceled. If the reset work runs concurrently, rocket_reset() might invoke drm_sched_stop() and drm_sched_start() on a destroyed scheduler. Should the background work be canceled before the scheduler is torn down? [Severity: High] This is a pre-existing issue, but I noticed another teardown ordering problem in the core destruction path: drivers/accel/rocket/rocket_core.c:rocket_core_fini() { ... iommu_group_put(core->iommu_group); core->iommu_group = NULL; rocket_job_fini(core); } Critical resources like core->iommu_group are torn down before stopping the DRM scheduler in rocket_job_fini(). If the scheduler dispatches a job concurrently, won't rocket_job_run() attempt to use a null iommu_group? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
