Re: [PATCH v4 11/31] accel: ethosu: Clear SRAM before submission

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] accel: ethosu: ethosu_job_sram_size() clobbers region 2 GEM BO with SRAM addr
- [High] accel: ethosu: job->base.entity is always NULL in ethosu_job_set_in_flight()
- [Medium] accel: ethosu: ethosu_job_close() clears sram_entity before killing entity
--

commit a7a26abf2b6252cbc62a4f8adc9daddbc6d61be4
Author: Rob Herring (Arm) <robh@kernel.org>

accel: ethosu: Clear SRAM before submission

This patch tracks the scheduler entity that last used the on-chip SRAM and
clears the SRAM before submitting a job from a different entity.

> diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
> index ef95a8abd61e2..3976115902e83 100644
> --- a/drivers/accel/ethosu/ethosu_job.c
> +++ b/drivers/accel/ethosu/ethosu_job.c
> @@ -45,11 +45,27 @@ static const struct dma_fence_ops ethosu_fence_ops = {
>  	.get_timeline_name = ethosu_fence_get_timeline_name,
>  };
>  
> -static void ethosu_job_set_in_flight(struct ethosu_device *dev,
> -				     struct ethosu_job *job)
> +static u32 ethosu_job_sram_size(struct ethosu_job *job)
>  {
> +	struct ethosu_validated_cmdstream_info *cmd_info =
> +		to_ethosu_bo(job->cmd_bo)->info;
> +
> +	return cmd_info->region_size[ETHOSU_SRAM_REGION];
> +}
> +
> +static bool ethosu_job_set_in_flight(struct ethosu_device *dev,
> +				     struct ethosu_job *job, u32 sram_size)
> +{
> +	bool clear_sram = false;
> +
>  	guard(spinlock)(&dev->job_lock);
> +	if (sram_size && dev->sram_entity != job->base.entity) {
> +		dev->sram_entity = job->base.entity;
> +		clear_sram = true;
> +	}

[Severity: High]
Isn't job->base.entity always NULL when ethosu_job_set_in_flight() is called
from ethosu_job_run()?

In the DRM scheduler, drm_sched_run_job_work() pops the job via
drm_sched_entity_pop_job() before invoking sched->ops->run_job(sched_job):

drivers/gpu/drm/scheduler/sched_entity.c:drm_sched_entity_pop_job() {
	...
	/* Jobs and entities might have different lifecycles. Since we're
	 * removing the job from the entities queue, set the jobs entity pointer
	 * to NULL to prevent any future access of the entity through this job.
	 */
	sched_job->entity = NULL;

	return sched_job;
}

Because job->base.entity is set to NULL before ethosu_job_run() runs, and
dev->sram_entity starts at NULL, won't dev->sram_entity != job->base.entity
always evaluate to false (NULL != NULL), leaving clear_sram false so that
memset_io() in ethosu_job_hw_submit() never executes?

Would tracking job->base.s_fence->drm_client_id (or
job->base.s_fence->scheduled.context) instead of job->base.entity avoid this?

>  	dev->in_flight_job = job;
> +
> +	return clear_sram;
>  }
>  
>  static struct ethosu_job *
> @@ -64,7 +80,8 @@ ethosu_job_clear_in_flight(struct ethosu_device *dev)
>  	return job;
>  }
>  
> -static void ethosu_job_hw_submit(struct ethosu_device *dev, struct ethosu_job *job)
> +static void ethosu_job_hw_submit(struct ethosu_device *dev, struct ethosu_job *job,
> +				 u32 sram_size, bool clear_sram)
>  {
>  	struct drm_gem_dma_object *cmd_bo = to_drm_gem_dma_obj(job->cmd_bo);
>  	struct ethosu_validated_cmdstream_info *cmd_info = to_ethosu_bo(job->cmd_bo)->info;
> @@ -79,7 +96,12 @@ static void ethosu_job_hw_submit(struct ethosu_device *dev, struct ethosu_job *j
>  		dev_dbg(dev->base.dev, "Region %d base addr = %pad\n", region, &bo->dma_addr);
>  	}
>  
> -	if (job->sram_size) {
> +	if (sram_size) {
> +		if (clear_sram) {
> +			memset_io(dev->sram, 0, dev->npu_info.sram_size);
> +			/* Order the SRAM clear before starting the NPU. */
> +			wmb();
> +		}
>  		writel_relaxed(lower_32_bits(dev->sramphys),
>  			       dev->regs + NPU_REG_BASEP(ETHOSU_SRAM_REGION));
>  		writel_relaxed(upper_32_bits(dev->sramphys),

[Severity: Critical]
When a job backs region 2 (ETHOSU_SRAM_REGION) with a GEM BO and sets
job->sram_size to 0, won't replacing the job->sram_size check with
ethosu_job_sram_size() overwrite the region 2 GEM BO DMA address with
dev->sramphys?

In ethosu_ioctl_submit_job(), region 2 is only reserved for on-chip SRAM when
job->sram_size is non-zero:

	/* BO region 2 is reserved if SRAM is used */
	if (job->region_bo_handles[ETHOSU_SRAM_REGION] && job->sram_size)
		return -EINVAL;

	if (edev->npu_info.sram_size < job->sram_size)
		return -EINVAL;

When job->sram_size is 0 and job->region_bo_handles[ETHOSU_SRAM_REGION] is
non-zero, ethosu_ioctl_submit_job() only validates
cmd_info->region_size[ETHOSU_SRAM_REGION] against gem->size, not against
edev->npu_info.sram_size.

Since ethosu_job_sram_size() unconditionally returns
cmd_info->region_size[ETHOSU_SRAM_REGION] (truncated to u32) without checking
job->sram_size, ethosu_job_hw_submit() will clobber
NPU_REG_BASEP(ETHOSU_SRAM_REGION) and NPU_REG_BASEP_HI(ETHOSU_SRAM_REGION)
with dev->sramphys, causing the NPU to perform unchecked DMA beyond the
bounds of on-chip SRAM (or at physical address 0 when no SRAM is configured).

Should this continue checking job->sram_size instead of
cmd_info->region_size[ETHOSU_SRAM_REGION]?

[ ... ]

> @@ -482,12 +507,22 @@ int ethosu_job_open(struct ethosu_file_priv *ethosu_priv)
>  	return ret;
>  }
>  
> +static void ethosu_job_clear_sram_entity(struct ethosu_device *dev,
> +					 struct drm_sched_entity *entity)
> +{
> +	guard(spinlock)(&dev->job_lock);
> +	if (dev->sram_entity == entity) {
> +		dev->sram_entity = NULL;
> +	}
> +}
> +
>  void ethosu_job_close(struct ethosu_file_priv *ethosu_priv)
>  {
>  	struct ethosu_device *dev = ethosu_priv->edev;
>  	struct drm_sched_entity *entity = &ethosu_priv->sched_entity;
>  
>  	scoped_guard(mutex, &dev->sched_lock) {
> +		ethosu_job_clear_sram_entity(dev, entity);
>  		drm_sched_entity_kill(entity);
>  		list_del(&ethosu_priv->sched_entity_list);
>  	}

[Severity: Medium]
Can ethosu_job_clear_sram_entity() race with ethosu_job_run() if it is called
before drm_sched_entity_kill()?

drm_sched_run_job_work() runs on the scheduler workqueue without holding
dev->sched_lock, invokes ethosu_job_run() -> ethosu_job_set_in_flight(), and
only signals entity->entity_idle after run_job() returns.

If ethosu_job_clear_sram_entity() runs before drm_sched_entity_kill() stops
the entity and waits on entity->entity_idle, a concurrent ethosu_job_run() can
repopulate dev->sram_entity right before ethosu_priv is freed in
ethosu_postclose(). If a subsequent open() reallocates ethosu_file_priv at the
same address, could the stale dev->sram_entity pointer match the new entity
and skip clearing SRAM?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-ethosu-fixes-v4-0-4aa25e22578e@kernel.org?part=11