Re: [PATCH 1/6] drm/tegra: Fix syncobj_in wait return value check

On Thursday, July 30, 2026 1:58 PM [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [Critical] If `host1x_fence_create()` fails, it returns an error pointer which is passed to `drm_syncobj_replace_fence()` due to a missing early return, causing a kernel crash when dereferenced.
> - [High] The initial reference to the `dma_fence` allocated by `host1x_fence_create()` is leaked on the success path.
> --

Subsequent patches in the thread fix both issues.

Cheers
Mikko

> 
> --- Patch [1]: [PATCH 1/6] drm/tegra: Fix syncobj_in wait return value check ---
> commit 250614410b887862e5e7b49b5d80c8b53c8aa353
> Author: Mikko Perttunen <[email protected]>
> 
> drm/tegra: Fix syncobj_in wait return value check
> 
> dma_fence_wait_timeout() returns the remaining timeout in jiffies on
> success, zero on timeout, and a negative error code if interrupted. The
> return value was assigned to 'err' and checked as if it were an errno,
> the opposite of the correct logic.
> 
> Fix the code to use a properly typed variable and check each condition
> correctly.
> 
> Closes: https://lore.kernel.org/all/[email protected]/
> 
> > diff --git a/drivers/gpu/drm/tegra/submit.c b/drivers/gpu/drm/tegra/submit.c
> > index e5841857c9378..5dad6dc4eb8c3 100644
> > --- a/drivers/gpu/drm/tegra/submit.c
> > +++ b/drivers/gpu/drm/tegra/submit.c
> 
> [ ... ]
> 
> [Severity: Critical]
> This is a pre-existing issue, but I noticed a potential problem in
> tegra_drm_ioctl_channel_submit() when handling syncobj_out:
> 
> 	if (syncobj) {
> 		struct dma_fence *fence = host1x_fence_create(job->syncpt, job->syncpt_end, true);
> 		if (IS_ERR(fence)) {
> 			err = PTR_ERR(fence);
> 			SUBMIT_ERR(context, "failed to create postfence: %d", err);
> 		}
> 
> 		drm_syncobj_replace_fence(syncobj, fence);
> 	}
> 
> If host1x_fence_create() fails, it returns an error pointer. Since there is
> no early return or goto in the error handling block, the error pointer falls
> through and is passed to drm_syncobj_replace_fence().
> 
> Looking at drm_syncobj_replace_fence() in drm_syncobj.c, it calls
> dma_fence_get(fence) which checks 'if (fence)'. This would evaluate to true
> for an error pointer, causing an attempt to increment the reference count on
> the invalid pointer.
> 
> Should there be a 'goto put_job' or similar return path inside the
> IS_ERR(fence) block to avoid this panic?
> 
> [Severity: High]
> This is also a pre-existing issue, but in the same block of code, does
> this leak the fence on the success path? 
> 
> host1x_fence_create() returns a new dma_fence with a reference count of 1.
> drm_syncobj_replace_fence() then takes its own reference to the fence by
> calling dma_fence_get().
> 
> However, I don't see a matching dma_fence_put(fence) in this function
> after drm_syncobj_replace_fence(). Will the initial reference to the fence
> be leaked for every successful ioctl call using syncobj_out?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1