Re: [PATCH] RFC: dma-buf: Add an API for importing and exporting sync files

[Jason Ekstrand]

On Wed, Feb 26, 2020 at 4:05 AM Daniel Vetter <daniel@xxxxxxxx> wrote:
>
> On Wed, Feb 26, 2020 at 10:16:05AM +0100, Christian KÃnig wrote:
> > Hi Jason,
> >
> > Am 26.02.20 um 00:58 schrieb Jason Ekstrand:
> > > Explicit synchronization is the future.  At least, that seems to be what
> > > most userspace APIs are agreeing on at this point.  However, most of our
> > > Linux APIs (both userspace and kernel UAPI) are currently built around
> > > implicit synchronization with dma-buf.  While work is ongoing to change
> > > many of the userspace APIs and protocols to an explicit synchronization
> > > model, switching over piecemeal is difficult due to the number of
> > > potential components involved.  On the kernel side, many drivers use
> > > dma-buf including GPU (3D/compute), display, v4l, and others.  In
> > > userspace, we have X11, several Wayland compositors, 3D drivers, compute
> > > drivers (OpenCL etc.), media encode/decode, and the list goes on.
> > >
> > > This patch provides a path forward by allowing userspace to manually
> > > manage the fences attached to a dma-buf.  Alternatively, one can think
> > > of this as making dma-buf's implicit synchronization simply a carrier
> > > for an explicit fence.  This is accomplished by adding two IOCTLs to
> > > dma-buf for importing and exporting a sync file to/from the dma-buf.
> > > This way a userspace component which is uses explicit synchronization,
> > > such as a Vulkan driver, can manually set the write fence on a buffer
> > > before handing it off to an implicitly synchronized component such as a
> > > Wayland compositor or video encoder.  In this way, each of the different
> > > components can be upgraded to an explicit synchronization model one at a
> > > time as long as the userspace pieces connecting them are aware of it and
> > > import/export fences at the right times.
> > >
> > > There is a potential race condition with this API if userspace is not
> > > careful.  A typical use case for implicit synchronization is to wait for
> > > the dma-buf to be ready, use it, and then signal it for some other
> > > component.  Because a sync_file cannot be created until it is guaranteed
> > > to complete in finite time, userspace can only signal the dma-buf after
> > > it has already submitted the work which uses it to the kernel and has
> > > received a sync_file back.  There is no way to atomically submit a
> > > wait-use-signal operation.  This is not, however, really a problem with
> > > this API so much as it is a problem with explicit synchronization
> > > itself.  The way this is typically handled is to have very explicit
> > > ownership transfer points in the API or protocol which ensure that only
> > > one component is using it at any given time.  Both X11 (via the PRESENT
> > > extension) and Wayland provide such ownership transfer points via
> > > explicit present and idle messages.
> > >
> > > The decision was intentionally made in this patch to make the import and
> > > export operations IOCTLs on the dma-buf itself rather than as a DRM
> > > IOCTL.  This makes it the import/export operation universal across all
> > > components which use dma-buf including GPU, display, v4l, and others.
> > > It also means that a userspace component can do the import/export
> > > without access to the DRM fd which may be tricky to get in cases where
> > > the client communicates with DRM via a userspace API such as OpenGL or
> > > Vulkan.  At a future date we may choose to add direct import/export APIs
> > > to components such as drm_syncobj to avoid allocating a file descriptor
> > > and going through two ioctls.  However, that seems to be something of a
> > > micro-optimization as import/export operations are likely to happen at a
> > > rate of a few per frame of rendered or decoded video.
> > >
> > > Signed-off-by: Jason Ekstrand <jason@xxxxxxxxxxxxxx>
> > > ---
> > >
> > > This is marked as an RFC because I intend it to start a discussion about
> > > how to solve a problem.  The current patch compiles but that's it for now.
> > > I'll be writing IGT tests and Vulkan driver patches which exercise it over
> > > the next couple of days.  In the mean time, feel free to tell me why you
> > > think this is a great and/or terrible idea. :-)
> >
> > For the exporting part I think it is an absolutely great idea because it
> > simplifies compatibility with explicit sync quite a bit.

Unfortunately, it only helps half of explicit sync and not the half
that's hard to deal with from Vulkan. :-/

> > But for the importing part it is a clear NAK at the moment. See we can't
> > allow userspace to mess with DMA-buf fences in that way because it rips open
> > a security hole you can push an elephant through.

Oh, sure, I'm 100% sure I did that part wrong.  Why else would I send
the patch but to have someone who actually knows what they're doing
tell me how to do it correctly? :-P

> > Just imagine that you access some DMA-buf with a shader and that operation
> > is presented as a fence on the DMA-bufs reservation object. And now you can
> > go ahead and replace that fence and free up the memory.
> >
> > Tricking the Linux kernel into allocating page tables in that freed memory
> > is trivial and that's basically it you can overwrite page tables with your
> > shader and gain access to all of system memory :)
> >
> > What we could do is to always make sure that the added fences will complete
> > later than the already existing ones, but that is also rather tricky to get
> > right. I wouldn't do that if we don't have a rather big use case for this.

Right.  I thought about that but I'm still learning how dma_resv
works.  It'd be easy enough to make a fence array that contains both
the old fence and the new fence and replace the old fence with that.
What I don't know is the proper way to replace the exclusive fence
safely.  Some sort of atomic_cpxchg loop, perhaps?  I presume there's
some way of doing it properly because DRM drivers are doing it all the
time.

> I think the main use-case for adding a fence is adding a write fence for
> vk winsys buffers, which run without any sync at all. So essentially what
> we'd do is promote one of the read fences which are already attached to be
> the write fence.

Correct.  We're effectively doing an import in ANV today but we're
doing it with a dummy execbuf which claims to write the BO and has a
batch that's just MI_BATCH_BUFFER_END.

> But yeah making sure we don't break any of the dma_resv guarantees about
> how these fences works is going to be somewhat tricky. Probably can reuse
> a big chunk of the fence container work we've done for syncobj timelines,
> since they have some of the same issues of having to chain fences to not
> break the world.

Happy to not break the world.  I just don't know how yet. :-)

--Jason