Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Time-of-check to time-of-use (TOCTOU) race condition in rocket_job_open() leading to kernel panic via ZERO_SIZE_PTR dereference. - [High] The ioctl submit handler aborts an armed scheduler job if fence acquisition fails, violating the drm_sched_job_arm() API contract. - [High] Unbalanced iommu_group reference acquisition causes a resource leak in the interrupt handler. -- commit 3814d4c84d6491257e9ab5433448c08da2327e1e Author: Triet Hoang <[email protected]> drm/rocket: Keep scheduler allocation in rocket_file_priv The scheduler array passed to drm_sched_entity_init() is retained by the entity when the device has multiple cores. This commit fixes a memory leak for single-core devices by keeping the allocation in rocket_file_priv. Link: https://lore.kernel.org/all/[email protected]/ > diff --git a/drivers/accel/rocket/rocket_drv.h > b/drivers/accel/rocket/rocket_drv.h > index 2c673bb99ccc1..9421e48ec5d84 100644 > --- a/drivers/accel/rocket/rocket_drv.h > +++ b/drivers/accel/rocket/rocket_drv.h > @@ -23,6 +23,7 @@ struct rocket_file_priv { > struct drm_mm mm; > struct mutex mm_lock; > > + struct drm_gpu_scheduler **scheds; > struct drm_sched_entity sched_entity; > }; > > diff --git a/drivers/accel/rocket/rocket_job.c > b/drivers/accel/rocket/rocket_job.c > index ffa0f8db5da65..ffb3cf5684c1e 100644 > --- a/drivers/accel/rocket/rocket_job.c > +++ b/drivers/accel/rocket/rocket_job.c > @@ -528,6 +528,8 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv) > rdev->num_cores, NULL); > if (ret) > kfree(scheds); > + else > + rocket_priv->scheds = scheds; > > return ret; > } [Severity: High] This isn't a bug introduced by this patch, but is it possible for rdev->num_cores to change concurrently during rocket_job_open()? If userspace rapidly opens the DRM device while the driver is still probing cores, could rdev->num_cores increase between the allocation and the loop? drivers/accel/rocket/rocket_job.c:rocket_job_open() { ... struct drm_gpu_scheduler **scheds = kmalloc_objs(*scheds, rdev->num_cores); ... for (core = 0; core < rdev->num_cores; core++) scheds[core] = &rdev->cores[core].sched; ... } If rdev->num_cores changes from 0 to 1 after the allocation, kmalloc_objs() will return ZERO_SIZE_PTR, and the loop will immediately write to scheds[0], causing a kernel panic. > @@ -536,8 +538,8 @@ void rocket_job_close(struct rocket_file_priv > *rocket_priv) > { > struct drm_sched_entity *entity = &rocket_priv->sched_entity; > > - kfree(entity->sched_list); > drm_sched_entity_destroy(entity); > + kfree(rocket_priv->scheds); > } [Severity: High] This is a pre-existing issue, but does the error handling in rocket_job_push() violate the drm_sched_job_arm() API contract? drivers/accel/rocket/rocket_job.c:rocket_job_push() { ... drm_sched_job_arm(&job->base); job->inference_done_fence = dma_fence_get(&job->base.s_fence->finished); ret = rocket_acquire_object_fences(job->in_bos, job->in_bo_count, &job->base, false); if (ret) goto err_unlock; ... } If rocket_acquire_object_fences() fails after the job is armed, the function bails out without calling drm_sched_entity_push_job(). The submit ioctl then cleans up the armed job: drivers/accel/rocket/rocket_job.c:rocket_ioctl_submit_job() { ... ret = rocket_job_push(rjob); if (ret) goto out_cleanup_job; out_cleanup_job: if (ret) drm_sched_job_cleanup(&rjob->base); ... } Is it safe to call drm_sched_job_cleanup() on a job that has already been armed? The DRM scheduler API requires that drm_sched_job_arm() is the point of no return and must be followed by drm_sched_entity_push_job(). Aborting here could leave armed fences in an inconsistent state. [Severity: High] This is a pre-existing issue, but is there a leaked IOMMU group reference here? drivers/accel/rocket/rocket_job.c:rocket_job_handle_irq() { ... iommu_detach_group(NULL, iommu_group_get(core->dev)); dma_fence_signal(core->in_flight_job->done_fence); ... } Every time a hardware job completes, iommu_group_get() is called to acquire a reference, which is then passed to iommu_detach_group(). Since iommu_detach_group() does not consume the reference and iommu_group_put() is never called, doesn't this leak one reference per job completion? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
