Re: [RFC PATCH] ALSA: core: Add DMA share buffer support

[Takashi Iwai]

On Fri, 25 Jan 2019 12:11:43 +0100,
Baolin Wang wrote:
> 
> Hi Takashi,
> On Fri, 25 Jan 2019 at 18:10, Takashi Iwai <tiwai@xxxxxxx> wrote:
> >
> > On Fri, 25 Jan 2019 10:25:37 +0100,
> > Baolin Wang wrote:
> > >
> > > Hi Jaroslav,
> > > On Thu, 24 Jan 2019 at 21:43, Jaroslav Kysela <perex@xxxxxxxx> wrote:
> > > >
> > > > Dne 23.1.2019 v 13:46 Leo Yan napsal(a):
> > > > > Hi all,
> > > > >
> > > > > On Wed, Jan 23, 2019 at 12:58:51PM +0100, Takashi Iwai wrote:
> > > > >> On Tue, 22 Jan 2019 21:25:35 +0100,
> > > > >> Mark Brown wrote:
> > > > >>>
> > > > >>> On Mon, Jan 21, 2019 at 03:15:43PM +0100, Jaroslav Kysela wrote:
> > > > >>>> Dne 21.1.2019 v 13:40 Mark Brown napsal(a):
> > > > >>>
> > > > >>>>> It was the bit about adding more extended permission control that I was
> > > > >>>>> worried about there, not the initial O_APPEND bit.  Indeed the O_APPEND
> > > > >>>>> bit sounds like it might also work from the base buffer sharing point of
> > > > >>>>> view, I have to confess I'd not heard of that feature before (it didn't
> > > > >>>>> come up in the discussion when Eric raised this in Prague).
> > > > >>>
> > > > >>>> With permissions, I meant to make possible to restrict the file
> > > > >>>> descriptor operations (ioctls) for the depending task (like access to
> > > > >>>> the DMA buffer, synchronize it for the non-coherent platforms and maybe
> > > > >>>> read/write the actual position, delay etc.). It should be relatively
> > > > >>>> easy to implement using the snd_pcm_file structure.
> > > > >>>
> > > > >>> Right, that's what I understood you to mean.  If you want to have a
> > > > >>> policy saying "it's OK to export a PCM file descriptor if it's only got
> > > > >>> permissions X and Y" the security module is going to need to know about
> > > > >>> the mechanism for setting those permissions.  With dma_buf that's all a
> > > > >>> bit easier as there's less new stuff, though I've no real idea how much
> > > > >>> of a big deal that actually is.
> > > > >>
> > > > >> There are many ways to implement such a thing, yeah.  If we'd need an
> > > > >> implementation that is done solely in the sound driver layer, I can
> > > > >> imagine to introduce either a new ioctl or an open flag (like O_EXCL)
> > > > >> to specify the restricted sharing.  That is, a kind of master / slave
> > > > >> model where only the master is allowed to manipulate the stream while
> > > > >> the slave can mmap, read/write and get status.
> > > > >
> > > > > In order to support EXCLUSIVE mode, it is necessary to convert the
> > > > > /dev/snd/ descriptor to an anon_inode:dmabuffer file descriptor.
> > > > > SELinux allows that file descriptor to be passed to the client. It can
> > > > > also be used by the AAudioService.
> > > >
> > > > Okay, so this is probably the only point which we should resolve for the
> > > > already available DMA buffer sharing in ALSA (the O_APPEND flag).
> > > >
> > > > I had another glance to your dma-buf implementation and I see many
> > > > things which might cause problems:
> > > >
> > > > - allow to call dma-buf ioctls only when the audio device is in specific
> > > > state (stream is not running)
> > >
> > > Right. Will fix.
> > >
> > > > - as Takashi mentioned, if we return another file-descriptor (dma-buf
> > > > export) to the user space and the server closes the main pcm
> > > > file-descriptor (the client does not) - the result will be a crash (dma
> > > > buffer will be freed, but referenced through the dma-buf interface)
> > >
> > > Yes, will fix.
> >
> > There are a few more overlooked problems.  A part of them was already
> > mentioned in my previous reply, but let me repeat:
> >
> > - The racy ioctls have to be considered; you can perform this export
> >   ioctl concurrently, and both of them write and mix up the setup,
> >   which is obviously broken.
> 
> Yes, I think I missed the snd_pcm_stream_lock, and will add.

Beware that it's not so trivial.  The stream lock is usually
spinlock.

In addition, we need to be careful about the PCM state, as Jaroslav
mentioned.  Basically SNDRV_PCM_STATE_SETUP is already too late for
attaching the buffer, since another buffer is already assigned to the
stream.  Similarly, after detaching, the stream state must go to
SNDRV_PCM_STATE_OPEN.

> > - What happens to the PCM buffer that has been allocated before
> >   attaching, if it's not the pre-allocated one?
> >   It should be released properly beforehand, otherwise leaks.
> 
> I am not sure I understood you correctly. If the PCM buffer has been
> allocated, the platform driver should handle it? Since we always use
> substream->dma_buffer.

substream->dma_buffer is merely a pre-allocated buffer, and not every
driver sets it up.  The actual PCM buffer is found in substream's
runtime, and this implementation isn't always with the preallocated
buffer.  It can even be a fixed IO-mapped buffer.

> > - There is no validation of the attached dma-buf pages; most drivers
> >   set coherent DMA mask, and they rely on it.  e.g. if a page over the
> >   DMA mask is passed, it will break silently.
> 
> Sorry maybe I did not get your point here. We have validate the
> dma_map_sg_attr() funtion, in this fucntion it will validate the DMA
> mask by dma_capable().

OK, then this should be fine -- at least about DMA mask.  But, how
about other setups, e.g. coherency?

Imagine that a buffer allocated for chip A is exported to another chip
B.  If chip B requires some special setup of the pages while A isn't,
this won't work.  For example, some HD-audio chips require the
non-cached pages while some HD-audio chips allow normal pages.


thanks,

Takashi