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

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Chad Versace <chadversary@chromium.org>, dri-devel@lists.freedesktop.org
Subject: Re: Why does OUT_FENCE_PTR point not to an fd (s32) but to an s64?
Date: Thu, 12 Jan 2017 21:29:38 +0200	[thread overview]
Message-ID: <3347856.CHH5B2jR7L@avalon> (raw)
In-Reply-To: <20170112192408.43suy2bdaxjb4r7r@phenom.ffwll.local>

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.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel