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

[Gustavo Padovan <gustavo@padovan.org>]

2017-01-12 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:

> 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@ideasonboard.com>:
> > >> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel