On Tue, Mar 3, 2026 at 4:15 PM Maarten Lankhorst <[email protected]> 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 <[email protected]> > >> > >> I'm not convinced this is the correct fix. > >> Userspace built before the change had the old size for drm_syncobj_create, > >> the size is encoded into the ioctl, and zero extended as needed. > >> > >> See drivers/gpu/drm/drm_ioctl.c: > >> out_size = in_size = _IOC_SIZE(cmd); > >> ... > >> if (ksize > in_size) > >> memset(kdata + in_size, 0, ksize - in_size); > >> > >> This is a bug in a newly built app, and should be handled by explicitly > >> zeroing > >> the entire struct or using named initializers, and only setting specific > >> members > >> as required. > >> > >> In particular, apps built before the change will never encounter this bug. > > > > Yeah, I've realized that after pushing the patch as well. > > > > But I still think this patch is the right thing to do, because without > > requesting the functionality by setting the flag the point should clearly > > not have any effect at all. > > > > And when an application would have only explicitly assigned the fields > > known previously and then later been compiled with the new points field it > > would have failed. > > > > It is good practice to memset() structures given to the kernel so that all > > bytes are zero initialized, but it is not documented as mandatory as far as > > I know. > 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
