2017-01-12 Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>: > Hi Daniel, > > On Thursday 12 Jan 2017 20:26:40 Daniel Vetter wrote: > > On Thu, Jan 12, 2017 at 05:17:26PM -0200, Gustavo Padovan wrote: > > > 2017-01-10 Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>: > > >> On Monday 09 Jan 2017 11:23:23 Daniel Vetter wrote: > > >>> On Fri, Jan 06, 2017 at 01:04:55PM -0800, Chad Versace wrote: > > >>>> Was this a mistake in the API? If so, can we fix this ABI mistake > > >>>> before kernel consumers rely on this? > > >>>> > > >>>> I naïvely expected that OUT_FENCE_PTR would be a pointer to, > > >>>> obviously, a fence fd (s32 __user *). But it's not. It's s64 __user > > >>>> *. Due to that surprise, I spent several hours chasing down weird > > >>>> corruption in Rob Clark's kmscube. The kernel unexpectedly cleared > > >>>> the 32 bits *above* an `int kms_fence_fd` in userspace. > > >>> > > >>> Never use unsized types for uabi. I guess we could have used s32, but > > >>> then someone is going to store this in a long and it goes boom on 64 > > >>> bit, > > >> > > >> Why so ? And why do we care ? The commonly accepted practice is to store > > >> file descriptors in int variables. s32 is an int on all platforms, so > > >> that's fine too. If we use a s32 pointer here, and someone decides to > > >> store it in a long, bool or cast it to a complex, that's their problem > > >> :-) > > > > > > The only thing that really needs to be s64 here is the OUT_FENCE_PTR > > > property in the Atomic interface because we carry a pointer there, but > > > all the manipulation after that is actually done after can easily be > > > done on s32 or int. > > > > > > We can't expect that userspace will know that we store as s64 and clear > > > the bits above if a int was passed down. if we use s32 we will be in > > > complaince with other linux apis that deals with fds. I'd say we fix > > > this before it can cause more damage out there. > > > > Changing uabi is kinda tricky, but it's still very new, so if we make sure > > it gets applied everywhere and doesn't accidentally ship we can it. Iirc > > fences are only in 4.10, so we should be fine ... > > Correct. That sounds good to me, there's still time for a v4.10-rc fix. I'd expect users defining an int and hitting the issue Chad hit instead of going to long types. So I hope we are safe here. I'll prepare a patch to make it s32. Gustavo _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
