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

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

[Baolin Wang]

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 <baolin.wang@linaro.org>
---
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 <linux/mm.h>
 #include <linux/bitops.h>
 #include <linux/pm_qos.h>
+#include <linux/dma-buf.h>
 
 #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 0000000..4e88c5f6
--- /dev/null
+++ b/include/sound/pcm_dmabuf.h
@@ -0,0 +1,29 @@
+#ifndef __SOUND_PCM_DMABUF_H
+#define __SOUND_PCM_DMABUF_H
+
+#include <sound/pcm.h>
+
+#ifdef CONFIG_SND_PCM_DMABUF
+int snd_pcm_dmabuf_export(struct snd_pcm_substream *substream);
+int snd_pcm_dmabuf_attach(struct snd_pcm_substream *substream, int fd);
+void snd_pcm_dmabuf_detach(struct snd_pcm_substream *substream);
+#else
+static inline int snd_pcm_dmabuf_export(struct snd_pcm_substream *substream)
+{
+	return -EBADF;
+}
+
+static inline int snd_pcm_dmabuf_attach(struct snd_pcm_substream *substream,
+					int fd)
+{
+	return -ENXIO;
+}
+
+static inline void snd_pcm_dmabuf_detach(struct snd_pcm_substream *substream)
+{
+
+}
+
+#endif
+
+#endif /* __SOUND_PCM_DMABUF_H */
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 404d4b9..4770b41 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -602,6 +602,9 @@ enum {
 #define SNDRV_PCM_IOCTL_READN_FRAMES	_IOR('A', 0x53, struct snd_xfern)
 #define SNDRV_PCM_IOCTL_LINK		_IOW('A', 0x60, int)
 #define SNDRV_PCM_IOCTL_UNLINK		_IO('A', 0x61)
+#define SNDRV_PCM_IOCTL_DMABUF_EXPORT	_IOR('A', 0x70, int)
+#define SNDRV_PCM_IOCTL_DMABUF_ATTACH	_IOW('A', 0x71, int)
+#define SNDRV_PCM_IOCTL_DMABUF_DETACH	_IO('A', 0x72)
 
 /*****************************************************************************
  *                                                                           *
diff --git a/sound/core/Kconfig b/sound/core/Kconfig
index 63b3ef9..5ee02f2 100644
--- a/sound/core/Kconfig
+++ b/sound/core/Kconfig
@@ -184,4 +184,8 @@ config SND_DMA_SGBUF
 	def_bool y
 	depends on X86
 
+config SND_PCM_DMABUF
+	def_bool y
+	select DMA_SHARED_BUFFER
+
 source "sound/core/seq/Kconfig"
diff --git a/sound/core/Makefile b/sound/core/Makefile
index ee4a4a6..beea463 100644
--- a/sound/core/Makefile
+++ b/sound/core/Makefile
@@ -49,3 +49,4 @@ obj-$(CONFIG_SND_OSSEMUL)	+= oss/
 obj-$(CONFIG_SND_SEQUENCER)	+= seq/
 
 obj-$(CONFIG_SND_COMPRESS_OFFLOAD)	+= snd-compress.o
+obj-$(CONFIG_SND_PCM_DMABUF)	+= pcm_dmabuf.o
diff --git a/sound/core/pcm.c b/sound/core/pcm.c
index 01b9d62..7781db2 100644
--- a/sound/core/pcm.c
+++ b/sound/core/pcm.c
@@ -32,6 +32,7 @@
 #include <sound/timer.h>
 #include <sound/control.h>
 #include <sound/info.h>
+#include <sound/pcm_dmabuf.h>
 
 #include "pcm_local.h"
 
@@ -890,6 +891,7 @@ static void snd_pcm_free_stream(struct snd_pcm_str * pstr)
 		substream_next = substream->next;
 		snd_pcm_timer_done(substream);
 		snd_pcm_substream_proc_done(substream);
+		snd_pcm_dmabuf_detach(substream);
 		kfree(substream);
 		substream = substream_next;
 	}
diff --git a/sound/core/pcm_compat.c b/sound/core/pcm_compat.c
index 946ab08..af24489 100644
--- a/sound/core/pcm_compat.c
+++ b/sound/core/pcm_compat.c
@@ -687,6 +687,9 @@ static long snd_pcm_ioctl_compat(struct file *file, unsigned int cmd, unsigned l
 	case SNDRV_PCM_IOCTL_XRUN:
 	case SNDRV_PCM_IOCTL_LINK:
 	case SNDRV_PCM_IOCTL_UNLINK:
+	case SNDRV_PCM_IOCTL_DMABUF_EXPORT:
+	case SNDRV_PCM_IOCTL_DMABUF_ATTACH:
+	case SNDRV_PCM_IOCTL_DMABUF_DETACH:
 		return snd_pcm_common_ioctl(file, substream, cmd, argp);
 	case SNDRV_PCM_IOCTL_HW_REFINE32:
 		return snd_pcm_ioctl_hw_params_compat(substream, 1, argp);
diff --git a/sound/core/pcm_dmabuf.c b/sound/core/pcm_dmabuf.c
new file mode 100644
index 0000000..76211ea
--- /dev/null
+++ b/sound/core/pcm_dmabuf.c
@@ -0,0 +1,531 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// Copyright(C) 2018 Linaro Limited. All rights reserved.
+// Author: Baolin Wang <baolin.wang@linaro.org>
+
+#include <linux/export.h>
+#include <linux/dma-buf.h>
+#include <sound/core.h>
+#include <sound/pcm.h>
+#include <linux/slab.h>
+
+struct pcm_dmabuf_attachment {
+	struct device *dev;
+	struct sg_table *sgt;
+	enum dma_data_direction dir;
+	struct list_head list;
+};
+
+struct pcm_dmabuf_object {
+	struct snd_dma_buffer *dmab;
+	struct dma_buf *dmabuf;
+	struct page **pages;
+	int pages_num;
+	struct mutex lock;
+	struct list_head attachments;
+};
+
+static int snd_pcm_map_attach(struct dma_buf *dma_buf,
+			      struct dma_buf_attachment *attach)
+{
+	struct pcm_dmabuf_attachment *pcm_attach;
+
+	pcm_attach = kzalloc(sizeof(*pcm_attach), GFP_KERNEL);
+	if (!pcm_attach)
+		return -ENOMEM;
+
+	pcm_attach->dir = DMA_NONE;
+	attach->priv = pcm_attach;
+
+	return 0;
+}
+
+static void snd_pcm_map_detach(struct dma_buf *dma_buf,
+			       struct dma_buf_attachment *attach)
+{
+	struct pcm_dmabuf_attachment *pcm_attach = attach->priv;
+	struct sg_table *sgt;
+
+	if (!pcm_attach)
+		return;
+
+	sgt = pcm_attach->sgt;
+	if (sgt) {
+		if (pcm_attach->dir != DMA_NONE)
+			dma_unmap_sg_attrs(attach->dev, sgt->sgl,
+					   sgt->nents,
+					   pcm_attach->dir,
+					   DMA_ATTR_SKIP_CPU_SYNC);
+
+		sg_free_table(sgt);
+	}
+
+	kfree(sgt);
+	kfree(pcm_attach);
+	attach->priv = NULL;
+}
+
+static struct sg_table *snd_pcm_dmabuf_to_sgt(struct pcm_dmabuf_object *obj)
+{
+	struct snd_dma_buffer *dmab = obj->dmab;
+	int type = dmab->dev.type, ret, i;
+	struct sg_table *sgt;
+	unsigned long offset;
+
+	sgt = kmalloc(sizeof(*sgt), GFP_KERNEL);
+	if (!sgt)
+		return ERR_PTR(-ENOMEM);
+
+	switch (type) {
+	case SNDRV_DMA_TYPE_CONTINUOUS:
+		offset = offset_in_page(dmab->area);
+
+		for (i = 0; i < obj->pages_num; i++) {
+			struct page *page =
+				virt_to_page(dmab->area + i * PAGE_SIZE);
+
+			obj->pages[i] = page;
+		}
+
+		ret = sg_alloc_table_from_pages(sgt, obj->pages, obj->pages_num,
+						offset, dmab->bytes, GFP_KERNEL);
+		if (ret)
+			goto error;
+
+		break;
+
+#ifdef CONFIG_GENERIC_ALLOCATOR
+	case SNDRV_DMA_TYPE_DEV_IRAM:
+#endif
+	case SNDRV_DMA_TYPE_DEV:
+	case SNDRV_DMA_TYPE_DEV_UC:
+		ret = dma_get_sgtable(dmab->dev.dev, sgt, dmab->area,
+				      dmab->addr, dmab->bytes);
+		if (ret)
+			goto error;
+
+		break;
+
+#ifdef CONFIG_SND_DMA_SGBUF
+	case SNDRV_DMA_TYPE_DEV_SG:
+	case SNDRV_DMA_TYPE_DEV_UC_SG:
+		struct snd_sg_buf *sgbuf = dmab->private_data;
+
+		obj->pages = sgbuf->page_table;
+		ret = sg_alloc_table_from_pages(sgt, obj->pages, obj->pages_num,
+						0, dmab->bytes, GFP_KERNEL);
+		if (ret)
+			goto error;
+
+		break;
+#endif
+
+	default:
+		ret = -ENXIO;
+		goto error;
+	}
+
+	return sgt;
+
+error:
+	kfree(sgt);
+	return ERR_PTR(ret);
+}
+
+static struct sg_table *snd_pcm_map_dmabuf(struct dma_buf_attachment *attach,
+					   enum dma_data_direction dir)
+{
+	struct pcm_dmabuf_attachment *pcm_attach = attach->priv;
+	struct pcm_dmabuf_object *obj = attach->dmabuf->priv;
+	struct sg_table *sgt;
+	int ret;
+
+	if (WARN_ON(dir == DMA_NONE || !pcm_attach))
+		return ERR_PTR(-EINVAL);
+
+	if (pcm_attach->dir == dir)
+		return pcm_attach->sgt;
+
+	if (WARN_ON(pcm_attach->dir != DMA_NONE))
+		return ERR_PTR(-EBUSY);
+
+	sgt = snd_pcm_dmabuf_to_sgt(obj);
+	if (IS_ERR(sgt))
+		return sgt;
+
+	ret = dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,
+			       DMA_ATTR_SKIP_CPU_SYNC);
+	if (!ret) {
+		sg_free_table(sgt);
+		kfree(sgt);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	pcm_attach->sgt = sgt;
+	pcm_attach->dir = dir;
+
+	return sgt;
+}
+
+static void snd_pcm_unmap_dmabuf(struct dma_buf_attachment *attach,
+				 struct sg_table *sgt,
+				 enum dma_data_direction dir)
+{
+	/* Nothing need to do */
+}
+
+static int snd_pcm_dmabuf_mmap(struct dma_buf *dma_buf,
+			       struct vm_area_struct *vma)
+{
+	struct pcm_dmabuf_object *obj = dma_buf->priv;
+	struct snd_dma_buffer *dmab = obj->dmab;
+	unsigned long addr = vma->vm_start;
+	unsigned long offset = vma->vm_pgoff * PAGE_SIZE;
+	int type = dmab->dev.type, ret, i;
+	struct sg_table *sgt;
+	struct scatterlist *sg;
+
+	switch (type) {
+#ifdef CONFIG_GENERIC_ALLOCATOR
+	case SNDRV_DMA_TYPE_DEV_IRAM:
+#endif
+	case SNDRV_DMA_TYPE_DEV:
+	case SNDRV_DMA_TYPE_DEV_UC:
+		return dma_mmap_coherent(dmab->dev.dev, vma,
+					 dmab->area, dmab->addr,
+					 vma->vm_end - vma->vm_start);
+
+#ifdef CONFIG_SND_DMA_SGBUF
+	case SNDRV_DMA_TYPE_DEV_SG:
+	case SNDRV_DMA_TYPE_DEV_UC_SG:
+#endif
+	case SNDRV_DMA_TYPE_CONTINUOUS:
+		vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
+
+		sgt = snd_pcm_dmabuf_to_sgt(obj);
+		if (IS_ERR(sgt))
+			return PTR_ERR(sgt);
+
+		for_each_sg(sgt->sgl, sg, sgt->nents, i) {
+			struct page *page = sg_page(sg);
+			unsigned long remainder = vma->vm_end - addr;
+			unsigned long len = sg->length;
+
+			if (offset >= sg->length) {
+				offset -= sg->length;
+				continue;
+			} else if (offset) {
+				page += offset / PAGE_SIZE;
+				len = sg->length - offset;
+				offset = 0;
+			}
+
+			len = min(len, remainder);
+			ret = remap_pfn_range(vma, addr, page_to_pfn(page), len,
+					      vma->vm_page_prot);
+			if (ret)
+				return ret;
+
+			addr += len;
+			if (addr >= vma->vm_end)
+				return 0;
+		}
+
+	default:
+		return -ENXIO;
+	}
+
+	return 0;
+}
+
+static void snd_pcm_dmabuf_release(struct dma_buf *dmabuf)
+{
+	struct pcm_dmabuf_object *obj = dmabuf->priv;
+
+	kfree(obj->pages);
+	kfree(obj);
+}
+
+static int snd_pcm_dmabuf_begin_cpu_access(struct dma_buf *dmabuf,
+					   enum dma_data_direction direction)
+{
+	struct pcm_dmabuf_object *obj = dmabuf->priv;
+	struct snd_dma_buffer *dmab = obj->dmab;
+	int type = dmab->dev.type;
+	struct pcm_dmabuf_attachment *pcm_attach;
+
+	switch (type) {
+#ifdef CONFIG_GENERIC_ALLOCATOR
+	case SNDRV_DMA_TYPE_DEV_IRAM:
+#endif
+	case SNDRV_DMA_TYPE_DEV:
+	case SNDRV_DMA_TYPE_DEV_UC:
+		/* Memory types are uncacheable, nothing need to do. */
+		return 0;
+
+#ifdef CONFIG_SND_DMA_SGBUF
+	case SNDRV_DMA_TYPE_DEV_SG:
+	case SNDRV_DMA_TYPE_DEV_UC_SG:
+#endif
+	case SNDRV_DMA_TYPE_CONTINUOUS:
+		mutex_lock(&obj->lock);
+
+		/*
+		 * Sync the whole DMA buffer properly in order for the CPU
+		 * to see the most up-to-date and correct copy of the DMA
+		 * buffer.
+		 */
+		list_for_each_entry(pcm_attach, &obj->attachments, list) {
+			dma_sync_sg_for_cpu(pcm_attach->dev,
+					    pcm_attach->sgt->sgl,
+					    pcm_attach->sgt->nents,
+					    direction);
+		}
+
+		mutex_unlock(&obj->lock);
+
+		break;
+
+	default:
+		return -ENXIO;
+	}
+
+	return 0;
+}
+
+static int snd_pcm_dmabuf_end_cpu_access(struct dma_buf *dmabuf,
+					 enum dma_data_direction direction)
+{
+	struct pcm_dmabuf_object *obj = dmabuf->priv;
+	struct snd_dma_buffer *dmab = obj->dmab;
+	struct pcm_dmabuf_attachment *pcm_attach;
+	int type = dmab->dev.type;
+
+	switch (type) {
+#ifdef CONFIG_GENERIC_ALLOCATOR
+	case SNDRV_DMA_TYPE_DEV_IRAM:
+#endif
+	case SNDRV_DMA_TYPE_DEV:
+	case SNDRV_DMA_TYPE_DEV_UC:
+		/* Memory types are uncacheable, nothing need to do. */
+		return 0;
+
+#ifdef CONFIG_SND_DMA_SGBUF
+	case SNDRV_DMA_TYPE_DEV_SG:
+	case SNDRV_DMA_TYPE_DEV_UC_SG:
+#endif
+	case SNDRV_DMA_TYPE_CONTINUOUS:
+		mutex_lock(&obj->lock);
+
+		/*
+		 * Sync the whole DMA buffer properly in order for the devices
+		 * to see the most up-to-date and correct copy of the DMA
+		 * buffer.
+		 */
+		list_for_each_entry(pcm_attach, &obj->attachments, list) {
+			dma_sync_sg_for_device(pcm_attach->dev,
+					       pcm_attach->sgt->sgl,
+					       pcm_attach->sgt->nents,
+					       direction);
+		}
+
+		mutex_unlock(&obj->lock);
+
+		break;
+
+	default:
+		return -ENXIO;
+	}
+
+	return 0;
+}
+
+static const struct dma_buf_ops snd_pcm_dmabuf_ops =  {
+	.attach = snd_pcm_map_attach,
+	.detach = snd_pcm_map_detach,
+	.map_dma_buf = snd_pcm_map_dmabuf,
+	.unmap_dma_buf = snd_pcm_unmap_dmabuf,
+	.release = snd_pcm_dmabuf_release,
+	.mmap = snd_pcm_dmabuf_mmap,
+	.begin_cpu_access = snd_pcm_dmabuf_begin_cpu_access,
+	.end_cpu_access = snd_pcm_dmabuf_end_cpu_access,
+};
+
+/**
+ * snd_pcm_dmabuf_export - export one dma buffer associated with a PCM substream
+ * @substream: PCM substream
+ *
+ * Return: a file descriptor for the given dma buffer, otherwise a negative
+ * value on error.
+ */
+int snd_pcm_dmabuf_export(struct snd_pcm_substream *substream)
+{
+	struct snd_dma_buffer *dmab = &substream->dma_buffer;
+	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
+	struct pcm_dmabuf_object *obj;
+	int ret;
+
+	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
+	if (!obj)
+		return -ENOMEM;
+
+	mutex_init(&obj->lock);
+	INIT_LIST_HEAD(&obj->attachments);
+	obj->dmab = dmab;
+	obj->pages_num = (dmab->bytes + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	obj->pages = kcalloc(obj->pages_num, sizeof(obj->pages[0]), GFP_KERNEL);
+	if (!obj->pages) {
+		ret = -ENOMEM;
+		goto alloc_err;
+	}
+
+	exp_info.ops = &snd_pcm_dmabuf_ops;
+	exp_info.size = dmab->bytes;
+	exp_info.flags = O_RDWR;
+	exp_info.priv = obj;
+
+	obj->dmabuf = dma_buf_export(&exp_info);
+	if (IS_ERR(obj->dmabuf)) {
+		ret = PTR_ERR(obj->dmabuf);
+		goto export_err;
+	}
+
+	ret = dma_buf_fd(obj->dmabuf, O_CLOEXEC);
+	if (ret < 0)
+		goto fd_err;
+
+	return ret;
+
+fd_err:
+	dma_buf_put(obj->dmabuf);
+export_err:
+	kfree(obj->pages);
+alloc_err:
+	kfree(obj);
+	return ret;
+}
+EXPORT_SYMBOL(snd_pcm_dmabuf_export);
+
+/**
+ * snd_pcm_dmabuf_attach - attach one device to the dma buffer
+ * @substream: PCM substream
+ * @fd: file descriptor for the dma buffer
+ *
+ * Add one attachment to the dma buffer and map the scatterlist table of
+ * the attachment into device address space.
+ *
+ * Return: zero if attaching successfully, otherwise a negative value on error.
+ */
+int snd_pcm_dmabuf_attach(struct snd_pcm_substream *substream, int fd)
+{
+	enum dma_data_direction dir =
+		substream->stream == SNDRV_PCM_STREAM_PLAYBACK ?
+		DMA_TO_DEVICE : DMA_FROM_DEVICE;
+	struct snd_card *card = substream->pcm->card;
+	struct device *dev = card->dev;
+	struct dma_buf_attachment *attachment;
+	struct pcm_dmabuf_object *obj;
+	struct pcm_dmabuf_attachment *pcm_attach;
+	struct snd_dma_buffer *dmab;
+	struct dma_buf *dmabuf;
+	struct sg_table *sgt;
+	int ret;
+
+	dmabuf = dma_buf_get(fd);
+	if (IS_ERR(dmabuf))
+		return PTR_ERR(dmabuf);
+
+	attachment = dma_buf_attach(dmabuf, dev);
+	if (IS_ERR(attachment)) {
+		ret = PTR_ERR(attachment);
+		goto err_put;
+	}
+
+	sgt = dma_buf_map_attachment(attachment, dir);
+	if (IS_ERR(sgt)) {
+		ret = PTR_ERR(sgt);
+		goto err_detach;
+	}
+
+	pcm_attach = attachment->priv;
+	pcm_attach->dev = dev;
+	INIT_LIST_HEAD(&pcm_attach->list);
+	obj = dmabuf->priv;
+	dmab = obj->dmab;
+
+	switch (dmab->dev.type) {
+	case SNDRV_DMA_TYPE_CONTINUOUS:
+		substream->runtime->dma_area = dmab->area;
+		substream->runtime->dma_addr = sg_dma_address(sgt->sgl);
+		substream->runtime->dma_bytes = dmab->bytes;
+		break;
+
+#ifdef CONFIG_GENERIC_ALLOCATOR
+	case SNDRV_DMA_TYPE_DEV_IRAM:
+#endif
+	case SNDRV_DMA_TYPE_DEV:
+	case SNDRV_DMA_TYPE_DEV_UC:
+		substream->runtime->dma_area = dmab->area;
+		substream->runtime->dma_addr = dmab->addr;
+		substream->runtime->dma_bytes = dmab->bytes;
+		break;
+
+#ifdef CONFIG_SND_DMA_SGBUF
+	case SNDRV_DMA_TYPE_DEV_SG:
+	case SNDRV_DMA_TYPE_DEV_UC_SG:
+		/* TODO: Do not support now */
+		/* fall-through */
+#endif
+
+	default:
+		ret = -ENXIO;
+		goto err_runtime_buf;
+	}
+
+	substream->attachment = attachment;
+
+	mutex_lock(&obj->lock);
+	list_add(&pcm_attach->list, &obj->attachments);
+	mutex_unlock(&obj->lock);
+
+	return 0;
+
+err_runtime_buf:
+	dma_buf_unmap_attachment(attachment, sgt, dir);
+err_detach:
+	dma_buf_detach(dmabuf, attachment);
+err_put:
+	dma_buf_put(dmabuf);
+
+	return ret;
+}
+EXPORT_SYMBOL(snd_pcm_dmabuf_attach);
+
+/**
+ * snd_pcm_dmabuf_detach - detach the given attachment from dma buffer
+ * @substream: PCM substream
+ */
+void snd_pcm_dmabuf_detach(struct snd_pcm_substream *substream)
+{
+	struct dma_buf_attachment *attachment = substream->attachment;
+	struct pcm_dmabuf_attachment *pcm_attach;
+	struct pcm_dmabuf_object *obj;
+	struct dma_buf *dmabuf;
+
+	if (!attachment)
+		return;
+
+	pcm_attach = attachment->priv;
+	dmabuf = attachment->dmabuf;
+	obj = dmabuf->priv;
+
+	mutex_lock(&obj->lock);
+	list_del(&pcm_attach->list);
+	mutex_unlock(&obj->lock);
+
+	dma_buf_unmap_attachment(attachment, pcm_attach->sgt, pcm_attach->dir);
+	dma_buf_detach(dmabuf, attachment);
+	dma_buf_put(dmabuf);
+	substream->attachment = NULL;
+}
+EXPORT_SYMBOL(snd_pcm_dmabuf_detach);
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index ac37a71..5fad8dc 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -33,6 +33,7 @@
 #include <sound/info.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
+#include <sound/pcm_dmabuf.h>
 #include <sound/timer.h>
 #include <sound/minors.h>
 #include <linux/uio.h>
@@ -2863,6 +2864,37 @@ static int snd_pcm_forward_ioctl(struct snd_pcm_substream *substream,
 	return result < 0 ? result : 0;
 }
 
+static int snd_pcm_dmabuf_export_ioctl(struct snd_pcm_substream *substream,
+				       int __user *_fd)
+{
+	int fd;
+
+	fd = snd_pcm_dmabuf_export(substream);
+	if (fd < 0)
+		return fd;
+
+	__put_user(fd, _fd);
+	return 0;
+}
+
+static int snd_pcm_dmabuf_attach_ioctl(struct snd_pcm_substream *substream,
+				       int __user *_fd)
+{
+	int fd;
+
+	if (get_user(fd, _fd))
+		return -EFAULT;
+
+	return snd_pcm_dmabuf_attach(substream, fd);
+}
+
+static int snd_pcm_dmabuf_detach_ioctl(struct snd_pcm_substream *substream)
+{
+	snd_pcm_dmabuf_detach(substream);
+
+	return 0;
+}
+
 static int snd_pcm_common_ioctl(struct file *file,
 				 struct snd_pcm_substream *substream,
 				 unsigned int cmd, void __user *arg)
@@ -2960,6 +2992,12 @@ static int snd_pcm_common_ioctl(struct file *file,
 		return snd_pcm_rewind_ioctl(substream, arg);
 	case SNDRV_PCM_IOCTL_FORWARD:
 		return snd_pcm_forward_ioctl(substream, arg);
+	case SNDRV_PCM_IOCTL_DMABUF_EXPORT:
+		return snd_pcm_dmabuf_export_ioctl(substream, arg);
+	case SNDRV_PCM_IOCTL_DMABUF_ATTACH:
+		return snd_pcm_dmabuf_attach_ioctl(substream, arg);
+	case SNDRV_PCM_IOCTL_DMABUF_DETACH:
+		return snd_pcm_dmabuf_detach_ioctl(substream);
 	}
 	pcm_dbg(substream->pcm, "unknown ioctl = 0x%x\n", cmd);
 	return -ENOTTY;
-- 
1.7.9.5

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

[Jaroslav Kysela]

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 <baolin.wang@linaro.org>
> ---
> 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 <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1726 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Takashi Iwai]

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1093 bytes --]

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).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Jaroslav Kysela]

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 <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1712 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Takashi Iwai]

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

[Leo Yan]

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

[Jaroslav Kysela]

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 <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1228 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Baolin Wang]

Hi Jaroslav,
On Thu, 24 Jan 2019 at 21:43, Jaroslav Kysela <perex@perex.cz> 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

[Takashi Iwai]

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@perex.cz> 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

[Takashi Iwai]

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 <perex@perex.cz> 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

[Baolin Wang]

Hi Takashi,
On Fri, 25 Jan 2019 at 18:10, Takashi Iwai <tiwai@suse.de> 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@perex.cz> 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 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.

>
> - 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().

>
> - 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.

Make sense. Need add some validation to make sure if this PCM can export or not.

Very appreciated for your useful comments.

-- 
Baolin Wang
Best Regards

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

[Baolin Wang]

On Fri, 25 Jan 2019 at 18:20, Takashi Iwai <tiwai@suse.de> 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 <perex@perex.cz> 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. So is this not enough?

-- 
Baolin Wang
Best Regards

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@suse.de> 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@perex.cz> 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

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

[Takashi Iwai]

On Fri, 25 Jan 2019 12:24:29 +0100,
Baolin Wang wrote:
> 
> On Fri, 25 Jan 2019 at 18:20, Takashi Iwai <tiwai@suse.de> 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 <perex@perex.cz> 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. 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.


Takashi

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

[Takashi Iwai]

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: [alsa-devel] [RFC PATCH] ALSA: core: Add DMA share buffer support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 806 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Baolin Wang]

On Fri, 25 Jan 2019 at 21:03, Takashi Iwai <tiwai@suse.de> 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 <tiwai@suse.de> 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@perex.cz> 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, 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.

OK. Thanks for your explanation.

>
> > > - 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.

OK. That's one case need to validate. Thanks for your comments.

-- 
Baolin Wang
Best Regards

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

[Baolin Wang]

On Fri, 25 Jan 2019 at 21:04, Takashi Iwai <tiwai@suse.de> 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: [alsa-devel] [RFC PATCH] ALSA: core: Add DMA share buffer support

[Jaroslav Kysela]

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 <sound/minors.h>
 #include <linux/uio.h>
 #include <linux/delay.h>
+#include <linux/anon_inodes.h>
+#include <linux/syscalls.h>

 #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);
+	}
+	fd_install(fd, nfile);
+	res = snd_pcm_open_substream(substream->pcm, substream->number,
+				     nfile, &rsubstream);
+	if (res < 0) {
+		ksys_close(fd);
+		return res;
+	}
+	put_user(fd, (int __user *)arg);
+	return 0;
+}
+
 static int snd_pcm_common_ioctl(struct file *file,
 				 struct snd_pcm_substream *substream,
 				 unsigned int cmd, void __user *arg)
@@ -2864,6 +2902,8 @@ static int snd_pcm_common_ioctl(struct file *file,
 			     (unsigned int __user *)arg))
 			return -EFAULT;
 		return 0;
+	case SNDRV_PCM_IOCTL_ANONYMOUS_DUP:
+		return snd_pcm_anonymous_dup(file, substream, (int __user *)arg);
 	case SNDRV_PCM_IOCTL_HW_REFINE:
 		return snd_pcm_hw_refine_user(substream, arg);
 	case SNDRV_PCM_IOCTL_HW_PARAMS:
-- 

						Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

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

[Takashi Iwai]

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 <sound/minors.h>
>  #include <linux/uio.h>
>  #include <linux/delay.h>
> +#include <linux/anon_inodes.h>
> +#include <linux/syscalls.h>
> 
>  #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);
> +	}
> +	fd_install(fd, nfile);
> +	res = snd_pcm_open_substream(substream->pcm, substream->number,
> +				     nfile, &rsubstream);
> +	if (res < 0) {
> +		ksys_close(fd);
> +		return res;
> +	}
> +	put_user(fd, (int __user *)arg);
> +	return 0;
> +}
> +
>  static int snd_pcm_common_ioctl(struct file *file,
>  				 struct snd_pcm_substream *substream,
>  				 unsigned int cmd, void __user *arg)
> @@ -2864,6 +2902,8 @@ static int snd_pcm_common_ioctl(struct file *file,
>  			     (unsigned int __user *)arg))
>  			return -EFAULT;
>  		return 0;
> +	case SNDRV_PCM_IOCTL_ANONYMOUS_DUP:
> +		return snd_pcm_anonymous_dup(file, substream, (int __user *)arg);
>  	case SNDRV_PCM_IOCTL_HW_REFINE:
>  		return snd_pcm_hw_refine_user(substream, arg);
>  	case SNDRV_PCM_IOCTL_HW_PARAMS:
> -- 
> 
> 						Jaroslav
> 
> -- 
> Jaroslav Kysela <perex@perex.cz>
> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
>