Why does OUT_FENCE_PTR point not to an fd (s32) but to an s64?

Daniel Vetter daniel at ffwll.ch
Thu Jan 12 19:26:40 UTC 2017


On Thu, Jan 12, 2017 at 05:17:26PM -0200, Gustavo Padovan wrote:
> 2017-01-10 Laurent Pinchart <laurent.pinchart at ideasonboard.com>:
> 
> > Hi Daniel,
> > 
> > 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 ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch


More information about the dri-devel mailing list