[PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[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 <ju.orth@gmail.com>
---
This patch fixes a regression that would cause conversions between
syncobj handles and fds to fail if userspace did not initialize a
recently-added field to 0.
---
 drivers/gpu/drm/drm_syncobj.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 250734dee928..49eccb43ce63 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -875,7 +875,7 @@ drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
 		return drm_syncobj_export_sync_file(file_private, args->handle,
 						    point, &args->fd);
 
-	if (args->point)
+	if (point)
 		return -EINVAL;
 
 	return drm_syncobj_handle_to_fd(file_private, args->handle,
@@ -909,7 +909,7 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
 							  args->handle,
 							  point);
 
-	if (args->point)
+	if (point)
 		return -EINVAL;
 
 	return drm_syncobj_fd_to_handle(file_private, args->fd,

---
base-commit: eb71ab2bf72260054677e348498ba995a057c463
change-id: 20260301-point-4305b6417f55

Best regards,
-- 
Julian Orth <ju.orth@gmail.com>

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Christian König]

On 3/1/26 13:34, Julian Orth wrote:
> 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 <ju.orth@gmail.com>

Good catch, Reviewed-by: Christian König <christian.koenig@amd.com>

As long as nobody objects I'm going to push this to drm-misc-fixes later today.

Thanks,
Christian.

> ---
> This patch fixes a regression that would cause conversions between
> syncobj handles and fds to fail if userspace did not initialize a
> recently-added field to 0.
> ---
>  drivers/gpu/drm/drm_syncobj.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 250734dee928..49eccb43ce63 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -875,7 +875,7 @@ drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
>                 return drm_syncobj_export_sync_file(file_private, args->handle,
>                                                     point, &args->fd);
> 
> -       if (args->point)
> +       if (point)
>                 return -EINVAL;
> 
>         return drm_syncobj_handle_to_fd(file_private, args->handle,
> @@ -909,7 +909,7 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
>                                                           args->handle,
>                                                           point);
> 
> -       if (args->point)
> +       if (point)
>                 return -EINVAL;
> 
>         return drm_syncobj_fd_to_handle(file_private, args->fd,
> 
> ---
> base-commit: eb71ab2bf72260054677e348498ba995a057c463
> change-id: 20260301-point-4305b6417f55
> 
> Best regards,
> --
> Julian Orth <ju.orth@gmail.com>
> 

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Dmitry Osipenko]

On 3/2/26 14:27, Christian König wrote:
> On 3/1/26 13:34, Julian Orth wrote:
>> 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 <ju.orth@gmail.com>
> 
> Good catch, Reviewed-by: Christian König <christian.koenig@amd.com>
> 
> As long as nobody objects I'm going to push this to drm-misc-fixes later today.

No objections, thanks

-- 
Best regards,
Dmitry

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Michel Dänzer]

On 3/1/26 13:34, Julian Orth wrote:
> 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 <ju.orth@gmail.com>
> ---
> This patch fixes a regression that would cause conversions between
> syncobj handles and fds to fail if userspace did not initialize a
> recently-added field to 0.

While I don't object to this change, I'd argue that this is really a workaround for broken user space, not a fix for a kernel regression.

User space must initialize the full struct to 0, except for the fields it wants to have other values. This kind of kernel-side workaround for neglecting that won't be possible in all cases.


-- 
Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
https://redhat.com             \               Libre software enthusiast

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Julian Orth]

On Tue, Mar 3, 2026 at 12:17 PM Michel Dänzer
<michel.daenzer@mailbox.org> wrote:
>
> While I don't object to this change, I'd argue that this is really a workaround for broken user space, not a fix for a kernel regression.
>
> User space must initialize the full struct to 0, except for the fields it wants to have other values. This kind of kernel-side workaround for neglecting that won't be possible in all cases.

When the code in my example was written, the points field did not
exist. Therefore, as far as that code is concerned, the authors
believed that the struct was fully initialized, since they manually
initialized all fields that were contained in the struct at that time.
The code was then later recompiled against a version of drm/drm.h that
contained the new field.

Or are you saying that userspace must always use memset(&arg, 0,
sizeof(arg)) to initialize ioctl arguments? If so, ioctl(2) does not
mention this.

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Michel Dänzer]

On 3/3/26 12:23, Julian Orth wrote:
> On Tue, Mar 3, 2026 at 12:17 PM Michel Dänzer
> <michel.daenzer@mailbox.org> wrote:
>>
>> While I don't object to this change, I'd argue that this is really a workaround for broken user space, not a fix for a kernel regression.
>>
>> User space must initialize the full struct to 0, except for the fields it wants to have other values. This kind of kernel-side workaround for neglecting that won't be possible in all cases.
> 
> When the code in my example was written, the points field did not
> exist. Therefore, as far as that code is concerned, the authors
> believed that the struct was fully initialized, since they manually
> initialized all fields that were contained in the struct at that time.
> The code was then later recompiled against a version of drm/drm.h that
> contained the new field.
> 
> Or are you saying that userspace must always use memset(&arg, 0,
> sizeof(arg)) to initialize ioctl arguments?

Yes, it must. Any approach which results in newly-added fields not being initialized to 0 is broken.


To prevent a potential follow-up concern: What if the kernel uses a larger size of the struct than user space? That's fine, user space encodes the size it uses in the parameters to the ioctl system call, and the kernel uses 0 for all fields beyond that size.


-- 
Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
https://redhat.com             \               Libre software enthusiast

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Julian Orth]

On Tue, Mar 3, 2026 at 12:38 PM Michel Dänzer
<michel.daenzer@mailbox.org> wrote:
>
> On 3/3/26 12:23, Julian Orth wrote:
> > On Tue, Mar 3, 2026 at 12:17 PM Michel Dänzer
> > <michel.daenzer@mailbox.org> wrote:
> >>
> >> While I don't object to this change, I'd argue that this is really a workaround for broken user space, not a fix for a kernel regression.
> >>
> >> User space must initialize the full struct to 0, except for the fields it wants to have other values. This kind of kernel-side workaround for neglecting that won't be possible in all cases.
> >
> > When the code in my example was written, the points field did not
> > exist. Therefore, as far as that code is concerned, the authors
> > believed that the struct was fully initialized, since they manually
> > initialized all fields that were contained in the struct at that time.
> > The code was then later recompiled against a version of drm/drm.h that
> > contained the new field.
> >
> > Or are you saying that userspace must always use memset(&arg, 0,
> > sizeof(arg)) to initialize ioctl arguments?
>
> Yes, it must. Any approach which results in newly-added fields not being initialized to 0 is broken.

Do you know of any other case where access to the new fields is not
guarded by some kind of flag that exists in all versions of the
struct?

>
>
> To prevent a potential follow-up concern: What if the kernel uses a larger size of the struct than user space? That's fine, user space encodes the size it uses in the parameters to the ioctl system call, and the kernel uses 0 for all fields beyond that size.
>
>
> --
> Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
> https://redhat.com             \               Libre software enthusiast

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Maarten Lankhorst]

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 <ju.orth@gmail.com>

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.

Kind regards,
~Maarten Lankhorst

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Christian König]

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 <ju.orth@gmail.com>
> 
> 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.

Regards,
Christian.

> 
> Kind regards,
> ~Maarten Lankhorst

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Maarten Lankhorst]

Hey,

Den 2026-03-03 kl. 15:59, skrev Christian König:
> 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 <ju.orth@gmail.com>
>>
>> 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.
I know that in case of xe, even padding members are tested for being zero. For new code it's
explicitly recommended to test to prevent running into undefined behavior. In some cases
data may look valid, even if it's just random garbage from the stack.

Is it too late to revert?

Kind regards,
Maarten Lankhorst

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Julian Orth]

On Tue, Mar 3, 2026 at 4:15 PM Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
>
> Hey,
>
> Den 2026-03-03 kl. 15:59, skrev Christian König:
> > 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 <ju.orth@gmail.com>
> >>
> >> 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.
> I know that in case of xe, even padding members are tested for being zero. For new code it's
> explicitly recommended to test to prevent running into undefined behavior. In some cases
> data may look valid, even if it's just random garbage from the stack.

This isn't about padding fields. This is about a new field that was
added to the struct, increasing its size. Existing code could not have
zero-initialized that field except by using memset or something
equivalent. As long as the requirement to use memset is not
documented, requiring existing code to use memset is a breaking change
because code that didn't use memset always worked until the new field
was added.

>
> Is it too late to revert?
>
> Kind regards,
> Maarten Lankhorst

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Michel Dänzer]

On 3/3/26 16:21, Julian Orth wrote:
> On Tue, Mar 3, 2026 at 4:15 PM Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
>>
>> Hey,
>>
>> Den 2026-03-03 kl. 15:59, skrev Christian König:
>>> 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 <ju.orth@gmail.com>
>>>>
>>>> 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.
>> I know that in case of xe, even padding members are tested for being zero. For new code it's
>> explicitly recommended to test to prevent running into undefined behavior. In some cases
>> data may look valid, even if it's just random garbage from the stack.
> 
> This isn't about padding fields. This is about a new field that was
> added to the struct, increasing its size. Existing code could not have
> zero-initialized that field except by using memset or something
> equivalent. As long as the requirement to use memset is not
> documented, requiring existing code to use memset is a breaking change
> because code that didn't use memset always worked until the new field
> was added.

It's always been effectively mandatory, it's not a breaking change.


-- 
Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
https://redhat.com             \               Libre software enthusiast

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Michel Dänzer]

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 <ju.orth@gmail.com>
>>
>> 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.


-- 
Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
https://redhat.com             \               Libre software enthusiast

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Julian Orth]

On Tue, Mar 3, 2026 at 4:29 PM Michel Dänzer <michel.daenzer@mailbox.org> 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 <ju.orth@gmail.com>
> >>
> >> 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.

>
>
> --
> Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
> https://redhat.com             \               Libre software enthusiast

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Maarten Lankhorst]

Hey,

Den 2026-03-03 kl. 16:36, skrev Julian Orth:
> On Tue, Mar 3, 2026 at 4:29 PM Michel Dänzer <michel.daenzer@mailbox.org> 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 <ju.orth@gmail.com>
>>>>
>>>> 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);

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Julian Orth]

On Tue, Mar 3, 2026 at 5:40 PM Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
>
> There is precedence in the ioctl, the pad member is checked against zero for the same reason.

I don't believe that this is comparable. Developers of code developed
against an older kernel could look at the kernel and see that the pad
field was checked against zero. They could not see the same for fields
that didn't exist at the time.

> 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:
>
> [...]
>
> +       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;

Should it not be

+       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;

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Michel Dänzer]

On 3/3/26 17:54, Julian Orth wrote:
> On Tue, Mar 3, 2026 at 5:40 PM Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
>>
>> There is precedence in the ioctl, the pad member is checked against zero for the same reason.
> 
> I don't believe that this is comparable. Developers of code developed
> against an older kernel could look at the kernel and see that the pad
> field was checked against zero. They could not see the same for fields
> that didn't exist at the time.

"Initialize only known fields" isn't a valid approach here. The full struct must be initialized to 0, including any fields added in the future.


-- 
Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
https://redhat.com             \               Libre software enthusiast

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Julian Orth]

On Tue, Mar 3, 2026 at 6:04 PM Michel Dänzer <michel.daenzer@mailbox.org> wrote:
>
> On 3/3/26 17:54, Julian Orth wrote:
> > On Tue, Mar 3, 2026 at 5:40 PM Maarten Lankhorst
> > <maarten.lankhorst@linux.intel.com> wrote:
> >>
> >> There is precedence in the ioctl, the pad member is checked against zero for the same reason.
> >
> > I don't believe that this is comparable. Developers of code developed
> > against an older kernel could look at the kernel and see that the pad
> > field was checked against zero. They could not see the same for fields
> > that didn't exist at the time.
>
> "Initialize only known fields" isn't a valid approach here. The full struct must be initialized to 0, including any fields added in the future.

It worked from the introduction of the ioctl until the new field was
added. How could anyone know about this requirement if it is not
documented? Some people might not even know that ioctl numbers encode
the size of the argument and therefore assume that the argument
structure is fixed. I think this is quite different from syscalls such
as clone3 which make the size argument explicit and where it can be
expected that the developer knows that the struct might grow when the
application is recompiled.

The new flag was added so that userspace can detect older kernels that
don't support the point field, such kernels return EINVAL when they
see the new flag.
Then why should the kernel not also use the absence of the flag to
detect older userspace that might be unaware of the point field?

>
>
> --
> Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
> https://redhat.com             \               Libre software enthusiast

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Michel Dänzer]

On 3/3/26 18:11, Julian Orth wrote:
> On Tue, Mar 3, 2026 at 6:04 PM Michel Dänzer <michel.daenzer@mailbox.org> wrote:
>> On 3/3/26 17:54, Julian Orth wrote:
>>> On Tue, Mar 3, 2026 at 5:40 PM Maarten Lankhorst
>>> <maarten.lankhorst@linux.intel.com> wrote:
>>>>
>>>> There is precedence in the ioctl, the pad member is checked against zero for the same reason.
>>>
>>> I don't believe that this is comparable. Developers of code developed
>>> against an older kernel could look at the kernel and see that the pad
>>> field was checked against zero. They could not see the same for fields
>>> that didn't exist at the time.
>>
>> "Initialize only known fields" isn't a valid approach here. The full struct must be initialized to 0, including any fields added in the future.
> 
> It worked from the introduction of the ioctl until the new field was
> added. How could anyone know about this requirement if it is not
> documented? Some people might not even know that ioctl numbers encode
> the size of the argument and therefore assume that the argument
> structure is fixed. I think this is quite different from syscalls such
> as clone3 which make the size argument explicit and where it can be
> expected that the developer knows that the struct might grow when the
> application is recompiled.
> 
> The new flag was added so that userspace can detect older kernels that
> don't support the point field, such kernels return EINVAL when they
> see the new flag.
> Then why should the kernel not also use the absence of the flag to
> detect older userspace that might be unaware of the point field?

I wrote in my first post in this thread that I don't object to your patch, so you can relax and stop trying to convince me not to object to it. :)


I'm just pointing out that this is working around broken user-space code, and that there are other similar cases where that kind of broken users-space code couldn't be worked around in the kernel, so it's better to also fix the user-space code anyway.


-- 
Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
https://redhat.com             \               Libre software enthusiast

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Julian Orth]

On Tue, Mar 3, 2026 at 6:18 PM Michel Dänzer <michel.daenzer@mailbox.org> wrote:
>
> I wrote in my first post in this thread that I don't object to your patch, so you can relax and stop trying to convince me not to object to it. :)
>
> I'm just pointing out that this is working around broken user-space code, and that there are other similar cases where that kind of broken users-space code couldn't be worked around in the kernel, so it's better to also fix the user-space code anyway.

At this point I think we're arguing about "how can ioctls be extended"
and "does userspace have to use memset" in general, not just about
this particular ioctl. You've made the argument that ioctls are not
extensible in general unless userspace uses memset. However, I'm not
yet convinced of this. As you've also said above, drm_ioctl happily
truncates or zero-extends ioctl arguments without returning an error
due to size mismatch. Therefore, the only way for userspace to detect
if the kernel supports the "extended" ioctl is to add a flag so that
the kernel can return an error if it doesn't know the flag. And then
that flag could also be used by the kernel to detect which fields of
the argument are potentially uninitialized.

That is why I asked above if you knew of any other examples where an
ioctl was extended and where memset(0) became effectively required due
to the extension.

>
>
> --
> Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
> https://redhat.com             \               Libre software enthusiast

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Maarten Lankhorst]

Hey,

Den 2026-03-03 kl. 18:30, skrev Julian Orth:
> On Tue, Mar 3, 2026 at 6:18 PM Michel Dänzer <michel.daenzer@mailbox.org> wrote:
>>
>> I wrote in my first post in this thread that I don't object to your patch, so you can relax and stop trying to convince me not to object to it. :)
>>
>> I'm just pointing out that this is working around broken user-space code, and that there are other similar cases where that kind of broken users-space code couldn't be worked around in the kernel, so it's better to also fix the user-space code anyway.
> 
> At this point I think we're arguing about "how can ioctls be extended"
> and "does userspace have to use memset" in general, not just about
> this particular ioctl. You've made the argument that ioctls are not
> extensible in general unless userspace uses memset. However, I'm not
> yet convinced of this. As you've also said above, drm_ioctl happily
> truncates or zero-extends ioctl arguments without returning an error
> due to size mismatch. Therefore, the only way for userspace to detect
> if the kernel supports the "extended" ioctl is to add a flag so that
> the kernel can return an error if it doesn't know the flag. And then
> that flag could also be used by the kernel to detect which fields of
> the argument are potentially uninitialized.
> 
> That is why I asked above if you knew of any other examples where an
> ioctl was extended and where memset(0) became effectively required due
> to the extension.

You don't even need to use memset, this would work too:

struct drm_syncobj_handle args = {
	.flags = 0
};

Or simply struct drm_syncobj_handle args = { };

Kind regards,
~Maarten Lankhorst

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Michel Dänzer]

On 3/3/26 18:44, Maarten Lankhorst wrote:
> Den 2026-03-03 kl. 18:30, skrev Julian Orth:
>> On Tue, Mar 3, 2026 at 6:18 PM Michel Dänzer <michel.daenzer@mailbox.org> wrote:
>>>
>>> I wrote in my first post in this thread that I don't object to your patch, so you can relax and stop trying to convince me not to object to it. :)
>>>
>>> I'm just pointing out that this is working around broken user-space code, and that there are other similar cases where that kind of broken users-space code couldn't be worked around in the kernel, so it's better to also fix the user-space code anyway.
>>
>> At this point I think we're arguing about "how can ioctls be extended"
>> and "does userspace have to use memset" in general, not just about
>> this particular ioctl. You've made the argument that ioctls are not
>> extensible in general unless userspace uses memset. However, I'm not
>> yet convinced of this. As you've also said above, drm_ioctl happily
>> truncates or zero-extends ioctl arguments without returning an error
>> due to size mismatch. Therefore, the only way for userspace to detect
>> if the kernel supports the "extended" ioctl is to add a flag so that
>> the kernel can return an error if it doesn't know the flag. And then
>> that flag could also be used by the kernel to detect which fields of
>> the argument are potentially uninitialized.
>>
>> That is why I asked above if you knew of any other examples where an
>> ioctl was extended and where memset(0) became effectively required due
>> to the extension.

Since it's always been effectively required for ioctl structs, "become" doesn't apply.


In terms of documentation, the "(How to avoid) Botching up ioctls" page says under Basics:

* Check all unused fields and flags and all the padding for whether it’s 0, and reject the ioctl if that’s not the case.

Which is what the code you're modifying here did: The code after the args->point checks doesn't use the point field, so it's checking that user space initialized the field to 0 per above.

This contradicts the arguments in the commit log, so I'm leaning toward objecting to this patch now.


> You don't even need to use memset, this would work too:
> 
> struct drm_syncobj_handle args = {
> 	.flags = 0
> };

TL;DR: This method isn't 100% safe either.

It won't initialize any padding which isn't covered by any struct field. We try to avoid that and have explicit padding fields instead, mistakes may happen though, and in theory such padding could later be used for a new field.

FWIW, libdrm uses memset for all ioctl structs.


-- 
Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
https://redhat.com             \               Libre software enthusiast

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Julian Orth]

On Tue, Mar 3, 2026 at 7:53 PM Michel Dänzer <michel.daenzer@mailbox.org> wrote:
>
> On 3/3/26 18:44, Maarten Lankhorst wrote:
> > Den 2026-03-03 kl. 18:30, skrev Julian Orth:
> >> On Tue, Mar 3, 2026 at 6:18 PM Michel Dänzer <michel.daenzer@mailbox.org> wrote:
> >>>
> >>> I wrote in my first post in this thread that I don't object to your patch, so you can relax and stop trying to convince me not to object to it. :)
> >>>
> >>> I'm just pointing out that this is working around broken user-space code, and that there are other similar cases where that kind of broken users-space code couldn't be worked around in the kernel, so it's better to also fix the user-space code anyway.
> >>
> >> At this point I think we're arguing about "how can ioctls be extended"
> >> and "does userspace have to use memset" in general, not just about
> >> this particular ioctl. You've made the argument that ioctls are not
> >> extensible in general unless userspace uses memset. However, I'm not
> >> yet convinced of this. As you've also said above, drm_ioctl happily
> >> truncates or zero-extends ioctl arguments without returning an error
> >> due to size mismatch. Therefore, the only way for userspace to detect
> >> if the kernel supports the "extended" ioctl is to add a flag so that
> >> the kernel can return an error if it doesn't know the flag. And then
> >> that flag could also be used by the kernel to detect which fields of
> >> the argument are potentially uninitialized.
> >>
> >> That is why I asked above if you knew of any other examples where an
> >> ioctl was extended and where memset(0) became effectively required due
> >> to the extension.
>
> Since it's always been effectively required for ioctl structs, "become" doesn't apply.
>
>
> In terms of documentation, the "(How to avoid) Botching up ioctls" page says under Basics:
>
> * Check all unused fields and flags and all the padding for whether it’s 0, and reject the ioctl if that’s not the case.
>
> Which is what the code you're modifying here did: The code after the args->point checks doesn't use the point field, so it's checking that user space initialized the field to 0 per above.

I don't believe that is true. The old code only checked args->point if
the flags argument was 0. If the flags argument contained
EXPORT_SYNC_FILE but not TIMELINE, then the old code ignored
args->point completely. The patch suggested by Maarten does what
you're suggesting: It unconditionally verifies that args->point does
not contain garbage. However, since the original code only used
args->points if TIMELINE was set, I believe the intention behind the
TIMELINE flag was to ignore the args->point field if the flag was
unset. That assumption is the basis for my patch.

>
> This contradicts the arguments in the commit log, so I'm leaning toward objecting to this patch now.
>
>
> > You don't even need to use memset, this would work too:
> >
> > struct drm_syncobj_handle args = {
> >       .flags = 0
> > };
>
> TL;DR: This method isn't 100% safe either.
>
> It won't initialize any padding which isn't covered by any struct field. We try to avoid that and have explicit padding fields instead, mistakes may happen though, and in theory such padding could later be used for a new field.

I don't think this is workable. There is a lot of code out there that
effectively vendors the ioctl definitions (and so is not affected by
new fields) and relies on field-based initialization affecting all
significant bytes. For example
https://github.com/Smithay/drm-rs/blob/08dee22f0dcfa4a73a18ca3a954d4f7c2c749c03/drm-ffi/src/syncobj.rs#L48-L58.

>
> FWIW, libdrm uses memset for all ioctl structs.
>
>
> --
> Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
> https://redhat.com             \               Libre software enthusiast

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Maarten Lankhorst]

Hey,

Den 2026-03-03 kl. 20:12, skrev Julian Orth:
> On Tue, Mar 3, 2026 at 7:53 PM Michel Dänzer <michel.daenzer@mailbox.org> wrote:
>>
>> On 3/3/26 18:44, Maarten Lankhorst wrote:
>>> Den 2026-03-03 kl. 18:30, skrev Julian Orth:
>>>> On Tue, Mar 3, 2026 at 6:18 PM Michel Dänzer <michel.daenzer@mailbox.org> wrote:
>>>>>
>>>>> I wrote in my first post in this thread that I don't object to your patch, so you can relax and stop trying to convince me not to object to it. :)
>>>>>
>>>>> I'm just pointing out that this is working around broken user-space code, and that there are other similar cases where that kind of broken users-space code couldn't be worked around in the kernel, so it's better to also fix the user-space code anyway.
>>>>
>>>> At this point I think we're arguing about "how can ioctls be extended"
>>>> and "does userspace have to use memset" in general, not just about
>>>> this particular ioctl. You've made the argument that ioctls are not
>>>> extensible in general unless userspace uses memset. However, I'm not
>>>> yet convinced of this. As you've also said above, drm_ioctl happily
>>>> truncates or zero-extends ioctl arguments without returning an error
>>>> due to size mismatch. Therefore, the only way for userspace to detect
>>>> if the kernel supports the "extended" ioctl is to add a flag so that
>>>> the kernel can return an error if it doesn't know the flag. And then
>>>> that flag could also be used by the kernel to detect which fields of
>>>> the argument are potentially uninitialized.
>>>>
>>>> That is why I asked above if you knew of any other examples where an
>>>> ioctl was extended and where memset(0) became effectively required due
>>>> to the extension.
>>
>> Since it's always been effectively required for ioctl structs, "become" doesn't apply.
>>
>>
>> In terms of documentation, the "(How to avoid) Botching up ioctls" page says under Basics:
>>
>> * Check all unused fields and flags and all the padding for whether it’s 0, and reject the ioctl if that’s not the case.
>>
>> Which is what the code you're modifying here did: The code after the args->point checks doesn't use the point field, so it's checking that user space initialized the field to 0 per above.
> 
> I don't believe that is true. The old code only checked args->point if
> the flags argument was 0. If the flags argument contained
> EXPORT_SYNC_FILE but not TIMELINE, then the old code ignored
> args->point completely. The patch suggested by Maarten does what
> you're suggesting: It unconditionally verifies that args->point does
> not contain garbage. However, since the original code only used
> args->points if TIMELINE was set, I believe the intention behind the
> TIMELINE flag was to ignore the args->point field if the flag was
> unset. That assumption is the basis for my patch.
> 
>>
>> This contradicts the arguments in the commit log, so I'm leaning toward objecting to this patch now.
>>
>>
>>> You don't even need to use memset, this would work too:
>>>
>>> struct drm_syncobj_handle args = {
>>>       .flags = 0
>>> };
>>
>> TL;DR: This method isn't 100% safe either.
>>
>> It won't initialize any padding which isn't covered by any struct field. We try to avoid that and have explicit padding fields instead, mistakes may happen though, and in theory such padding could later be used for a new field.
> 
> I don't think this is workable. There is a lot of code out there that
> effectively vendors the ioctl definitions (and so is not affected by
> new fields) and relies on field-based initialization affecting all
> significant bytes. For example
> https://github.com/Smithay/drm-rs/blob/08dee22f0dcfa4a73a18ca3a954d4f7c2c749c03/drm-ffi/src/syncobj.rs#L48-L58.

Vendoring is not affected. When you carry your own definition, it's your
responsibility for updating the users if you update the struct too.

Using rust here is a good example. The rust code would still use the old definition.
If you use it against the new definition it fails to compile, or use default valuesif asked.
You would notice your newly introduced bug at compile time.

Kind regards,
~Maarten Lankhorst

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Michel Dänzer]

On 3/3/26 20:12, Julian Orth wrote:
> On Tue, Mar 3, 2026 at 7:53 PM Michel Dänzer <michel.daenzer@mailbox.org> wrote:
>>
>> On 3/3/26 18:44, Maarten Lankhorst wrote:
>>> Den 2026-03-03 kl. 18:30, skrev Julian Orth:
>>>> On Tue, Mar 3, 2026 at 6:18 PM Michel Dänzer <michel.daenzer@mailbox.org> wrote:
>>>>>
>>>>> I wrote in my first post in this thread that I don't object to your patch, so you can relax and stop trying to convince me not to object to it. :)
>>>>>
>>>>> I'm just pointing out that this is working around broken user-space code, and that there are other similar cases where that kind of broken users-space code couldn't be worked around in the kernel, so it's better to also fix the user-space code anyway.
>>>>
>>>> At this point I think we're arguing about "how can ioctls be extended"
>>>> and "does userspace have to use memset" in general, not just about
>>>> this particular ioctl. You've made the argument that ioctls are not
>>>> extensible in general unless userspace uses memset. However, I'm not
>>>> yet convinced of this. As you've also said above, drm_ioctl happily
>>>> truncates or zero-extends ioctl arguments without returning an error
>>>> due to size mismatch. Therefore, the only way for userspace to detect
>>>> if the kernel supports the "extended" ioctl is to add a flag so that
>>>> the kernel can return an error if it doesn't know the flag. And then
>>>> that flag could also be used by the kernel to detect which fields of
>>>> the argument are potentially uninitialized.
>>>>
>>>> That is why I asked above if you knew of any other examples where an
>>>> ioctl was extended and where memset(0) became effectively required due
>>>> to the extension.
>>
>> Since it's always been effectively required for ioctl structs, "become" doesn't apply.
>>
>>
>> In terms of documentation, the "(How to avoid) Botching up ioctls" page says under Basics:
>>
>> * Check all unused fields and flags and all the padding for whether it’s 0, and reject the ioctl if that’s not the case.
>>
>> Which is what the code you're modifying here did: The code after the args->point checks doesn't use the point field, so it's checking that user space initialized the field to 0 per above.
> 
> I don't believe that is true. The old code only checked args->point if
> the flags argument was 0. If the flags argument contained
> EXPORT_SYNC_FILE but not TIMELINE, then the old code ignored
> args->point completely.

Right, the code didn't fully implement the rule which has been documented for 13 years. Makes no practical difference though, the user-space code would have hit a failure regardless.


> However, since the original code only used args->points if TIMELINE was set,
> I believe the intention behind the TIMELINE flag was to ignore the args->point
> field if the flag was unset. That assumption is the basis for my patch.

Whatever Rob's intention was, checking the value of the local variable instead seems of little use, since it can have a non-0 value only if the TIMELINE flag is set. If the intention is to catch the TIMELINE flag being set without the IMPORT_SYNC_FILE flag, that should be done directly instead, or it won't be caught if args->point is 0.


>>> You don't even need to use memset, this would work too:
>>>
>>> struct drm_syncobj_handle args = {
>>>       .flags = 0
>>> };
>>
>> TL;DR: This method isn't 100% safe either.
>>
>> It won't initialize any padding which isn't covered by any struct field. We try to avoid that and have explicit padding fields instead, mistakes may happen though, and in theory such padding could later be used for a new field.
> 
> I don't think this is workable.

libdrm begs to differ. It shows that it's not only workable but really easy. There's no reason for doing it any other way.


-- 
Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
https://redhat.com             \               Libre software enthusiast

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Julian Orth]

On Wed, Mar 4, 2026 at 12:15 PM Michel Dänzer
<michel.daenzer@mailbox.org> wrote:
>
> On 3/3/26 20:12, Julian Orth wrote:
> > On Tue, Mar 3, 2026 at 7:53 PM Michel Dänzer <michel.daenzer@mailbox.org> wrote:
> >>
> >> On 3/3/26 18:44, Maarten Lankhorst wrote:
> >>> Den 2026-03-03 kl. 18:30, skrev Julian Orth:
> >>>> On Tue, Mar 3, 2026 at 6:18 PM Michel Dänzer <michel.daenzer@mailbox.org> wrote:
> >>>>>
> >>>>> I wrote in my first post in this thread that I don't object to your patch, so you can relax and stop trying to convince me not to object to it. :)
> >>>>>
> >>>>> I'm just pointing out that this is working around broken user-space code, and that there are other similar cases where that kind of broken users-space code couldn't be worked around in the kernel, so it's better to also fix the user-space code anyway.
> >>>>
> >>>> At this point I think we're arguing about "how can ioctls be extended"
> >>>> and "does userspace have to use memset" in general, not just about
> >>>> this particular ioctl. You've made the argument that ioctls are not
> >>>> extensible in general unless userspace uses memset. However, I'm not
> >>>> yet convinced of this. As you've also said above, drm_ioctl happily
> >>>> truncates or zero-extends ioctl arguments without returning an error
> >>>> due to size mismatch. Therefore, the only way for userspace to detect
> >>>> if the kernel supports the "extended" ioctl is to add a flag so that
> >>>> the kernel can return an error if it doesn't know the flag. And then
> >>>> that flag could also be used by the kernel to detect which fields of
> >>>> the argument are potentially uninitialized.
> >>>>
> >>>> That is why I asked above if you knew of any other examples where an
> >>>> ioctl was extended and where memset(0) became effectively required due
> >>>> to the extension.
> >>
> >> Since it's always been effectively required for ioctl structs, "become" doesn't apply.
> >>
> >>
> >> In terms of documentation, the "(How to avoid) Botching up ioctls" page says under Basics:
> >>
> >> * Check all unused fields and flags and all the padding for whether it’s 0, and reject the ioctl if that’s not the case.
> >>
> >> Which is what the code you're modifying here did: The code after the args->point checks doesn't use the point field, so it's checking that user space initialized the field to 0 per above.
> >
> > I don't believe that is true. The old code only checked args->point if
> > the flags argument was 0. If the flags argument contained
> > EXPORT_SYNC_FILE but not TIMELINE, then the old code ignored
> > args->point completely.
>
> Right, the code didn't fully implement the rule which has been documented for 13 years. Makes no practical difference though, the user-space code would have hit a failure regardless.
>
>
> > However, since the original code only used args->points if TIMELINE was set,
> > I believe the intention behind the TIMELINE flag was to ignore the args->point
> > field if the flag was unset. That assumption is the basis for my patch.
>
> Whatever Rob's intention was, checking the value of the local variable instead seems of little use, since it can have a non-0 value only if the TIMELINE flag is set. If the intention is to catch the TIMELINE flag being set without the IMPORT_SYNC_FILE flag, that should be done directly instead, or it won't be caught if args->point is 0.

You're right, the better check would probably have been (using a shorthand)

    if (!IMPORT_SYNC_FILE && TIMELINE) return EINVAL;

However, since that was not done at the time, there might now be
userspace that relies on this not returning an error as long as the
point is 0. The intention of my patch was to be strictly more lenient.

>
>
> >>> You don't even need to use memset, this would work too:
> >>>
> >>> struct drm_syncobj_handle args = {
> >>>       .flags = 0
> >>> };
> >>
> >> TL;DR: This method isn't 100% safe either.
> >>
> >> It won't initialize any padding which isn't covered by any struct field. We try to avoid that and have explicit padding fields instead, mistakes may happen though, and in theory such padding could later be used for a new field.
> >
> > I don't think this is workable.
>
> libdrm begs to differ. It shows that it's not only workable but really easy. There's no reason for doing it any other way.

Using memset to initialize padding bytes between fields is workable.
Having the kernel add checks for this for existing ioctls is not
workable because it would break usespace that doesn't do this. Which
is every rust program out there as far as I can tell.

I'm not aware of any ioctls that actually have padding bytes between
fields so this discussion is mostly academic.

>
>
> --
> Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
> https://redhat.com             \               Libre software enthusiast

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Michel Dänzer]

On 3/4/26 12:25, Julian Orth wrote:
> On Wed, Mar 4, 2026 at 12:15 PM Michel Dänzer
> <michel.daenzer@mailbox.org> wrote:
>> On 3/3/26 20:12, Julian Orth wrote:
>>> On Tue, Mar 3, 2026 at 7:53 PM Michel Dänzer <michel.daenzer@mailbox.org> wrote:
>>>> On 3/3/26 18:44, Maarten Lankhorst wrote:
>>>>>
>>>>> You don't even need to use memset, this would work too:
>>>>>
>>>>> struct drm_syncobj_handle args = {
>>>>>       .flags = 0
>>>>> };
>>>>
>>>> TL;DR: This method isn't 100% safe either.
>>>>
>>>> It won't initialize any padding which isn't covered by any struct field. We try to avoid that and have explicit padding fields instead, mistakes may happen though, and in theory such padding could later be used for a new field.
>>>
>>> I don't think this is workable.
>>
>> libdrm begs to differ. It shows that it's not only workable but really easy. There's no reason for doing it any other way.
> 
> Using memset to initialize padding bytes between fields is workable.
> Having the kernel add checks for this for existing ioctls is not
> workable because it would break usespace that doesn't do this.

As discussed in this thread, memset is also required for when the size of an ioctl struct is extended, even if there is no such padding.


> Which is every rust program out there as far as I can tell.

That's surprising. Surely there must be some unsafe code involved which allows uninitialized memory to be passed to ioctl()?


> I'm not aware of any ioctls that actually have padding bytes between
> fields so this discussion is mostly academic.

I covered that in my previous post quoted above.


-- 
Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
https://redhat.com             \               Libre software enthusiast

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Julian Orth]

On Wed, Mar 4, 2026 at 12:47 PM Michel Dänzer
<michel.daenzer@mailbox.org> wrote:
>
> On 3/4/26 12:25, Julian Orth wrote:
> > On Wed, Mar 4, 2026 at 12:15 PM Michel Dänzer
> > <michel.daenzer@mailbox.org> wrote:
> >> On 3/3/26 20:12, Julian Orth wrote:
> >>> On Tue, Mar 3, 2026 at 7:53 PM Michel Dänzer <michel.daenzer@mailbox.org> wrote:
> >>>> On 3/3/26 18:44, Maarten Lankhorst wrote:
> >>>>>
> >>>>> You don't even need to use memset, this would work too:
> >>>>>
> >>>>> struct drm_syncobj_handle args = {
> >>>>>       .flags = 0
> >>>>> };
> >>>>
> >>>> TL;DR: This method isn't 100% safe either.
> >>>>
> >>>> It won't initialize any padding which isn't covered by any struct field. We try to avoid that and have explicit padding fields instead, mistakes may happen though, and in theory such padding could later be used for a new field.
> >>>
> >>> I don't think this is workable.
> >>
> >> libdrm begs to differ. It shows that it's not only workable but really easy. There's no reason for doing it any other way.
> >
> > Using memset to initialize padding bytes between fields is workable.
> > Having the kernel add checks for this for existing ioctls is not
> > workable because it would break usespace that doesn't do this.
>
> As discussed in this thread, memset is also required for when the size of an ioctl struct is extended, even if there is no such padding.

That is not a concern in rust code. If the rust code extends the
definition of the struct, all call sites will cause a compilation
error until the new field is also initialized.

The issue I'm talking about here is strictly about padding bytes between fields.

>
>
> > Which is every rust program out there as far as I can tell.
>
> That's surprising. Surely there must be some unsafe code involved which allows uninitialized memory to be passed to ioctl()?

The memory is not uninitialized from the perspective of the rust
language since all fields are initialized. Only the padding bytes are
uninitialized and they cannot be accessed in safe rust, therefore no
unsafe is required.

I've never seen any rust code that uses memset to initialize ioctl
arguments. It assumes that the ioctl implementation will only read
from the named fields. Therefore, while the ioctl syscall itself is
unsafe, developers assume that the safety requirements are satisfied
in this regard.

>
>
> > I'm not aware of any ioctls that actually have padding bytes between
> > fields so this discussion is mostly academic.
>
> I covered that in my previous post quoted above.
>
>
> --
> Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
> https://redhat.com             \               Libre software enthusiast

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Michel Dänzer]

On 3/4/26 13:32, Julian Orth wrote:
> On Wed, Mar 4, 2026 at 12:47 PM Michel Dänzer
> <michel.daenzer@mailbox.org> wrote:
>> On 3/4/26 12:25, Julian Orth wrote:
>>> On Wed, Mar 4, 2026 at 12:15 PM Michel Dänzer
>>> <michel.daenzer@mailbox.org> wrote:
>>>> On 3/3/26 20:12, Julian Orth wrote:
>>>>> On Tue, Mar 3, 2026 at 7:53 PM Michel Dänzer <michel.daenzer@mailbox.org> wrote:
>>>>>> On 3/3/26 18:44, Maarten Lankhorst wrote:
>>>>>>>
>>>>>>> You don't even need to use memset, this would work too:
>>>>>>>
>>>>>>> struct drm_syncobj_handle args = {
>>>>>>>       .flags = 0
>>>>>>> };
>>>>>>
>>>>>> TL;DR: This method isn't 100% safe either.
>>>>>>
>>>>>> It won't initialize any padding which isn't covered by any struct field. We try to avoid that and have explicit padding fields instead, mistakes may happen though, and in theory such padding could later be used for a new field.
>>>>>
>>>>> I don't think this is workable.
>>>>
>>>> libdrm begs to differ. It shows that it's not only workable but really easy. There's no reason for doing it any other way.
>>>
>>> Using memset to initialize padding bytes between fields is workable.
>>> Having the kernel add checks for this for existing ioctls is not
>>> workable because it would break usespace that doesn't do this.
>>
>> As discussed in this thread, memset is also required for when the size of an ioctl struct is extended, even if there is no such padding.
> 
> That is not a concern in rust code. If the rust code extends the
> definition of the struct, all call sites will cause a compilation
> error until the new field is also initialized.
> 
> The issue I'm talking about here is strictly about padding bytes between fields.

So this is not related to the issue which motivated your patch?


>>> Which is every rust program out there as far as I can tell.
>>
>> That's surprising. Surely there must be some unsafe code involved which allows uninitialized memory to be passed to ioctl()?
> 
> The memory is not uninitialized from the perspective of the rust
> language since all fields are initialized. Only the padding bytes are
> uninitialized and they cannot be accessed in safe rust, therefore no
> unsafe is required.
> 
> I've never seen any rust code that uses memset to initialize ioctl
> arguments. It assumes that the ioctl implementation will only read
> from the named fields. Therefore, while the ioctl syscall itself is
> unsafe, developers assume that the safety requirements are satisfied
> in this regard.

As discussed in this thread, that's not a valid assumption and may blow up in their face sooner or later. ("No padding not covered by any fields" is best-effort, not a guarantee)


-- 
Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
https://redhat.com             \               Libre software enthusiast

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Julian Orth]

On Wed, Mar 4, 2026 at 3:30 PM Michel Dänzer <michel.daenzer@mailbox.org> wrote:
>
> On 3/4/26 13:32, Julian Orth wrote:
> > On Wed, Mar 4, 2026 at 12:47 PM Michel Dänzer
> > <michel.daenzer@mailbox.org> wrote:
> >> On 3/4/26 12:25, Julian Orth wrote:
> >>> On Wed, Mar 4, 2026 at 12:15 PM Michel Dänzer
> >>> <michel.daenzer@mailbox.org> wrote:
> >>>> On 3/3/26 20:12, Julian Orth wrote:
> >>>>> On Tue, Mar 3, 2026 at 7:53 PM Michel Dänzer <michel.daenzer@mailbox.org> wrote:
> >>>>>> On 3/3/26 18:44, Maarten Lankhorst wrote:
> >>>>>>>
> >>>>>>> You don't even need to use memset, this would work too:
> >>>>>>>
> >>>>>>> struct drm_syncobj_handle args = {
> >>>>>>>       .flags = 0
> >>>>>>> };
> >>>>>>
> >>>>>> TL;DR: This method isn't 100% safe either.
> >>>>>>
> >>>>>> It won't initialize any padding which isn't covered by any struct field. We try to avoid that and have explicit padding fields instead, mistakes may happen though, and in theory such padding could later be used for a new field.
> >>>>>
> >>>>> I don't think this is workable.
> >>>>
> >>>> libdrm begs to differ. It shows that it's not only workable but really easy. There's no reason for doing it any other way.
> >>>
> >>> Using memset to initialize padding bytes between fields is workable.
> >>> Having the kernel add checks for this for existing ioctls is not
> >>> workable because it would break usespace that doesn't do this.
> >>
> >> As discussed in this thread, memset is also required for when the size of an ioctl struct is extended, even if there is no such padding.
> >
> > That is not a concern in rust code. If the rust code extends the
> > definition of the struct, all call sites will cause a compilation
> > error until the new field is also initialized.
> >
> > The issue I'm talking about here is strictly about padding bytes between fields.
>
> So this is not related to the issue which motivated your patch?

I don't believe the ioctls in question contain padding bytes on any
architecture. This thread is in response to your statement above:

>>>>>>> struct drm_syncobj_handle args = {
>>>>>>>       .flags = 0
>>>>>>> };
>>>>>>
>>>>>> TL;DR: This method isn't 100% safe either.
>>>>>>
>>>>>> It won't initialize any padding which isn't covered by any struct field. We try to avoid that and have explicit padding fields instead, mistakes may happen though, and in theory such padding could later be used for a new field.

----

>
>
> >>> Which is every rust program out there as far as I can tell.
> >>
> >> That's surprising. Surely there must be some unsafe code involved which allows uninitialized memory to be passed to ioctl()?
> >
> > The memory is not uninitialized from the perspective of the rust
> > language since all fields are initialized. Only the padding bytes are
> > uninitialized and they cannot be accessed in safe rust, therefore no
> > unsafe is required.
> >
> > I've never seen any rust code that uses memset to initialize ioctl
> > arguments. It assumes that the ioctl implementation will only read
> > from the named fields. Therefore, while the ioctl syscall itself is
> > unsafe, developers assume that the safety requirements are satisfied
> > in this regard.
>
> As discussed in this thread, that's not a valid assumption and may blow up in their face sooner or later. ("No padding not covered by any fields" is best-effort, not a guarantee)
>
>
> --
> Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
> https://redhat.com             \               Libre software enthusiast

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Maarten Lankhorst]

Hey,

Den 2026-03-04 kl. 12:25, skrev Julian Orth:
> On Wed, Mar 4, 2026 at 12:15 PM Michel Dänzer
> <michel.daenzer@mailbox.org> wrote:
>>
>> On 3/3/26 20:12, Julian Orth wrote:
>>> On Tue, Mar 3, 2026 at 7:53 PM Michel Dänzer <michel.daenzer@mailbox.org> wrote:
>>>>
>>>> On 3/3/26 18:44, Maarten Lankhorst wrote:
>>>>> Den 2026-03-03 kl. 18:30, skrev Julian Orth:
>>>>>> On Tue, Mar 3, 2026 at 6:18 PM Michel Dänzer <michel.daenzer@mailbox.org> wrote:
>>>>>>>
>>>>>>> I wrote in my first post in this thread that I don't object to your patch, so you can relax and stop trying to convince me not to object to it. :)
>>>>>>>
>>>>>>> I'm just pointing out that this is working around broken user-space code, and that there are other similar cases where that kind of broken users-space code couldn't be worked around in the kernel, so it's better to also fix the user-space code anyway.
>>>>>>
>>>>>> At this point I think we're arguing about "how can ioctls be extended"
>>>>>> and "does userspace have to use memset" in general, not just about
>>>>>> this particular ioctl. You've made the argument that ioctls are not
>>>>>> extensible in general unless userspace uses memset. However, I'm not
>>>>>> yet convinced of this. As you've also said above, drm_ioctl happily
>>>>>> truncates or zero-extends ioctl arguments without returning an error
>>>>>> due to size mismatch. Therefore, the only way for userspace to detect
>>>>>> if the kernel supports the "extended" ioctl is to add a flag so that
>>>>>> the kernel can return an error if it doesn't know the flag. And then
>>>>>> that flag could also be used by the kernel to detect which fields of
>>>>>> the argument are potentially uninitialized.
>>>>>>
>>>>>> That is why I asked above if you knew of any other examples where an
>>>>>> ioctl was extended and where memset(0) became effectively required due
>>>>>> to the extension.
>>>>
>>>> Since it's always been effectively required for ioctl structs, "become" doesn't apply.
>>>>
>>>>
>>>> In terms of documentation, the "(How to avoid) Botching up ioctls" page says under Basics:
>>>>
>>>> * Check all unused fields and flags and all the padding for whether it’s 0, and reject the ioctl if that’s not the case.
>>>>
>>>> Which is what the code you're modifying here did: The code after the args->point checks doesn't use the point field, so it's checking that user space initialized the field to 0 per above.
>>>
>>> I don't believe that is true. The old code only checked args->point if
>>> the flags argument was 0. If the flags argument contained
>>> EXPORT_SYNC_FILE but not TIMELINE, then the old code ignored
>>> args->point completely.
>>
>> Right, the code didn't fully implement the rule which has been documented for 13 years. Makes no practical difference though, the user-space code would have hit a failure regardless.
>>
>>
>>> However, since the original code only used args->points if TIMELINE was set,
>>> I believe the intention behind the TIMELINE flag was to ignore the args->point
>>> field if the flag was unset. That assumption is the basis for my patch.
>>
>> Whatever Rob's intention was, checking the value of the local variable instead seems of little use, since it can have a non-0 value only if the TIMELINE flag is set. If the intention is to catch the TIMELINE flag being set without the IMPORT_SYNC_FILE flag, that should be done directly instead, or it won't be caught if args->point is 0.
> 
> You're right, the better check would probably have been (using a shorthand)
> 
>     if (!IMPORT_SYNC_FILE && TIMELINE) return EINVAL;
> 
> However, since that was not done at the time, there might now be
> userspace that relies on this not returning an error as long as the
> point is 0. The intention of my patch was to be strictly more lenient.
> 
>>
>>
>>>>> You don't even need to use memset, this would work too:
>>>>>
>>>>> struct drm_syncobj_handle args = {
>>>>>       .flags = 0
>>>>> };
>>>>
>>>> TL;DR: This method isn't 100% safe either.
>>>>
>>>> It won't initialize any padding which isn't covered by any struct field. We try to avoid that and have explicit padding fields instead, mistakes may happen though, and in theory such padding could later be used for a new field.
>>>
>>> I don't think this is workable.
>>
>> libdrm begs to differ. It shows that it's not only workable but really easy. There's no reason for doing it any other way.
> 
> Using memset to initialize padding bytes between fields is workable.
> Having the kernel add checks for this for existing ioctls is not
> workable because it would break usespace that doesn't do this. Which
> is every rust program out there as far as I can tell.
> 
> I'm not aware of any ioctls that actually have padding bytes between
> fields so this discussion is mostly academic.

This is exactly why we attempt to ensure there is no implicit padding,
and explicitly check for all 'pad' members to be zero. Additionally
we attempt to stay 32-bits vs 64-bits clean, so no handling needs to
be performed in the compat ioctl's layout.

Kind regards,
~Maarten Lankhorst

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Maarten Lankhorst]

Den 2026-03-03 kl. 18:18, skrev Michel Dänzer:
> On 3/3/26 18:11, Julian Orth wrote:
>> On Tue, Mar 3, 2026 at 6:04 PM Michel Dänzer <michel.daenzer@mailbox.org> wrote:
>>> On 3/3/26 17:54, Julian Orth wrote:
>>>> On Tue, Mar 3, 2026 at 5:40 PM Maarten Lankhorst
>>>> <maarten.lankhorst@linux.intel.com> wrote:
>>>>>
>>>>> There is precedence in the ioctl, the pad member is checked against zero for the same reason.
>>>>
>>>> I don't believe that this is comparable. Developers of code developed
>>>> against an older kernel could look at the kernel and see that the pad
>>>> field was checked against zero. They could not see the same for fields
>>>> that didn't exist at the time.
>>>
>>> "Initialize only known fields" isn't a valid approach here. The full struct must be initialized to 0, including any fields added in the future.
>>
>> It worked from the introduction of the ioctl until the new field was
>> added. How could anyone know about this requirement if it is not
>> documented? Some people might not even know that ioctl numbers encode
>> the size of the argument and therefore assume that the argument
>> structure is fixed. I think this is quite different from syscalls such
>> as clone3 which make the size argument explicit and where it can be
>> expected that the developer knows that the struct might grow when the
>> application is recompiled.
>>
>> The new flag was added so that userspace can detect older kernels that
>> don't support the point field, such kernels return EINVAL when they
>> see the new flag.
>> Then why should the kernel not also use the absence of the flag to
>> detect older userspace that might be unaware of the point field?
> 
> I wrote in my first post in this thread that I don't object to your patch, so you can relax and stop trying to convince me not to object to it. :)
> 
> 
> I'm just pointing out that this is working around broken user-space code, and that there are other similar cases where that kind of broken users-space code couldn't be worked around in the kernel, so it's better to also fix the user-space code anyway.


My point is that it works for old userspace without problems. It's only
newly compiled userspace with new headers that will run into problems.
The code as written would have continued to work, but if you update to
the new header and don't initialise the new members then it's a userspace
problem. It should not be worked around in the kernel, since it's newly
written bad userspace code, not old bad userspace code that stopped working
when the kernel changed.

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Julian Orth]

On Tue, Mar 3, 2026 at 6:41 PM Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
>
> My point is that it works for old userspace without problems. It's only
> newly compiled userspace with new headers that will run into problems.
> The code as written would have continued to work, but if you update to
> the new header and don't initialise the new members then it's a userspace
> problem. It should not be worked around in the kernel, since it's newly
> written bad userspace code, not old bad userspace code that stopped working
> when the kernel changed.

But it's not newly written. The example is, say, 5 year old code. The
binary that was compiled 5 years ago works fine as you say. But if you
take the same code and just run gcc again, the new binary will no
longer work.

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[David Laight]

On Tue, 3 Mar 2026 18:44:59 +0100
Julian Orth <ju.orth@gmail.com> wrote:

> On Tue, Mar 3, 2026 at 6:41 PM Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
> >
> > My point is that it works for old userspace without problems. It's only
> > newly compiled userspace with new headers that will run into problems.
> > The code as written would have continued to work, but if you update to
> > the new header and don't initialise the new members then it's a userspace
> > problem. It should not be worked around in the kernel, since it's newly
> > written bad userspace code, not old bad userspace code that stopped working
> > when the kernel changed.  
> 
> But it's not newly written. The example is, say, 5 year old code. The
> binary that was compiled 5 years ago works fine as you say. But if you
> take the same code and just run gcc again, the new binary will no
> longer work.
> 

Worse, the recompiled version may well work when you test it, and even
when deployed.
But you'll get non-obvious random failures - a support nightmare.

Probably best code is something like:
	case OLD_IOCTL_CODE:
		if (ioc->flag & NEW_FLAG)
			return -EINVAL;
		/* FALLTHROUGH *.
	case NEW_IOCTL_CODE:
		if (!(ioc->flag & NEW_FLAG))
			ioc->new_field = 0;

    David

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Maarten Lankhorst]

Hey,

Den 2026-03-03 kl. 20:58, skrev David Laight:
> On Tue, 3 Mar 2026 18:44:59 +0100
> Julian Orth <ju.orth@gmail.com> wrote:
> 
>> On Tue, Mar 3, 2026 at 6:41 PM Maarten Lankhorst
>> <maarten.lankhorst@linux.intel.com> wrote:
>>>
>>> My point is that it works for old userspace without problems. It's only
>>> newly compiled userspace with new headers that will run into problems.
>>> The code as written would have continued to work, but if you update to
>>> the new header and don't initialise the new members then it's a userspace
>>> problem. It should not be worked around in the kernel, since it's newly
>>> written bad userspace code, not old bad userspace code that stopped working
>>> when the kernel changed.  
>>
>> But it's not newly written. The example is, say, 5 year old code. The
>> binary that was compiled 5 years ago works fine as you say. But if you
>> take the same code and just run gcc again, the new binary will no
>> longer work.
>>
> 
> Worse, the recompiled version may well work when you test it, and even
> when deployed.
> But you'll get non-obvious random failures - a support nightmare.

I believe in a lot of cases the headers are simply copied to the project,
so it will continue to work.

I don't see a difference between moving to a new compiler. Compiling code
from 5 years ago might also run into new compiler warnings that didn't
exist on the older version of the compiler.

> Probably best code is something like:
> 	case OLD_IOCTL_CODE:
> 		if (ioc->flag & NEW_FLAG)
> 			return -EINVAL;
> 		/* FALLTHROUGH *.
> 	case NEW_IOCTL_CODE:
> 		if (!(ioc->flag & NEW_FLAG))
> 			ioc->new_field = 0;
> 
>     David
Yeah, that is what we do by explicitly checking the size parameter. Any
undefined members are zero'd. If you define the new member in userspace,
you're also taking on the responsibility for having it contained defined
information, even if you only zero the struct.

Kind regards,
~Maarten Lankhorst

Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Maarten Lankhorst]

Hey,

Den 2026-03-03 kl. 17:54, skrev Julian Orth:
> On Tue, Mar 3, 2026 at 5:40 PM Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
>>
>> There is precedence in the ioctl, the pad member is checked against zero for the same reason.
> 
> I don't believe that this is comparable. Developers of code developed
> against an older kernel could look at the kernel and see that the pad
> field was checked against zero. They could not see the same for fields
> that didn't exist at the time.
> 
>> 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:
>>
>> [...]
>>
>> +       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;
> 
> Should it not be
> 
> +       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;
Yeah copy paste error. :)