On 3/3/26 16:21, Julian Orth wrote: > 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.
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
