On Tue, Mar 3, 2026 at 7:53 PM Michel Dänzer <[email protected]> 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 <[email protected]>
> >> 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
