Re: [PATCH v4] drm/imagination: Propagate all errors from KCCB command submission code

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] State Corruption and Use-After-Free due to missing CCCB rollback on KCCB kick failure.
- [Medium] KCCB slot reservation leak on error paths in pvr_kccb_send_cmd_powered().

Pre-existing issues:
- [High] Paired fragment job hangs indefinitely if its paired geometry job fails early.
--

commit 169ea0ff06e137cf7e22eac4d32e0b8218dddecb
Author: Alessio Belle <alessio.belle@imgtec.com>

drm/imagination: Propagate all errors from KCCB command submission code

This patch updates pvr_kccb_send_cmd_reserved_powered() to return an int
instead of void, allowing it to propagate errors. The error is now correctly
propagated to the DRM scheduler's run_job callback in pvr_queue_run_job().

Link: https://patch.msgid.link/20260907-b4-upstream-propagate-all-errors-from-kccb-cmd-submission-code-v3-1-4578a0fadf69@imgtec.com

> diff --git a/drivers/gpu/drm/imagination/pvr_ccb.c b/drivers/gpu/drm/imagination/pvr_ccb.c
> index b702d122d7918..dfd0682705425 100644
> --- a/drivers/gpu/drm/imagination/pvr_ccb.c
> +++ b/drivers/gpu/drm/imagination/pvr_ccb.c
[ ... ]
> @@ -268,19 +273,25 @@ pvr_kccb_send_cmd_reserved_powered(struct pvr_device *pvr_dev,
>  	struct rogue_fwif_ccb_ctl *ctrl = pvr_ccb->ctrl;
>  	u32 old_write_offset;
>  	u32 new_write_offset;
> +	int err;
>  
> -	WARN_ON(pvr_dev->lost);
> +	if (pvr_dev->lost)
> +		return -EIO;

[Severity: Medium]
If pvr_kccb_send_cmd_reserved_powered() is called by
pvr_kccb_send_cmd_powered() after a successful pvr_kccb_reserve_slot_sync(),
the slot reservation count has been incremented. Returning early with -EIO
here avoids consuming the slot, but does this skip releasing the reservation?
If pvr_kccb_release_slot() is never called for this error path, it might
slowly leak KCCB slot capacity until no commands can be submitted.

[ ... ]
> @@ -378,8 +396,7 @@ pvr_kccb_send_cmd_powered(struct pvr_device *pvr_dev, struct rogue_fwif_kccb_cmd
>  	if (err)
>  		return err;
>  
> -	pvr_kccb_send_cmd_reserved_powered(pvr_dev, cmd, kccb_slot);
> -	return 0;
> +	return pvr_kccb_send_cmd_reserved_powered(pvr_dev, cmd, kccb_slot);
>  }

[Severity: Medium]
Similarly, when pvr_kccb_send_cmd_reserved_powered() fails and returns an
error, the caller pvr_kccb_send_cmd_powered() just propagates this error
without calling pvr_kccb_release_slot(). Could this permanently leak the
slot reservation acquired by pvr_kccb_reserve_slot_sync() earlier in the
same function?

[ ... ]
> diff --git a/drivers/gpu/drm/imagination/pvr_queue.c b/drivers/gpu/drm/imagination/pvr_queue.c
> index 09993e858df8b..ffac59ee8ae52 100644
> --- a/drivers/gpu/drm/imagination/pvr_queue.c
> +++ b/drivers/gpu/drm/imagination/pvr_queue.c
> @@ -769,17 +769,19 @@ static struct dma_fence *pvr_queue_run_job(struct drm_sched_job *sched_job)
>  		    (job->type != DRM_PVR_JOB_TYPE_GEOMETRY ||
>  		     job->paired_job->type != DRM_PVR_JOB_TYPE_FRAGMENT ||
>  		     job->hwrt != job->paired_job->hwrt ||
> -		     job->ctx != job->paired_job->ctx)))
> -		return ERR_PTR(-EINVAL);
> +		     job->ctx != job->paired_job->ctx))) {
> +		err = -EINVAL;
> +		goto err_release;
> +	}
>  
>  	err = pvr_job_get_pm_ref(job);
>  	if (WARN_ON(err))
> -		return ERR_PTR(err);
> +		goto err_release;
>  
>  	if (job->paired_job) {
>  		err = pvr_job_get_pm_ref(job->paired_job);
>  		if (WARN_ON(err))
> -			return ERR_PTR(err);
> +			goto err_release;
>  	}

[Severity: High]
This is a pre-existing issue, but it is further exposed by this new early
failure check. If a paired geometry job fails early here and returns an error,
the geometry job aborts. However, the paired fragment job's execution of
pvr_queue_run_job() unconditionally returns its done_fence:

    if (job->paired_job && job->type == DRM_PVR_JOB_TYPE_FRAGMENT &&
        job->done_fence->ops) {
            return dma_fence_get(job->done_fence);
    }

Since the fragment job relies entirely on the geometry job to submit both
jobs to the hardware, does this early failure leave the fragment job waiting
forever for a fence that will never signal, permanently hanging the fragment
queue scheduler?

>  
>  	/* Submit our job to the CCCB */
> @@ -793,25 +795,33 @@ static struct dma_fence *pvr_queue_run_job(struct drm_sched_job *sched_job)
>  
>  		/* Submit the fragment job along the geometry job and send a combined kick. */
>  		pvr_queue_submit_job_to_cccb(frag_job);
> -		pvr_cccb_send_kccb_combined_kick(pvr_dev,
> -						 &geom_queue->cccb, &frag_queue->cccb,
> -						 pvr_context_get_fw_addr(geom_job->ctx) +
> -						 geom_queue->ctx_offset,
> -						 pvr_context_get_fw_addr(frag_job->ctx) +
> -						 frag_queue->ctx_offset,
> -						 job->hwrt,
> -						 frag_job->fw_ccb_cmd_type ==
> -						 ROGUE_FWIF_CCB_CMD_TYPE_FRAG_PR);
> +		err = pvr_cccb_send_kccb_combined_kick(pvr_dev,
> +						       &geom_queue->cccb, &frag_queue->cccb,
> +						       pvr_context_get_fw_addr(geom_job->ctx) +
> +						       geom_queue->ctx_offset,
> +						       pvr_context_get_fw_addr(frag_job->ctx) +
> +						       frag_queue->ctx_offset,
> +						       job->hwrt,
> +						       frag_job->fw_ccb_cmd_type ==
> +						       ROGUE_FWIF_CCB_CMD_TYPE_FRAG_PR);
>  	} else {
>  		struct pvr_queue *queue = container_of(job->base.sched,
>  						       struct pvr_queue, scheduler);
>  
> -		pvr_cccb_send_kccb_kick(pvr_dev, &queue->cccb,
> -					pvr_context_get_fw_addr(job->ctx) + queue->ctx_offset,
> -					job->hwrt);
> +		err = pvr_cccb_send_kccb_kick(pvr_dev, &queue->cccb,
> +					      pvr_context_get_fw_addr(job->ctx) +
> +					      queue->ctx_offset,
> +					      job->hwrt);
>  	}
>  
> +	if (WARN_ON(err))
> +		goto err_release;

[Severity: High]
When pvr_queue_submit_job_to_cccb() executes, it copies the job commands into
the shared Client Circular Command Buffer (CCCB) and advances the local
write_offset.

If the subsequent firmware kick fails and returns an error, the new error
handling path jumps to err_release and returns ERR_PTR(err). The DRM
scheduler then frees the job and its associated memory.

However, the driver does not roll back the CCCB write_offset. When a future
job successfully kicks the KCCB, will the firmware process the aborted job's
commands from the CCCB and attempt to access memory that the host has already
freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-b4-upstream-propagate-all-errors-from-kccb-cmd-submission-code-v4-1-b684f58a06ce@imgtec.com?part=1