Re: [alsa-devel] [RFC PATCH] ALSA: core: Add DMA share buffer support
On Mon, 28 Jan 2019 14:31:23 +0100,
Jaroslav Kysela wrote:
>
> Dne 25.1.2019 v 19:25 Mark Brown napsal(a):
> > On Fri, Jan 25, 2019 at 02:19:22PM +0100, Takashi Iwai wrote:
> >> Leo Yan wrote:
> >
> >>> If we directly use the device node /dev/snd/ as file descriptor, even
> >>> though we specify flag O_EXCL when open it, but it still is not an
> >>> anon inode file descriptor. Thus this is not safe enough and will be
> >>> blocked by SELinux. On the other hand, this patch wants to use
> >>> dma-buf framework to provide file descriptor for the audio buffer, and
> >>> this audio buffer can be one of mutiple audio buffers in the system
> >>> and it can be shared to any audio client program.
> >
> >> Hrm, it sounds like a workaround just to bypass SELinux check...
> >
> >> The sound server can open another PCM stream with O_APPEND, and pass
> >> that fd to the client, too?
> >
> > So long as we can teach SELinux that they're safe to export, yeah.
>
> It seems that SELinux works with the file, so the SELinux will block the
> fd pass, because the file descriptor (through standard dup()) continues
> to use the /dev/snd inode.
>
> I would propose to implement a dup ioctl to return a new
> anon_inode:snd-pcm file descriptor (see bellow).
I like the idea. This would work around the messy issues gracefully,
and more importantly, it's easier to maintain for us.
And the restriction of ioctls for anon dup should be fairly easy to
implement on top of this.
Thanks!
Takashi
> If we agree on this, I can propose the full solution.
>
> --
> Subject: [PATCH] ALSA: pcm: implement the anonymous dup (inode file
> descriptor)
>
> This patch implements new SNDRV_PCM_IOCTL_ANONYMOUS_DUP ioctl which
> returns the new duplicated anonymous inode file descriptor
> (anon_inode:snd-pcm) which can be passed to the restricted clients.
>
> This implementation is just a concept for comments - it does not contain
> the additional restriction control.
>
> TODO: The clients might be restricted to disallow a set of
> controls (ioctls) for the audio stream.
>
> This patch is meant to be the alternative for the dma-buf interface. Both
> implementation have some pros and cons:
>
> anon_inode:dmabuf
>
> - a bit standard export API for the DMA buffers
> - fencing for the concurrent access [1]
> - driver/kernel interface for the DMA buffer [1]
> - multiple attach/detach scheme [1]
>
> [1] the real usage for the sound PCM is unknown at the moment for this feature
>
> anon_inode:snd-pcm
>
> - simple (no problem with ref-counting, non-standard mmap implementation etc.)
> - allow to use more sound interfaces for the file descriptor like status
> ioctls
> - more fine grained security policies (another anon_inode name unshared with
> other drivers)
> ---
> include/uapi/sound/asound.h | 1 +
> sound/core/pcm_compat.c | 1 +
> sound/core/pcm_native.c | 40
> 3 files changed, 42 insertions(+)
>
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 404d4b9ffe76..ad821a52f970 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -576,6 +576,7 @@ enum {
> #define SNDRV_PCM_IOCTL_TSTAMP _IOW('A', 0x02, int)
> #define SNDRV_PCM_IOCTL_TTSTAMP _IOW('A', 0x03, int)
> #define SNDRV_PCM_IOCTL_USER_PVERSION_IOW('A', 0x04, int)
> +#define SNDRV_PCM_IOCTL_ANONYMOUS_DUP _IOW('A', 0x05, int)
> #define SNDRV_PCM_IOCTL_HW_REFINE_IOWR('A', 0x10, struct
> snd_pcm_hw_params)
> #define SNDRV_PCM_IOCTL_HW_PARAMS_IOWR('A', 0x11, struct
> snd_pcm_hw_params)
> #define SNDRV_PCM_IOCTL_HW_FREE _IO('A', 0x12)
> diff --git a/sound/core/pcm_compat.c b/sound/core/pcm_compat.c
> index 946ab080ac00..22446cd574ee 100644
> --- a/sound/core/pcm_compat.c
> +++ b/sound/core/pcm_compat.c
> @@ -675,6 +675,7 @@ static long snd_pcm_ioctl_compat(struct file *file,
> unsigned int cmd, unsigned l
> case SNDRV_PCM_IOCTL_TSTAMP:
> case SNDRV_PCM_IOCTL_TTSTAMP:
> case SNDRV_PCM_IOCTL_USER_PVERSION:
> + case SNDRV_PCM_IOCTL_ANONYMOUS_DUP:
> case SNDRV_PCM_IOCTL_HWSYNC:
> case SNDRV_PCM_IOCTL_PREPARE:
> case SNDRV_PCM_IOCTL_RESET:
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 26afb6b0889a..a21bb482b4b0 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -37,6 +37,8 @@
> #include
> #include
> #include
> +#include
> +#include
>
> #include "pcm_local.h"
>
> @@ -2836,6 +2838,42 @@ static int snd_pcm_forward_ioctl(struct
> snd_pcm_substream *substream,
> return result < 0 ? result : 0;
> }
>
> +static int snd_pcm_anonymous_dup(struct file *file,
> + struct snd_pcm_substream *substream,
> + int __user *arg)
> +{
> + int fd;
> + int res;
> + int dup_mode;
> + int flags;
> + struct file *nfile;
> + struct snd_pcm_substream
Re: [alsa-devel] [RFC PATCH] ALSA: core: Add DMA share buffer support
Dne 25.1.2019 v 19:25 Mark Brown napsal(a):
> On Fri, Jan 25, 2019 at 02:19:22PM +0100, Takashi Iwai wrote:
>> Leo Yan wrote:
>
>>> If we directly use the device node /dev/snd/ as file descriptor, even
>>> though we specify flag O_EXCL when open it, but it still is not an
>>> anon inode file descriptor. Thus this is not safe enough and will be
>>> blocked by SELinux. On the other hand, this patch wants to use
>>> dma-buf framework to provide file descriptor for the audio buffer, and
>>> this audio buffer can be one of mutiple audio buffers in the system
>>> and it can be shared to any audio client program.
>
>> Hrm, it sounds like a workaround just to bypass SELinux check...
>
>> The sound server can open another PCM stream with O_APPEND, and pass
>> that fd to the client, too?
>
> So long as we can teach SELinux that they're safe to export, yeah.
It seems that SELinux works with the file, so the SELinux will block the
fd pass, because the file descriptor (through standard dup()) continues
to use the /dev/snd inode.
I would propose to implement a dup ioctl to return a new
anon_inode:snd-pcm file descriptor (see bellow).
If we agree on this, I can propose the full solution.
--
Subject: [PATCH] ALSA: pcm: implement the anonymous dup (inode file
descriptor)
This patch implements new SNDRV_PCM_IOCTL_ANONYMOUS_DUP ioctl which
returns the new duplicated anonymous inode file descriptor
(anon_inode:snd-pcm) which can be passed to the restricted clients.
This implementation is just a concept for comments - it does not contain
the additional restriction control.
TODO: The clients might be restricted to disallow a set of
controls (ioctls) for the audio stream.
This patch is meant to be the alternative for the dma-buf interface. Both
implementation have some pros and cons:
anon_inode:dmabuf
- a bit standard export API for the DMA buffers
- fencing for the concurrent access [1]
- driver/kernel interface for the DMA buffer [1]
- multiple attach/detach scheme [1]
[1] the real usage for the sound PCM is unknown at the moment for this feature
anon_inode:snd-pcm
- simple (no problem with ref-counting, non-standard mmap implementation etc.)
- allow to use more sound interfaces for the file descriptor like status ioctls
- more fine grained security policies (another anon_inode name unshared with
other drivers)
---
include/uapi/sound/asound.h | 1 +
sound/core/pcm_compat.c | 1 +
sound/core/pcm_native.c | 40
3 files changed, 42 insertions(+)
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 404d4b9ffe76..ad821a52f970 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -576,6 +576,7 @@ enum {
#define SNDRV_PCM_IOCTL_TSTAMP _IOW('A', 0x02, int)
#define SNDRV_PCM_IOCTL_TTSTAMP_IOW('A', 0x03, int)
#define SNDRV_PCM_IOCTL_USER_PVERSION _IOW('A', 0x04, int)
+#define SNDRV_PCM_IOCTL_ANONYMOUS_DUP _IOW('A', 0x05, int)
#define SNDRV_PCM_IOCTL_HW_REFINE _IOWR('A', 0x10, struct
snd_pcm_hw_params)
#define SNDRV_PCM_IOCTL_HW_PARAMS _IOWR('A', 0x11, struct
snd_pcm_hw_params)
#define SNDRV_PCM_IOCTL_HW_FREE_IO('A', 0x12)
diff --git a/sound/core/pcm_compat.c b/sound/core/pcm_compat.c
index 946ab080ac00..22446cd574ee 100644
--- a/sound/core/pcm_compat.c
+++ b/sound/core/pcm_compat.c
@@ -675,6 +675,7 @@ static long snd_pcm_ioctl_compat(struct file *file,
unsigned int cmd, unsigned l
case SNDRV_PCM_IOCTL_TSTAMP:
case SNDRV_PCM_IOCTL_TTSTAMP:
case SNDRV_PCM_IOCTL_USER_PVERSION:
+ case SNDRV_PCM_IOCTL_ANONYMOUS_DUP:
case SNDRV_PCM_IOCTL_HWSYNC:
case SNDRV_PCM_IOCTL_PREPARE:
case SNDRV_PCM_IOCTL_RESET:
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 26afb6b0889a..a21bb482b4b0 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -37,6 +37,8 @@
#include
#include
#include
+#include
+#include
#include "pcm_local.h"
@@ -2836,6 +2838,42 @@ static int snd_pcm_forward_ioctl(struct
snd_pcm_substream *substream,
return result < 0 ? result : 0;
}
+static int snd_pcm_anonymous_dup(struct file *file,
+struct snd_pcm_substream *substream,
+int __user *arg)
+{
+ int fd;
+ int res;
+ int dup_mode;
+ int flags;
+ struct file *nfile;
+ struct snd_pcm_substream *rsubstream;
+
+ if (get_user(dup_mode, (int __user *)arg))
+ return -EFAULT;
+ if (dup_mode != 0)
+ return -ENOSYS;
+ flags = file->f_flags & (O_RDWR|O_NONBLOCK);
+ flags |= O_APPEND | O_CLOEXEC;
+ fd = get_unused_fd_flags(flags);
+ if (fd < 0)
+ return fd;
+ nfile = anon_inode_getfile("snd-pcm", file->f_op, NULL, flags);
+ if (IS_ERR(nfile)) {
+ put_unused_fd(fd);
+ return PTR_ERR(nfile);
+
Re: [RFC PATCH] ALSA: core: Add DMA share buffer support
On Fri, 25 Jan 2019 at 21:04, Takashi Iwai wrote: > > > > > > Erm, obviously it's not enough. Each attach / detach needs to manage > > > the refcount, too, for covering the cases above. It can re-use the > > > PCM mmap_refount, though. > > > > But we've used the DMA buffer file's refcounting to manage the DMA > > buffer. So is this not enough? > > Unless you manage the PCM substream refcount (or block the state > change), the PCM stream itself can be released (or re-setup) freely. > OK. Thanks for your comments. -- Baolin Wang Best Regards
Re: [RFC PATCH] ALSA: core: Add DMA share buffer support
On Fri, 25 Jan 2019 at 21:03, Takashi Iwai wrote: > > On Fri, 25 Jan 2019 12:11:43 +0100, > Baolin Wang wrote: > > > > Hi Takashi, > > On Fri, 25 Jan 2019 at 18:10, Takashi Iwai 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 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. Right. Thanks for your reminding. > > 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. Make sense. > > > > - 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,
Re: [alsa-devel] [RFC PATCH] ALSA: core: Add DMA share buffer support
On Fri, Jan 25, 2019 at 02:19:22PM +0100, Takashi Iwai wrote: > Leo Yan wrote: > > If we directly use the device node /dev/snd/ as file descriptor, even > > though we specify flag O_EXCL when open it, but it still is not an > > anon inode file descriptor. Thus this is not safe enough and will be > > blocked by SELinux. On the other hand, this patch wants to use > > dma-buf framework to provide file descriptor for the audio buffer, and > > this audio buffer can be one of mutiple audio buffers in the system > > and it can be shared to any audio client program. > Hrm, it sounds like a workaround just to bypass SELinux check... > The sound server can open another PCM stream with O_APPEND, and pass > that fd to the client, too? So long as we can teach SELinux that they're safe to export, yeah. signature.asc Description: PGP signature
Re: [alsa-devel] [RFC PATCH] ALSA: core: Add DMA share buffer support
On Wed, 23 Jan 2019 13:46:58 +0100, Leo Yan wrote: > > 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. > > I am lacking security related knowledge, especially for SELinux. > So only can give background information but not sure if it's really > helpful for discussion. > > Android web page [1] give some information for this: > > "The shared memory is referenced using a file descriptor that is > generated by the ALSA driver. If the file descriptor is directly > associated with a /dev/snd/ driver file, then it can be used by the > AAudio service in SHARED mode. But the descriptor cannot be passed to > the client code for EXCLUSIVE mode. The /dev/snd/ file descriptor > would provide too broad of access to the client, so it is blocked by > SELinux. > > 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. > > An anon_inode:dmabuffer file descriptor can be generated using the > Android Ion memory library." > > So we work out dmabuf driver for audio buffer, the audio buffer will > be exported and attached by using dma-buf framework; then we can > return one file descriptor which is generated by dma-buf and this > file descriptor is bound with anon inode based on dma-buf core code. > > If we directly use the device node /dev/snd/ as file descriptor, even > though we specify flag O_EXCL when open it, but it still is not an > anon inode file descriptor. Thus this is not safe enough and will be > blocked by SELinux. On the other hand, this patch wants to use > dma-buf framework to provide file descriptor for the audio buffer, and > this audio buffer can be one of mutiple audio buffers in the system > and it can be shared to any audio client program. > > Again, I have no less knowledge for SELinux so sorry if I introduce any > noise at here. And very appreciate any comments for this. Hrm, it sounds like a workaround just to bypass SELinux check... The sound server can open another PCM stream with O_APPEND, and pass that fd to the client, too? thanks, Takashi
Re: [RFC PATCH] ALSA: core: Add DMA share buffer support
On Fri, 25 Jan 2019 12:24:29 +0100, Baolin Wang wrote: > > On Fri, 25 Jan 2019 at 18:20, Takashi Iwai wrote: > > > > On Fri, 25 Jan 2019 11:10:25 +0100, > > Takashi Iwai 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 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. > > > > > > - The PCM buffer can be re-allocated on the fly. If the current > > > buffer is abandoned while exporting, it leads to the UAF. > > > > > > - Similarly, what if the PCM stream that is attached is closed without > > > detaching itself? Or, what if the PCM stream attaches itself twice > > > without detaching? > > > > > > - The driver may provide its own mmap method, and you can't hard-code > > > the mmap implementation as currently in snd_pcm_dmabuf_mmap(). > > > > > > I suppose you can drop of most of the code in snd_pcm_dmabuf_map(), > > > instead, assign PCM substream in obj, and call snd_pcm_mmap_data() > > > with the given VMA. If this really works,
Re: [RFC PATCH] ALSA: core: Add DMA share buffer support
On Fri, 25 Jan 2019 12:11:43 +0100, Baolin Wang wrote: > > Hi Takashi, > On Fri, 25 Jan 2019 at 18:10, Takashi Iwai 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 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
Re: [RFC PATCH] ALSA: core: Add DMA share buffer support
On Fri, 25 Jan 2019 at 18:20, Takashi Iwai wrote: > > On Fri, 25 Jan 2019 11:10:25 +0100, > Takashi Iwai 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 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. > > > > - The PCM buffer can be re-allocated on the fly. If the current > > buffer is abandoned while exporting, it leads to the UAF. > > > > - Similarly, what if the PCM stream that is attached is closed without > > detaching itself? Or, what if the PCM stream attaches itself twice > > without detaching? > > > > - The driver may provide its own mmap method, and you can't hard-code > > the mmap implementation as currently in snd_pcm_dmabuf_mmap(). > > > > I suppose you can drop of most of the code in snd_pcm_dmabuf_map(), > > instead, assign PCM substream in obj, and call snd_pcm_mmap_data() > > with the given VMA. If this really works, it manages the mmap > > refcount, so the previous two issues should be covered there. > > But it needs more consideration... > > Erm, obviously it's not enough. Each attach / detach needs to manage > the refcount, too, for covering the cases above. It can re-use the > PCM mmap_refount, though. But we've used the DMA buffer file's refcounting to manage the DMA buffer.
Re: [RFC PATCH] ALSA: core: Add DMA share buffer support
Hi Takashi, On Fri, 25 Jan 2019 at 18:10, Takashi Iwai 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 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. > > - The PCM buffer can be re-allocated on the fly. If the current > buffer is abandoned while exporting, it leads to the UAF. So I need some validation to check if the buffer is available now when exporting it. > > - Similarly, what if the PCM stream that is attached is closed without > detaching itself? Or, what if the PCM stream attaches itself twice We will detach it automatically in snd_pcm_free_stream() > without detaching? Sure, need add validation for this case. > > - The driver may provide its own mmap method, and you can't hard-code > the mmap implementation as currently in snd_pcm_dmabuf_mmap(). > > I suppose you can drop of most of the code in snd_pcm_dmabuf_map(), > instead, assign PCM substream in obj, and call snd_pcm_mmap_data() > with the given VMA. If this really works, it manages the mmap > refcount, so the previous two issues should be covered there. > But it needs more consideration... Ah, I think I missed the snd_pcm_mmap_data() function. Yes, so I can remove the implementation in snd_pcm_dmabuf_map(). > > - What happens to the PCM buffer that has been allocated before > attaching, if it's not the pre-allocated one? > It should be released
Re: [RFC PATCH] ALSA: core: Add DMA share buffer support
On Fri, 25 Jan 2019 11:10:25 +0100, Takashi Iwai 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 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. > > - The PCM buffer can be re-allocated on the fly. If the current > buffer is abandoned while exporting, it leads to the UAF. > > - Similarly, what if the PCM stream that is attached is closed without > detaching itself? Or, what if the PCM stream attaches itself twice > without detaching? > > - The driver may provide its own mmap method, and you can't hard-code > the mmap implementation as currently in snd_pcm_dmabuf_mmap(). > > I suppose you can drop of most of the code in snd_pcm_dmabuf_map(), > instead, assign PCM substream in obj, and call snd_pcm_mmap_data() > with the given VMA. If this really works, it manages the mmap > refcount, so the previous two issues should be covered there. > But it needs more consideration... Erm, obviously it's not enough. Each attach / detach needs to manage the refcount, too, for covering the cases above. It can re-use the PCM mmap_refount, though. Takashi
Re: [RFC PATCH] ALSA: core: Add DMA share buffer support
On Fri, 25 Jan 2019 10:25:37 +0100, Baolin Wang wrote: > > Hi Jaroslav, > On Thu, 24 Jan 2019 at 21:43, Jaroslav Kysela 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. - The PCM buffer can be re-allocated on the fly. If the current buffer is abandoned while exporting, it leads to the UAF. - Similarly, what if the PCM stream that is attached is closed without detaching itself? Or, what if the PCM stream attaches itself twice without detaching? - The driver may provide its own mmap method, and you can't hard-code the mmap implementation as currently in snd_pcm_dmabuf_mmap(). I suppose you can drop of most of the code in snd_pcm_dmabuf_map(), instead, assign PCM substream in obj, and call snd_pcm_mmap_data() with the given VMA. If this really works, it manages the mmap refcount, so the previous two issues should be covered there. But it needs more consideration... - 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. - 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. - Some drivers don't use the standard memory pages but keep their own hardware buffer (e.g. rme96 or rm32 driver). This ioctl would be completely broken on such hardware. That is, we need some sanity check whether the PCM allows the arbitrary dma-buf or not. thanks, Takashi
Re: [RFC PATCH] ALSA: core: Add DMA share buffer support
Hi Jaroslav, On Thu, 24 Jan 2019 at 21:43, Jaroslav Kysela 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. > > - the attach function calls dma_buf_get(fd), but what if fd points to > another dma-buf allocation from a different driver? the unexpected > private data will cause crash - there should be a type checking in the > dma-buf interface There is a validation (is_dma_buf_file() ) in dma_buf_get() function before getting the dma buffer. > If I look to the dma_buf_fd() implementation: > > fd = get_unused_fd_flags(flags); > fd_install(fd, dmabuf->file); > > .. what if we just add one new ioctl to the ALSA's PCM API which will > return a new anonymous inode descriptor with the restricted access to > the main PCM device to satisfy the SELinux requirements / security > policies? It might be more nice and simple solution than to implement > the full dma-buf interface for the ALSA's PCM devices. I will do some investigation for your suggestion and talk with the security people if it can work. Thanks for your suggestion. -- Baolin Wang Best Regards
Re: [RFC PATCH] ALSA: core: Add DMA share buffer support
On Thu, Jan 24, 2019 at 02:43:02PM +0100, Jaroslav Kysela wrote: > If I look to the dma_buf_fd() implementation: > fd = get_unused_fd_flags(flags); > fd_install(fd, dmabuf->file); > .. what if we just add one new ioctl to the ALSA's PCM API which will > return a new anonymous inode descriptor with the restricted access to > the main PCM device to satisfy the SELinux requirements / security > policies? It might be more nice and simple solution than to implement > the full dma-buf interface for the ALSA's PCM devices. That certainly works for me so long as the security people are happy. > Question: The dma-buf also implements the fencing, but I am not able to > determine, if this mechanism is used in android [1]. It may allow > concurrent mmap and synchronize apps - but the sound server should > manage the access to the DMA buffer anyway. In my opinion, it makes much > sense for the video-pipes when the hardware does some accelerations > (encoding/decoding). We had the same discuission off list and couldn't think of a need for that feature in the audio context but left it in as it's already there with dma-buf so there's no real cost to implementing it and we weren't sure we weren't missing something. signature.asc Description: PGP signature
Re: [RFC PATCH] ALSA: core: Add DMA share buffer support
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) - 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) - the attach function calls dma_buf_get(fd), but what if fd points to another dma-buf allocation from a different driver? the unexpected private data will cause crash - there should be a type checking in the dma-buf interface If I look to the dma_buf_fd() implementation: fd = get_unused_fd_flags(flags); fd_install(fd, dmabuf->file); .. what if we just add one new ioctl to the ALSA's PCM API which will return a new anonymous inode descriptor with the restricted access to the main PCM device to satisfy the SELinux requirements / security policies? It might be more nice and simple solution than to implement the full dma-buf interface for the ALSA's PCM devices. Question: The dma-buf also implements the fencing, but I am not able to determine, if this mechanism is used in android [1]. It may allow concurrent mmap and synchronize apps - but the sound server should manage the access to the DMA buffer anyway. In my opinion, it makes much sense for the video-pipes when the hardware does some accelerations (encoding/decoding). Jaroslav > [1] https://source.android.com/devices/audio/aaudio -- Jaroslav Kysela Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
Re: [RFC PATCH] ALSA: core: Add DMA share buffer support
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. I am lacking security related knowledge, especially for SELinux. So only can give background information but not sure if it's really helpful for discussion. Android web page [1] give some information for this: "The shared memory is referenced using a file descriptor that is generated by the ALSA driver. If the file descriptor is directly associated with a /dev/snd/ driver file, then it can be used by the AAudio service in SHARED mode. But the descriptor cannot be passed to the client code for EXCLUSIVE mode. The /dev/snd/ file descriptor would provide too broad of access to the client, so it is blocked by SELinux. 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. An anon_inode:dmabuffer file descriptor can be generated using the Android Ion memory library." So we work out dmabuf driver for audio buffer, the audio buffer will be exported and attached by using dma-buf framework; then we can return one file descriptor which is generated by dma-buf and this file descriptor is bound with anon inode based on dma-buf core code. If we directly use the device node /dev/snd/ as file descriptor, even though we specify flag O_EXCL when open it, but it still is not an anon inode file descriptor. Thus this is not safe enough and will be blocked by SELinux. On the other hand, this patch wants to use dma-buf framework to provide file descriptor for the audio buffer, and this audio buffer can be one of mutiple audio buffers in the system and it can be shared to any audio client program. Again, I have no less knowledge for SELinux so sorry if I introduce any noise at here. And very appreciate any comments for this. Thanks, Leo Yan [1] https://source.android.com/devices/audio/aaudio
Re: [RFC PATCH] ALSA: core: Add DMA share buffer support
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. thanks, Takashi
Re: [RFC PATCH] ALSA: core: Add DMA share buffer support
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. > Even with the full operation mode, it's difficult imagine what the > depending task can break with the sound interface except probably annoy > users with some cracks for the playback or start/stop the streaming rapidly. In some embedded systems the ability to play back notifications clearly can be very important to system functionality. Obviously at times the main purpose of the system is to play or record audio. I don't have a *super* strong opinion here but I'm not really a security person. signature.asc Description: PGP signature
Re: [RFC PATCH] ALSA: core: Add DMA share buffer support
Dne 21.1.2019 v 13:40 Mark Brown napsal(a): > On Fri, Jan 18, 2019 at 08:39:32PM +0100, Takashi Iwai wrote: >> Mark Brown wrote: > multiple tasks). I would probably go in this way and add more extended permission control for the PCM device, so permissions can be restricted for the passed descriptor to the producer or the consumer task. In this > >>> One concern I have with doing some ALSA-specific custom permissions >>> thing is integration with frameworks like SELinux (they'd presumably >>> need to learn about the ALSA specific stuff to manage it). It also > >> Well, I wonder what makes it more difficult by the approach Jaroslav >> suggested. With O_APPEND, you can just call mmap() normally, and >> that's all. What's the merit of dma-buf approach wrt the security? > > 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. Even with the full operation mode, it's difficult imagine what the depending task can break with the sound interface except probably annoy users with some cracks for the playback or start/stop the streaming rapidly. Jaroslav -- Jaroslav Kysela Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
Re: [RFC PATCH] ALSA: core: Add DMA share buffer support
On Fri, Jan 18, 2019 at 08:39:32PM +0100, Takashi Iwai wrote: > Mark Brown wrote: > > > multiple tasks). I would probably go in this way and add more extended > > > permission control for the PCM device, so permissions can be restricted > > > for the passed descriptor to the producer or the consumer task. In this > > One concern I have with doing some ALSA-specific custom permissions > > thing is integration with frameworks like SELinux (they'd presumably > > need to learn about the ALSA specific stuff to manage it). It also > Well, I wonder what makes it more difficult by the approach Jaroslav > suggested. With O_APPEND, you can just call mmap() normally, and > that's all. What's the merit of dma-buf approach wrt the security? 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). signature.asc Description: PGP signature
Re: [RFC PATCH] ALSA: core: Add DMA share buffer support
On Fri, 18 Jan 2019 20:08:05 +0100, Mark Brown wrote: > > On Fri, Jan 18, 2019 at 10:35:44AM +0100, Jaroslav Kysela wrote: > > > the tinyalsa implementation does not show much - it's equal to the > > standard mmap access for the PCM devices. Even considering the Mark's > > text, there must be an arbiter (sound server) which communicates with > > the producer or consumer to control the data flow. I really would like > > to see a real usage for this. > > Right, the driving force behind implementing this is Android which had > been using an out of tree version of this approach based on ION but > that's run into trouble due to other outside changes. > > > It seems to me that the only point to implement this is the > > permissions. We already have O_APPEND mode for the PCM file descriptor > > which can reuse the PCM device multiple times (mmap the buffer to > > multiple tasks). I would probably go in this way and add more extended > > permission control for the PCM device, so permissions can be restricted > > for the passed descriptor to the producer or the consumer task. In this > > way, the restricted task might reuse other control mechanism offered buy > > the PCM file descriptor without requesting the arbiter to do so (like > > read the actual position in the DMA buffer, get the audio delay or so - > > reduce context switches). > > One concern I have with doing some ALSA-specific custom permissions > thing is integration with frameworks like SELinux (they'd presumably > need to learn about the ALSA specific stuff to manage it). It also > seems like it's adding a lot more security sensitive interfaces and > code which which will require audit and review, one of the things I > really like about this approach is that it's incredibly simple from > the security point of view. Well, I wonder what makes it more difficult by the approach Jaroslav suggested. With O_APPEND, you can just call mmap() normally, and that's all. What's the merit of dma-buf approach wrt the security? BTW, the suggested patch seems to have a problem when the attached PCM performs hw_free. Then the mapped target will be gone while another process still mapping it. And the code looks pretty racy. thanks, Takashi
Re: [RFC PATCH] ALSA: core: Add DMA share buffer support
On Fri, Jan 18, 2019 at 10:35:44AM +0100, Jaroslav Kysela wrote: > the tinyalsa implementation does not show much - it's equal to the > standard mmap access for the PCM devices. Even considering the Mark's > text, there must be an arbiter (sound server) which communicates with > the producer or consumer to control the data flow. I really would like > to see a real usage for this. Right, the driving force behind implementing this is Android which had been using an out of tree version of this approach based on ION but that's run into trouble due to other outside changes. > It seems to me that the only point to implement this is the > permissions. We already have O_APPEND mode for the PCM file descriptor > which can reuse the PCM device multiple times (mmap the buffer to > multiple tasks). I would probably go in this way and add more extended > permission control for the PCM device, so permissions can be restricted > for the passed descriptor to the producer or the consumer task. In this > way, the restricted task might reuse other control mechanism offered buy > the PCM file descriptor without requesting the arbiter to do so (like > read the actual position in the DMA buffer, get the audio delay or so - > reduce context switches). One concern I have with doing some ALSA-specific custom permissions thing is integration with frameworks like SELinux (they'd presumably need to learn about the ALSA specific stuff to manage it). It also seems like it's adding a lot more security sensitive interfaces and code which which will require audit and review, one of the things I really like about this approach is that it's incredibly simple from the security point of view. signature.asc Description: PGP signature
Re: [RFC PATCH] ALSA: core: Add DMA share buffer support
Dne 18.1.2019 v 05:55 Baolin Wang napsal(a): > This patch adds dma share buffer support, which allows a dma-buf to be shared > between processes by passing file descriptors between them, allowing multiple > processes to cooperate in filling the DMA buffer used by the audio hardware > without the need to copy data around. This reduces both latency and CPU load, > especially in configurations where only one application is playing audio and > so context switches can be avoided. > > In userspace, we can use ioctl:SNDRV_PCM_IOCTL_DMABUF_EXPORT to export one > dma buffer to a PCM, and use ioctl:SNDRV_PCM_IOCTL_DMABUF_ATTACH and > ioctl:SNDRV_PCM_IOCTL_DMABUF_DETACH to attach or detach one device to the > dma buffer. Morevoer we can use dma buffer ioctl:DMA_BUF_IOCTL_SYNC to > guarantee the cache coherency of the DMA buffer. > > You can refer to below patches [1] created by Leo Yan to use the dma buffer > in userspace. > > [1] https://github.com/tinyalsa/tinyalsa/pull/126 > > Welcome any comments. Thanks. > > Signed-off-by: Baolin Wang > --- > Hi, > > Before sending to ALSA mailing list, we had some internal discussion off line, > so I posted them to follow up. > > 1. One issue proposed by Srinivas Kandagatla, he proposed one use case > example: > DSP1 pre-process(compress-to-pcm) the buffers, pass the fd to DSP2/audio-ip > to play pcm data. The dmabuf exporter (DSP1) is a different device than > audio device in this instance, so the DSP1 device cannot call > snd_pcm_dmabuf_export() to export one dma buffer and pass to the > DSP2/audio-ip. > > Our original design is, the dma buffer should be associated with one PCM > device, since different PCM devices may need different buffer types (see > SNDRV_DMA_TYPE_XXX) to play PCM data, and need consider the PCM device's > coherent capability for DMA memory. My concern is, if we let other > non-audio device to allocate one buffer and export it, how to guarantee > the dma buffer can be used by PCM device and have the same coherent capability > with the PCM device. > > So I am not sure for this issue and need more suggestion to get a consensus. > > 2. Second question raised by Srinivas Kandagatla, he wondered: > "Am still unclear on the whole idea making a audio device dmabuf exporter > in Linux systems, unless you are sharing the dmabuf fd with other devices. > > If you are working with just one device we could just live with mmap, > isn't it?" > > Mark Brown gave the answer: > "The issue is permissions management. We've got a sound server that owns > all the sound hardware and needs to be able to retain administrative control > over it which means that permissions for the sound devices are locked down to > it. For performance reasons on systems where it's practical (eg, where there's > multiple audio streams the system can use to play data to the hardware) we > want to be able to have that server allow other applications with lower > permissions to stream data to/from the hardware without going through it. The > applications can't mmap() the device directly as that's too painful to do that > securely from a permission management point of view so we want to be able to > have the sound server do the mmap() then hand the mapped buffer off to the > application for it to use via a FD over a pipe. That way the only thing with > direct access to the devices is the sound server but the clients have a data > path that doesn't need to bounce through another process. > > Practically speaking the only use case I can think of in an audio context is > for one reader and one writer (AIUI Android is envisioning this as one > hardware > and one software), it's difficult to see how you could usefully chain multiple > in place transformations together in the way that you can with video > applications > but I'm possibly not thinking of something here." Hi, the tinyalsa implementation does not show much - it's equal to the standard mmap access for the PCM devices. Even considering the Mark's text, there must be an arbiter (sound server) which communicates with the producer or consumer to control the data flow. I really would like to see a real usage for this. It seems to me that the only point to implement this is the permissions. We already have O_APPEND mode for the PCM file descriptor which can reuse the PCM device multiple times (mmap the buffer to multiple tasks). I would probably go in this way and add more extended permission control for the PCM device, so permissions can be restricted for the passed descriptor to the producer or the consumer task. In this way, the restricted task might reuse other control mechanism offered buy the PCM file descriptor without requesting the arbiter to do so (like read the actual position in the DMA buffer, get the audio delay or so - reduce context switches). Thanks, Jaroslav -- Jaroslav Kysela Linux Sound Maintainer; ALSA Project; Red
[RFC PATCH] ALSA: core: Add DMA share buffer support
This patch adds dma share buffer support, which allows a dma-buf to be shared
between processes by passing file descriptors between them, allowing multiple
processes to cooperate in filling the DMA buffer used by the audio hardware
without the need to copy data around. This reduces both latency and CPU load,
especially in configurations where only one application is playing audio and
so context switches can be avoided.
In userspace, we can use ioctl:SNDRV_PCM_IOCTL_DMABUF_EXPORT to export one
dma buffer to a PCM, and use ioctl:SNDRV_PCM_IOCTL_DMABUF_ATTACH and
ioctl:SNDRV_PCM_IOCTL_DMABUF_DETACH to attach or detach one device to the
dma buffer. Morevoer we can use dma buffer ioctl:DMA_BUF_IOCTL_SYNC to
guarantee the cache coherency of the DMA buffer.
You can refer to below patches [1] created by Leo Yan to use the dma buffer
in userspace.
[1] https://github.com/tinyalsa/tinyalsa/pull/126
Welcome any comments. Thanks.
Signed-off-by: Baolin Wang
---
Hi,
Before sending to ALSA mailing list, we had some internal discussion off line,
so I posted them to follow up.
1. One issue proposed by Srinivas Kandagatla, he proposed one use case example:
DSP1 pre-process(compress-to-pcm) the buffers, pass the fd to DSP2/audio-ip
to play pcm data. The dmabuf exporter (DSP1) is a different device than
audio device in this instance, so the DSP1 device cannot call
snd_pcm_dmabuf_export() to export one dma buffer and pass to the DSP2/audio-ip.
Our original design is, the dma buffer should be associated with one PCM
device, since different PCM devices may need different buffer types (see
SNDRV_DMA_TYPE_XXX) to play PCM data, and need consider the PCM device's
coherent capability for DMA memory. My concern is, if we let other
non-audio device to allocate one buffer and export it, how to guarantee
the dma buffer can be used by PCM device and have the same coherent capability
with the PCM device.
So I am not sure for this issue and need more suggestion to get a consensus.
2. Second question raised by Srinivas Kandagatla, he wondered:
"Am still unclear on the whole idea making a audio device dmabuf exporter
in Linux systems, unless you are sharing the dmabuf fd with other devices.
If you are working with just one device we could just live with mmap,
isn't it?"
Mark Brown gave the answer:
"The issue is permissions management. We've got a sound server that owns
all the sound hardware and needs to be able to retain administrative control
over it which means that permissions for the sound devices are locked down to
it. For performance reasons on systems where it's practical (eg, where there's
multiple audio streams the system can use to play data to the hardware) we
want to be able to have that server allow other applications with lower
permissions to stream data to/from the hardware without going through it. The
applications can't mmap() the device directly as that's too painful to do that
securely from a permission management point of view so we want to be able to
have the sound server do the mmap() then hand the mapped buffer off to the
application for it to use via a FD over a pipe. That way the only thing with
direct access to the devices is the sound server but the clients have a data
path that doesn't need to bounce through another process.
Practically speaking the only use case I can think of in an audio context is
for one reader and one writer (AIUI Android is envisioning this as one hardware
and one software), it's difficult to see how you could usefully chain multiple
in place transformations together in the way that you can with video
applications
but I'm possibly not thinking of something here."
---
include/sound/pcm.h |2 +
include/sound/pcm_dmabuf.h | 29 +++
include/uapi/sound/asound.h |3 +
sound/core/Kconfig |4 +
sound/core/Makefile |1 +
sound/core/pcm.c|2 +
sound/core/pcm_compat.c |3 +
sound/core/pcm_dmabuf.c | 531 +++
sound/core/pcm_native.c | 38
9 files changed, 613 insertions(+)
create mode 100644 include/sound/pcm_dmabuf.h
create mode 100644 sound/core/pcm_dmabuf.c
diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index c47d9b4..d353e55 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -30,6 +30,7 @@
#include
#include
#include
+#include
#define snd_pcm_substream_chip(substream) ((substream)->private_data)
#define snd_pcm_chip(pcm) ((pcm)->private_data)
@@ -455,6 +456,7 @@ struct snd_pcm_substream {
size_t buffer_bytes_max;/* limit ring buffer size */
struct snd_dma_buffer dma_buffer;
size_t dma_max;
+ struct dma_buf_attachment *attachment;
/* -- hardware operations -- */
const struct snd_pcm_ops *ops;
/* -- runtime information -- */
diff --git a/include/sound/pcm_dmabuf.h b/include/sound/pcm_dmabuf.h
new file mode 100644
index 000..4e88c5f6
--- /dev/null
+++
