Re: [PATCH 1/3] drm/rocket: Remove unused reset worker

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] `rocket_iommu_domain_put()` does not check for NULL before dereferencing, leading to a crash on the error path.
- [Medium] The ioctl handler silently ignores errors when submitting multiple jobs, reporting success to userspace even if submissions fail.
--

commit cc3020dc8d8efb7ac65346d6d8612ab4fbbadb48
Author: Maíra Canal <mcanal@igalia.com>

drm/rocket: Remove unused reset worker

The reset.work work_struct and its rocket_reset_work() handler were
initialized and cancelled, but never actually queued. This patch drops
the dead worker, its initialization, cancellation, and the unused struct
member.

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index 2f1861f960ccb..8a26139a44f46 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but can this lead to a NULL pointer dereference
on the error path?

In rocket_ioctl_submit_job(), if rocket_copy_tasks() fails, it jumps to
out_cleanup_job, which then falls through to out_put_job:

drivers/accel/rocket/rocket_job.c:rocket_ioctl_submit_job() {
	...
	ret = rocket_copy_tasks(dev, file, job, rjob);
	if (ret)
		goto out_cleanup_job;
	...
out_cleanup_job:
	if (ret)
		drm_sched_job_cleanup(&rjob->base);
out_put_job:
	rocket_job_put(rjob);

	return ret;
}

This calls rocket_job_cleanup(), which unconditionally passes job->domain to
rocket_iommu_domain_put():

drivers/accel/rocket/rocket_job.c:rocket_job_cleanup() {
	...
	rocket_iommu_domain_put(job->domain);
	...
}

But rocket_iommu_domain_put() does not check if domain is NULL before passing
it to kref_put():

drivers/accel/rocket/rocket_drv.c:rocket_iommu_domain_put() {
	kref_put(&domain->kref, rocket_iommu_domain_destroy);
}

Since job->domain is assigned after rocket_copy_tasks() succeeds, could this
dereference a NULL pointer if an early failure occurs?


[Severity: Medium]
This is a pre-existing issue, but does this code silently ignore job
submission errors?

In rocket_ioctl_submit(), the loop calls rocket_ioctl_submit_job(), but the
return value is not captured or checked:

drivers/accel/rocket/rocket_job.c:rocket_ioctl_submit() {
	...
	for (i = 0; i < args->job_count; i++)
		rocket_ioctl_submit_job(dev, file, &jobs[i]);

exit:
	kvfree(jobs);

	return ret;
}

Since ret remains 0, the ioctl will return success to userspace even if
some or all jobs fail to submit. Will userspace assume the jobs were
successfully queued in this case?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605-rocket-per-task-jobs-v1-0-e3319adf3c49@igalia.com?part=1