Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
New issues: - [Critical] Splitting an inference into independent `drm_sched_job` tasks breaks IOMMU domain isolation due to DRM scheduler interleaving. - [High] Implicit BO dependencies are incorrectly anchored to the last task of an inference instead of the first, causing premature hardware execution. - [High] IOMMU detachment in `rocket_job_cleanup()` races with the execution of the next inference because the ioctl thread holds the final kref. - [High] If a task fails to start in `rocket_job_run()`, the rest of the inference is not cancelled, leading to partial out-of-sequence execution. Pre-existing issues: - [Critical] Unconditional call to `rocket_iommu_domain_put()` in `rocket_job_cleanup()` causes a NULL pointer dereference if job initialization fails early. - [High] The IRQ handler accesses NPU registers before claiming the in-flight task, risking a hardware fault if the device has been reset concurrently. - [High] The driver silently ignores failures of individual job submissions in `rocket_ioctl_submit`. -- commit ea82a8e8306fe83c325efae485834200ae29aec9 Author: Maíra Canal <[email protected]> Subject: drm/rocket: Submit one drm_sched_job per task This commit redesigns the job submission path so each task is its own drm_sched_job. It ensures all submissions flow through the DRM scheduler so the reset path can serialize against hardware execution, while holding an inference refcount across tasks. > diff --git a/drivers/accel/rocket/rocket_job.c > b/drivers/accel/rocket/rocket_job.c > --- a/drivers/accel/rocket/rocket_job.c > +++ b/drivers/accel/rocket/rocket_job.c [ ... ] > @@ -186,43 +170,68 @@ static int rocket_job_push(struct rocket_job *job) > struct drm_gem_object **bos; > struct ww_acquire_ctx acquire_ctx; > - int ret = 0; > + int ret, i; > > - bos = kvmalloc_array(job->in_bo_count + job->out_bo_count, sizeof(void > *), > - GFP_KERNEL); > + bos = kvmalloc_array(bo_count, sizeof(void *), GFP_KERNEL); > + if (!bos) { > + ret = -ENOMEM; > + goto err_cleanup_tasks; > + } > memcpy(bos, job->in_bos, job->in_bo_count * sizeof(void *)); > memcpy(&bos[job->in_bo_count], job->out_bos, job->out_bo_count * > sizeof(void *)); > > - ret = drm_gem_lock_reservations(bos, job->in_bo_count + > job->out_bo_count, &acquire_ctx); > + ret = drm_gem_lock_reservations(bos, bo_count, &acquire_ctx); > + if (ret) > + goto err_free_bos; > + > + /* Anchor the BO synchronization on the last task: its finished fence is > + * the inference's completion fence. > + */ > + ret = rocket_acquire_object_fences(job->in_bos, job->in_bo_count, > + &last->base, false); [Severity: High] Does anchoring the input dependencies to the last task allow the first task to run before the input buffers are ready? Since the DRM scheduler uses these dependencies to know when a job is ready to run, adding them to last->base leaves job->tasks[0] without dependencies. The scheduler might submit the first task to the hardware immediately, causing a read-before-write data race. Should the input dependencies be anchored to the first task instead? [ ... ] > @@ -233,10 +242,22 @@ static void rocket_job_cleanup(struct kref *ref) > refcount); > unsigned int i; > > + /* > + * The last task holding a reference is gone, so the inference is over. > + * The ordered scheduler workqueue runs this (from free_job()) before > + * the next inference's run_job(), so the core's IOMMU group is free > + * in time for it. > + */ > + if (job->core) > + iommu_detach_group(NULL, job->core->iommu_group); [Severity: High] Can this detachment race with the execution of a subsequent inference? The ioctl thread initializes the inference refcount to 1 and drops it at the end of rocket_ioctl_submit_job(). If the scheduler finishes all tasks quickly, it drops its references, leaving the refcount at 1. The scheduler might then start the next inference, which attaches its own IOMMU domain. If the ioctl thread then wakes up and drops its final reference, rocket_job_cleanup() will call iommu_detach_group(). Could this unmap the IOMMU while the next inference is actively executing on the hardware? > + > rocket_iommu_domain_put(job->domain); [Severity: Critical] This is a pre-existing issue, but does this unconditionally dereference the domain pointer? If an error occurs early in rocket_ioctl_submit_job() (for example, if rocket_copy_tasks() fails), the error path calls rocket_job_put(rjob) and triggers cleanup before job->domain is initialized. Since rocket_iommu_domain_put() internally executes kref_put(&domain->kref, ...), passing a NULL pointer here will cause a kernel panic. Should this check if job->domain is valid before putting it? [ ... ] > @@ -286,108 +307,102 @@ static struct dma_fence *rocket_job_run(struct > drm_sched_job *sched_job) > struct rocket_core *core = sched_to_core(rdev, sched_job->sched); > struct dma_fence *fence = NULL; > int ret; [ ... ] > - ret = pm_runtime_get_sync(core->dev); > - if (ret < 0) > - return fence; > + ret = pm_runtime_resume_and_get(core->dev); > + if (ret < 0) { > + dma_fence_put(fence); > + return ERR_PTR(ret); > + } [Severity: High] If pm_runtime_resume_and_get() or iommu_attach_group() fails and returns an ERR_PTR, does the rest of the inference still execute? The scheduler will complete this failed task with an error, but it appears the job->cancelled flag is not set on these early failure paths. Since subsequent tasks in the same inference will not observe the cancelled flag, will they be submitted out of order, potentially leaving the hardware in an invalid state? > > - ret = iommu_attach_group(job->domain->domain, core->iommu_group); > - if (ret < 0) > - return fence; > + /* Attach the domain once for the whole inference. */ > + if (!job->core) { > + ret = iommu_attach_group(job->domain->domain, > core->iommu_group); [Severity: Critical] Does attaching the IOMMU domain only for the first task break isolation when the scheduler interleaves tasks? With the redesign mapping each task to a separate drm_sched_job, the DRM scheduler can interleave tasks from different inferences on the same core. If Process A's first task attaches its domain and then Process B's first task attaches its domain, Process A's second task will see job->core is already set. Will Process A's second task then execute using Process B's IOMMU domain, allowing cross-process memory access? [ ... ] > @@ -395,20 +409,20 @@ static void rocket_job_handle_irq(struct rocket_core > *core) > { > + struct rocket_task *task; > + > pm_runtime_mark_last_busy(core->dev); > > rocket_pc_writel(core, OPERATION_ENABLE, 0x0); > rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff); > > - scoped_guard(mutex, &core->job_lock) > - if (core->in_flight_job) { > - if (core->in_flight_job->next_task_idx < > core->in_flight_job->task_count) { > - rocket_job_hw_submit(core, core->in_flight_job); > - return; > - } > - > - iommu_detach_group(NULL, iommu_group_get(core->dev)); > - dma_fence_signal(core->in_flight_job->done_fence); > - pm_runtime_put_autosuspend(core->dev); > - core->in_flight_job = NULL; > - } > + /* > + * Claim the in-flight task: the reset path may run concurrently, so > + * whichever of us wins owns the PM put. > + */ > + task = xchg(&core->in_flight_task, NULL); [Severity: High] This is a pre-existing issue, but does writing to these registers before claiming the in-flight task risk a hardware fault? If a timeout occurs concurrently, rocket_reset() can claim the task, drop the PM reference, and reset the hardware. If this threaded IRQ handler then writes to the OPERATION_ENABLE and INTERRUPT_CLEAR registers while the device is powered down or mid-reset, could it trigger a bus error? Should the handler claim the task first to guarantee the device is awake? [ ... ] > int rocket_ioctl_submit(struct drm_device *dev, void *data, struct drm_file > *file) > { [ ... ] > for (i = 0; i < args->job_count; i++) > rocket_ioctl_submit_job(dev, file, &jobs[i]); > > exit: > kvfree(jobs); > > return ret; > } [Severity: High] This is a pre-existing issue, but is the return value of rocket_ioctl_submit_job() intentionally ignored here? If an individual job fails to submit due to an out-of-memory error or invalid BO handles, the failure is neither checked nor accumulated. Since user space will receive a success return code (ret = 0), does this hide submission failures? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
