Hey,
Den 2026-03-03 kl. 16:36, skrev Julian Orth:
> On Tue, Mar 3, 2026 at 4:29 PM Michel Dänzer <[email protected]>
> wrote:
>>
>> On 3/3/26 15:59, Christian König wrote:
>>> On 3/3/26 15:53, Maarten Lankhorst wrote:
>>>> Hey,
>>>>
>>>> Den 2026-03-01 kl. 13:34, skrev Julian Orth:
>>>>> Consider the following application:
>>>>>
>>>>> #include <fcntl.h>
>>>>> #include <string.h>
>>>>> #include <drm/drm.h>
>>>>> #include <sys/ioctl.h>
>>>>>
>>>>> int main(void) {
>>>>> int fd = open("/dev/dri/renderD128", O_RDWR);
>>>>> struct drm_syncobj_create arg1;
>>>>> ioctl(fd, DRM_IOCTL_SYNCOBJ_CREATE, &arg1);
>>>>> struct drm_syncobj_handle arg2;
>>>>> memset(&arg2, 1, sizeof(arg2)); // simulate dirty stack
>>>>> arg2.handle = arg1.handle;
>>>>> arg2.flags = 0;
>>>>> arg2.fd = 0;
>>>>> arg2.pad = 0;
>>>>> // arg2.point = 0; // userspace is required to set point to 0
>>>>> ioctl(fd, DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD, &arg2);
>>>>> }
>>>>>
>>>>> The last ioctl returns EINVAL because args->point is not 0. However,
>>>>> userspace developed against older kernel versions is not aware of the
>>>>> new point field and might therefore not initialize it.
>>>>>
>>>>> The correct check would be
>>>>>
>>>>> if (args->flags & DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_TIMELINE)
>>>>> return -EINVAL;
>>>>>
>>>>> However, there might already be userspace that relies on this not
>>>>> returning an error as long as point == 0. Therefore use the more lenient
>>>>> check.
>>>>>
>>>>> Fixes: c2d3a7300695 ("drm/syncobj: Extend EXPORT_SYNC_FILE for timeline
>>>>> syncobjs")
>>>>> Signed-off-by: Julian Orth <[email protected]>
>>>>
>>>> I'm not convinced this is the correct fix.
>>>> Userspace built before the change had the old size for drm_syncobj_create,
>>>> the size is encoded into the ioctl, and zero extended as needed.
>>>>
>>>> See drivers/gpu/drm/drm_ioctl.c:
>>>> out_size = in_size = _IOC_SIZE(cmd);
>>>> ...
>>>> if (ksize > in_size)
>>>> memset(kdata + in_size, 0, ksize - in_size);
>>>>
>>>> This is a bug in a newly built app, and should be handled by explicitly
>>>> zeroing
>>>> the entire struct or using named initializers, and only setting specific
>>>> members
>>>> as required.
>>>>
>>>> In particular, apps built before the change will never encounter this bug.
>>>
>>> Yeah, I've realized that after pushing the patch as well.
>>>
>>> But I still think this patch is the right thing to do, because without
>>> requesting the functionality by setting the flag the point should clearly
>>> not have any effect at all.
>>>
>>> And when an application would have only explicitly assigned the fields
>>> known previously and then later been compiled with the new points field it
>>> would have failed.
>>>
>>> It is good practice to memset() structures given to the kernel so that all
>>> bytes are zero initialized, but it is not documented as mandatory as far as
>>> I know.
>>
>> Even though it may not be documented, it is in fact mandatory. Otherwise
>> it's not possible to safely extend ioctl structs in general.
>
> The intention of the original patch was to ignore the args->points
> field if the flag is not set:
>
> if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE)
> point = args->point;
>
> Using args->point unconditionally later was therefore a mistake.
There is precedence in the ioctl, the pad member is checked against zero for
the same reason.
The check was there because it is invalid to pass when IMPORT/EXPORT_SYNC_FILE
was not set.
This is what I would recommend instead:
----
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 2d4ab745fdad9..176fac24a3198 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -857,7 +857,6 @@ drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void
*data,
struct drm_syncobj_handle *args = data;
unsigned int valid_flags = DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE |
DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE;
- u64 point = 0;
if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
return -EOPNOTSUPP;
@@ -868,15 +867,14 @@ drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev,
void *data,
if (args->flags & ~valid_flags)
return -EINVAL;
- if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE)
- point = args->point;
+ if (!(args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE) &&
+ !(args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE) &&
+ args->point)
+ return -EINVAL;
if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE)
return drm_syncobj_export_sync_file(file_private, args->handle,
- point, &args->fd);
-
- if (args->point)
- return -EINVAL;
+ args->point, &args->fd);
return drm_syncobj_handle_to_fd(file_private, args->handle,
&args->fd);
@@ -889,7 +887,6 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void
*data,
struct drm_syncobj_handle *args = data;
unsigned int valid_flags = DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_TIMELINE |
DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE;
- u64 point = 0;
if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
return -EOPNOTSUPP;
@@ -900,17 +897,16 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev,
void *data,
if (args->flags & ~valid_flags)
return -EINVAL;
- if (args->flags & DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_TIMELINE)
- point = args->point;
+ if (!(args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE) &&
+ !(args->flags & DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE) &&
+ args->point)
+ return -EINVAL;
if (args->flags & DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE)
return drm_syncobj_import_sync_file_fence(file_private,
args->fd,
args->handle,
- point);
-
- if (args->point)
- return -EINVAL;
+ args->point);
return drm_syncobj_fd_to_handle(file_private, args->fd,
&args->handle);
