linux-kernel - Re: [PATCH v3 3/4] usb: gadget: functionfs: Add DMABUF import interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 09 Jan 2024 12:06:58 +0100
From: Paul Cercueil <paul@...pouillou.net>
To: Daniel Vetter <daniel@...ll.ch>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Sumit Semwal
 <sumit.semwal@...aro.org>, Christian König
 <christian.koenig@....com>, Jonathan Corbet <corbet@....net>, Michael
 Hennerich <Michael.Hennerich@...log.com>, linux-doc@...r.kernel.org, 
 linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org, 
 dri-devel@...ts.freedesktop.org, Andrzej Pietrasiewicz
 <andrzej.p@...labora.com>,  linaro-mm-sig@...ts.linaro.org, Nuno
 Sá <noname.nuno@...il.com>, Jonathan Cameron
 <jic23@...nel.org>,  linux-media@...r.kernel.org
Subject: Re: [PATCH v3 3/4] usb: gadget: functionfs: Add DMABUF import
 interface

Hi Daniel / Sima,

Le lundi 08 janvier 2024 à 20:19 +0100, Daniel Vetter a écrit :
> On Mon, Jan 08, 2024 at 05:27:33PM +0100, Paul Cercueil wrote:
> > Le lundi 08 janvier 2024 à 16:29 +0100, Daniel Vetter a écrit :
> > > On Mon, Jan 08, 2024 at 03:21:21PM +0100, Paul Cercueil wrote:
> > > > Hi Daniel (Sima?),
> > > > 
> > > > Le lundi 08 janvier 2024 à 13:39 +0100, Daniel Vetter a écrit :
> > > > > On Mon, Jan 08, 2024 at 01:00:55PM +0100, Paul Cercueil
> > > > > wrote:
> > > > > > +static void ffs_dmabuf_signal_done(struct ffs_dma_fence
> > > > > > *dma_fence, int ret)
> > > > > > +{
> > > > > > +	struct ffs_dmabuf_priv *priv = dma_fence->priv;
> > > > > > +	struct dma_fence *fence = &dma_fence->base;
> > > > > > +
> > > > > > +	dma_fence_get(fence);
> > > > > > +	fence->error = ret;
> > > > > > +	dma_fence_signal(fence);
> > > > > > +
> > > > > > +	dma_buf_unmap_attachment(priv->attach, dma_fence-
> > > > > > >sgt,
> > > > > > dma_fence->dir);
> > > > > > +	dma_fence_put(fence);
> > > > > > +	ffs_dmabuf_put(priv->attach);
> > > > > 
> > > > > So this can in theory take the dma_resv lock, and if the usb
> > > > > completion
> > > > > isn't an unlimited worker this could hold up completion of
> > > > > future
> > > > > dma_fence, resulting in a deadlock.
> > > > > 
> > > > > Needs to be checked how usb works, and if stalling
> > > > > indefinitely
> > > > > in
> > > > > the
> > > > > io_complete callback can hold up the usb stack you need to:
> > > > > 
> > > > > - drop a dma_fence_begin/end_signalling annotations in here
> > > > > - pull out the unref stuff into a separate preallocated
> > > > > worker
> > > > > (or at
> > > > >   least the final unrefs for ffs_dma_buf).
> > > > 
> > > > Only ffs_dmabuf_put() can attempt to take the dma_resv and
> > > > would
> > > > have
> > > > to be in a worker, right? Everything else would be inside the
> > > > dma_fence_begin/end_signalling() annotations?
> > > 
> > > Yup. Also I noticed that unlike the iio patches you don't have
> > > the
> > > dma_buf_unmap here in the completion path (or I'm blind?), which
> > > helps a
> > > lot with avoiding trouble.
> > 
> > They both call dma_buf_unmap_attachment() in the "signal done"
> > callback, the only difference I see is that it is called after the
> > dma_fence_put() in the iio patches, while it's called before
> > dma_fence_put() here.
> 
> I was indeed blind ...
> 
> So the trouble is this wont work because:
> - dma_buf_unmap_attachment() requires dma_resv_lock. This is a
> somewhat
>   recent-ish change from 47e982d5195d ("dma-buf: Move
>   dma_buf_map_attachment() to dynamic locking specification"), so
> maybe
>   old kernel or you don't have full lockdep enabled to get the right
>   splat.
> 
> - dma_fence critical section forbids dma_resv_lock
> 
> Which means you need to move this out, but then there's the potential
> cache management issue. Which current gpu drivers just kinda ignore
> because it doesn't matter for current use-case, they all cache the
> mapping
> for about as long as the attachment exists. You might want to do the
> same,
> unless that somehow breaks a use-case you have, I have no idea about
> that.
> If something breaks with unmap_attachment moved out of the fence
> handling
> then I guess it's high time to add separate cache-management only to
> dma_buf (and that's probably going to be quite some wiring up, not
> sure
> even how easy that would be to do nor what exactly the interface
> should
> look like).

Ok. Then I'll just cache the mapping for now, I think.

> Cheers, Sima

Cheers,
-Paul