Re: [PATCH 01/11] accel: ethosu: Fix ethosu_job_open() return value

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The dynamically allocated `done_fence` is leaked on error paths during job submission in `ethosu_ioctl_submit_job`.
- [High] NULL pointer dereference in `ethosu_ioctl_submit_job` error path due to premature cleanup of an uninitialized scheduler job.
--

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

    accel: ethosu: Fix ethosu_job_open() return value
    
    A WARN_ON() returns a 0 or 1, not the original negative errno. Just drop
    the WARN_ON() as the FD open will pass the return code to userspace and
    there's only one possible source of the error (drm_sched_entity_init()).

> diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
> index 6a038c0384ccd..99c8812691e55 100644
> --- a/drivers/accel/ethosu/ethosu_job.c
> +++ b/drivers/accel/ethosu/ethosu_job.c

[ ... ]

While reviewing this file, I noticed a couple of pre-existing issues in the
job submission error paths.

If the ejob->done_fence allocation fails, it bypasses job initialization but
still attempts cleanup:

drivers/accel/ethosu/ethosu_job.c:ethosu_ioctl_submit_job() {
    ...
	ejob->done_fence = kzalloc_obj(*ejob->done_fence);
	if (!ejob->done_fence) {
		ret = -ENOMEM;
		goto out_cleanup_job;
	}

	ret = drm_sched_job_init(&ejob->base,
				 &file_priv->sched_entity,
				 1, NULL, file->client_id);
    ...
out_cleanup_job:
	if (ret)
		drm_sched_job_cleanup(&ejob->base);
out_put_job:
	ethosu_job_err_cleanup(ejob);

	return ret;
}

[Severity: High]
This isn't a bug introduced by this patch, but does calling
drm_sched_job_cleanup(&ejob->base) cause a NULL pointer dereference here?

Since drm_sched_job_init() hasn't been called yet, ejob->base.s_fence is
completely zero-initialized. drm_sched_job_cleanup() unconditionally
dereferences job->s_fence when checking the reference count, which would
trigger a panic on this memory allocation failure path.

Additionally, looking at the cleanup function called further down that same
error path:

drivers/accel/ethosu/ethosu_job.c:ethosu_job_err_cleanup() {
	unsigned int i;

	ethosu_perfmon_put(job->perfmon);

	for (i = 0; i < job->region_cnt; i++)
		drm_gem_object_put(job->region_bo[i]);

	drm_gem_object_put(job->cmd_bo);

	kfree(job);
}

[Severity: High]
This is a pre-existing issue, but will this leak ejob->done_fence?

If ethosu_ioctl_submit_job() fails later in the function (for example, if
drm_gem_object_lookup() returns an error), it jumps to out_cleanup_job and
eventually calls ethosu_job_err_cleanup(). The cleanup function frees the
job container and BOs, but appears to miss freeing the dynamically allocated
ejob->done_fence pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827-ethosu-fixes-v1-0-346f9ea8791c@kernel.org?part=1