On 03/06/2026 23:25, Maíra Canal wrote:
v3d_submit_process_post_deps() looks up the out_sync syncobj via
drm_syncobj_find(), and if userspace passes a non-zero handle that doesn't
refer to a valid syncobj, the lookup silently returns NULL and the
post-deps step skips publishing the submission's last fence to it. The
ioctl still returns success, leaving userspace to wait on a invalid
syncobj.
Instead of silently ignoring an invalid non-zero out_sync, move the syncobj
lookup to the submission and make it fail with -ENOENT up front, mirroring
the syncobj validation already done for in_sync. Now,
v3d_submit_process_post_deps() only does the fence replacement.
Note that the lookup is skipped when the multi-sync extension is in use,
since args->out_sync is unused in that case.
To keep cleanup symmetric on error paths, convert the function
v3d_put_multisync_post_deps() into a single function that releases the
references that were acquired but never published for both single-sync
and multi-sync.
Suggested-by: Tvrtko Ursulin <[email protected]>
Signed-off-by: Maíra Canal <[email protected]>
---
drivers/gpu/drm/v3d/v3d_submit.c | 50 +++++++++++++++++++++++++++++-----------
1 file changed, 36 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index 2beb99a25104..dc27770d85fd 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -338,17 +338,15 @@ v3d_submit_attach_object_fences(struct v3d_submit *submit)
}
static void
-v3d_submit_process_post_deps(struct v3d_submit *submit, u32 out_sync,
+v3d_submit_process_post_deps(struct v3d_submit *submit, struct drm_syncobj
*sync_out,
struct v3d_submit_ext *se)
{
bool has_multisync = se && (se->flags & DRM_V3D_EXT_ID_MULTI_SYNC);
struct v3d_job *last_job = submit->jobs[submit->job_count - 1];
- struct drm_syncobj *sync_out;
/* Update the return sync object for the job */
/* If it only supports a single signal semaphore*/
if (!has_multisync) {
- sync_out = drm_syncobj_find(submit->file_priv, out_sync);
if (sync_out) {
drm_syncobj_replace_fence(sync_out,
last_job->done_fence);
drm_syncobj_put(sync_out);
Is it worth adding an assert that sync_obj and has_multisync are
mutually exclusive? It caught me up thinking what prevents a ref leak.
@@ -381,7 +379,7 @@ v3d_push_job(struct v3d_job *job)
}
static int
-v3d_submit_jobs(struct v3d_submit *submit, u32 out_sync,
+v3d_submit_jobs(struct v3d_submit *submit, struct drm_syncobj *sync_out,
struct v3d_submit_ext *se)
{
struct v3d_dev *v3d = submit->v3d;
@@ -406,7 +404,7 @@ v3d_submit_jobs(struct v3d_submit *submit, u32 out_sync,
v3d_submit_attach_object_fences(submit);
v3d_submit_unlock_reservations(submit);
- v3d_submit_process_post_deps(submit, out_sync, se);
+ v3d_submit_process_post_deps(submit, sync_out, se);
v3d_submit_put_jobs(submit);
@@ -444,10 +442,13 @@ v3d_setup_csd_jobs_and_bos(struct v3d_submit *submit,
}
static void
-v3d_put_multisync_post_deps(struct v3d_submit_ext *se)
+v3d_submit_put_post_deps(struct drm_syncobj *sync_out, struct v3d_submit_ext
*se)
{
unsigned int i;
+ if (sync_out)
+ drm_syncobj_put(sync_out);
+
if (!(se && se->out_sync_count))
return;
@@ -1006,6 +1007,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
{
struct v3d_submit submit = { .v3d = to_v3d_dev(dev), .file_priv =
file_priv };
struct drm_v3d_submit_cl *args = data;
+ struct drm_syncobj *sync_out = NULL;
struct v3d_submit_ext se = {0};
struct v3d_bin_job *bin = NULL;
struct v3d_render_job *render = NULL;
@@ -1032,6 +1034,12 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
}
}
+ if (args->out_sync && !(se.flags & DRM_V3D_EXT_ID_MULTI_SYNC)) {
Is it feasible to make sure out_sync is zero if multi-sync flag is set?
Or we have to allow letting garbage in for backwards compatibility?
Either way:
Reviewed-by: Tvrtko Ursulin <[email protected]>
Regards,
Tvrtko
+ sync_out = drm_syncobj_find(file_priv, args->out_sync);
+ if (!sync_out)
+ return -ENOENT;
+ }
+
if (args->bcl_start != args->bcl_end) {
bin = (struct v3d_bin_job *) v3d_submit_add_job(&submit,
V3D_BIN);
if (IS_ERR(bin)) {
@@ -1088,7 +1096,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
if (ret)
goto fail;
- ret = v3d_submit_jobs(&submit, args->out_sync, &se);
+ ret = v3d_submit_jobs(&submit, sync_out, &se);
if (ret)
goto fail_unreserve;
@@ -1098,7 +1106,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
v3d_submit_unlock_reservations(&submit);
fail:
v3d_submit_cleanup_jobs(&submit);
- v3d_put_multisync_post_deps(&se);
+ v3d_submit_put_post_deps(sync_out, &se);
return ret;
}
@@ -1118,6 +1126,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
{
struct v3d_submit submit = { .v3d = to_v3d_dev(dev), .file_priv =
file_priv };
struct drm_v3d_submit_tfu *args = data;
+ struct drm_syncobj *sync_out = NULL;
struct v3d_submit_ext se = {0};
struct v3d_tfu_job *job = NULL;
int ret = 0;
@@ -1137,6 +1146,12 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
}
}
+ if (args->out_sync && !(se.flags & DRM_V3D_EXT_ID_MULTI_SYNC)) {
+ sync_out = drm_syncobj_find(file_priv, args->out_sync);
+ if (!sync_out)
+ return -ENOENT;
+ }
+
job = (struct v3d_tfu_job *) v3d_submit_add_job(&submit, V3D_TFU);
if (IS_ERR(job)) {
ret = PTR_ERR(job);
@@ -1178,7 +1193,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
if (ret)
goto fail;
- ret = v3d_submit_jobs(&submit, args->out_sync, &se);
+ ret = v3d_submit_jobs(&submit, sync_out, &se);
if (ret)
goto fail_unreserve;
@@ -1188,7 +1203,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
v3d_submit_unlock_reservations(&submit);
fail:
v3d_submit_cleanup_jobs(&submit);
- v3d_put_multisync_post_deps(&se);
+ v3d_submit_put_post_deps(sync_out, &se);
return ret;
}
@@ -1208,6 +1223,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
{
struct v3d_submit submit = { .v3d = to_v3d_dev(dev), .file_priv =
file_priv };
struct drm_v3d_submit_csd *args = data;
+ struct drm_syncobj *sync_out = NULL;
struct v3d_submit_ext se = {0};
int ret;
@@ -1234,6 +1250,12 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
}
}
+ if (args->out_sync && !(se.flags & DRM_V3D_EXT_ID_MULTI_SYNC)) {
+ sync_out = drm_syncobj_find(file_priv, args->out_sync);
+ if (!sync_out)
+ return -ENOENT;
+ }
+
ret = v3d_setup_csd_jobs_and_bos(&submit, args, &se);
if (ret)
goto fail;
@@ -1246,7 +1268,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
if (ret)
goto fail;
- ret = v3d_submit_jobs(&submit, args->out_sync, &se);
+ ret = v3d_submit_jobs(&submit, sync_out, &se);
if (ret)
goto fail_unreserve;
@@ -1256,7 +1278,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
v3d_submit_unlock_reservations(&submit);
fail:
v3d_submit_cleanup_jobs(&submit);
- v3d_put_multisync_post_deps(&se);
+ v3d_submit_put_post_deps(sync_out, &se);
return ret;
}
@@ -1354,7 +1376,7 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
if (ret)
goto fail;
- ret = v3d_submit_jobs(&submit, 0, &se);
+ ret = v3d_submit_jobs(&submit, NULL, &se);
if (ret)
goto fail_unreserve;
@@ -1364,7 +1386,7 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
v3d_submit_unlock_reservations(&submit);
fail:
v3d_submit_cleanup_jobs(&submit);
- v3d_put_multisync_post_deps(&se);
+ v3d_submit_put_post_deps(NULL, &se);
return ret;
}
