On Wed, Mar 4, 2026 at 12:15 PM Michel Dänzer
<[email protected]> wrote:
>
> On 3/3/26 20:12, Julian Orth wrote:
> > 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.
>
> 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
