Re: [PATCH 06/10] drm/v3d: Reject invalid syncobj handles in submit ioctls

["Maíra Canal" <mcanal@igalia.com>]

Hi Tvrtko,

On 16/04/26 08:59, Tvrtko Ursulin wrote:
> 
> On 13/04/2026 16:03, Maíra Canal wrote:
>> drm_sched_job_add_syncobj_dependency() returns -ENOENT both when the
>> handle is zero and when the handle is non-zero but does not find a
>> corresponding existing syncobj (userspace bug). The driver previously
>> ignored -ENOENT in both cases, silently accepting broken handles.
>>
>> Distinguish the two: skip the call entirely when the handle is zero, as
>> there is no dependency, and let -ENOENT propagate for non-zero handles
>> that don't resolve, turning the error into a proper return to userspace.
>>
>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>> ---
>>   drivers/gpu/drm/v3d/v3d_submit.c | 23 ++++++++++++-----------
>>   1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/ 
>> v3d_submit.c
>> index 054367ba533d..fe8b5757c3e8 100644
>> --- a/drivers/gpu/drm/v3d/v3d_submit.c
>> +++ b/drivers/gpu/drm/v3d/v3d_submit.c
>> @@ -171,12 +171,11 @@ v3d_job_add_syncobjs(struct v3d_job *job, struct 
>> drm_file *file_priv,
>>       int ret = 0;
>>       if (!has_multisync) {
>> -        ret = drm_sched_job_add_syncobj_dependency(&job->base, 
>> file_priv,
>> -                               in_sync, 0);
>> -        // TODO: Investigate why this was filtered out for the IOCTL.
>> -        if (ret && ret != -ENOENT)
>> -            return ret;
>> -        return 0;
>> +        /* Ignore syncobj if its handle is NULL */
> 
> Nitpick s/NULL/zero/ since it is not a pointer.
> 
>> +        if (in_sync)
>> +            ret = drm_sched_job_add_syncobj_dependency(&job->base, 
>> file_priv,
>> +                                   in_sync, 0);
> 
> Ah now ret needs to be initialized to zero (comment from previous patch)!
> 
>> +        return ret;
>>       }
>>       if (se->in_sync_count && se->wait_stage == queue) {
>> @@ -190,11 +189,13 @@ v3d_job_add_syncobjs(struct v3d_job *job, struct 
>> drm_file *file_priv,
>>                   return -EFAULT;
>>               }
>> -            ret = drm_sched_job_add_syncobj_dependency(&job->base,
>> -                                   file_priv, in.handle, 0);
>> -            // TODO: Investigate why this was filtered out for the 
>> IOCTL.
>> -            if (ret && ret != -ENOENT)
>> -                return ret;
>> +            /* Ignore syncobj if its handle is NULL */
> 
> As above. Or perhaps even NULL makes sense as in null handle being an 
> invalid handle reads more obvious than a zero handle?
> 
>> +            if (in.handle) {
>> +                ret = drm_sched_job_add_syncobj_dependency(&job->base,
>> +                                       file_priv, in.handle, 0);
>> +                if (ret)
>> +                    return ret;
>> +            }
>>           }
>>       }
>>
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> 
> With a slight danger there is some userspace out there which relies on 
> this? Have you look at how Mesa uses it, and the history?

I've checked userspace usage and thankfully, we never relied on this.
IGT tests are also passing (which were the reason I added this -ENOENT
some years ago, as without them, the tests would fail).

Thanks for your review!

Best regards,
- Maíra

> 
> Regards,
> 
> Tvrtko
>