[PATCH 0/5] ALSA: control: fix issue of concurrent access for TLV operations

[PATCH 0/5] ALSA: control: fix issue of concurrent access for TLV operations

[Takashi Sakamoto]

Hi,

In a design of ALSA control core, each set of elements have data of
Type-Length-Value shape. This is called as TLV data. In ALSA control
interface, applications can three types of requests to elements about
TLV data; read, write and command.

In current implementation of ALSA control core, these three requests
are handled with read lock of a counting semaphore. However, this has an
issue of concurrent access because in theory write/command requests have
sub-effects to change state of the set of elements in a point of TLV data.
Read requests and write/command requests are handled exclusively as well
as each write/command requests.

This patchset is for this purpose. Additionally, this applies code
refactoring to TLV ioctl handler and TLV handler for user-defined element
set so that they get better shape for readers.

Takashi Sakamoto (5):
  ALSA: control: queue events within locking of controls_rwsem for TLV
    operation
  ALSA: control: use counting semaphore as write lock for TLV
    write/command operations
  ALSA: control: obsolete user_ctl_lock
  ALSA: control: code refactoring TLV ioctl handler
  ALSA: control: code refactoring for TLV request handler to user
    element set

 include/sound/core.h |   2 -
 sound/core/control.c | 252 +++++++++++++++++++++++++++++++--------------------
 sound/core/init.c    |   1 -
 3 files changed, 155 insertions(+), 100 deletions(-)

-- 
2.11.0

[PATCH 1/5] ALSA: control: queue events within locking of controls_rwsem for TLV operation

[Takashi Sakamoto]

Any control event is queued by a call of snd_ctl_notify(). This function
adds the event to each queue of opened file data corresponding to ALSA
control character devices. This function acquired two types of lock; a
counting semaphore for a list of the opened file data and a spinlock for
card data opened by the file. Typically, this function is called after
acquiring a counting semaphore for a list of elements in the card data.

In current implementation of TLV request handler, the function is called
after releasing the semaphore for a list of elements in the card data.
This release is not necessarily needed.

This commit removes the release to call the function within the critical
section so that later commits are simple.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index ecd358213b83..31334abe673c 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1450,9 +1450,8 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file,
 		err = kctl->tlv.c(kctl, op_flag, tlv.length, _tlv->tlv);
 		if (err > 0) {
 			struct snd_ctl_elem_id id = kctl->id;
-			up_read(&card->controls_rwsem);
 			snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_TLV, &id);
-			return 0;
+			err = 0;
 		}
 	} else {
 		if (op_flag != SNDRV_CTL_TLV_OP_READ) {
-- 
2.11.0

[PATCH 2/5] ALSA: control: use counting semaphore as write lock for TLV write/command operations

[Takashi Sakamoto]

In ALSA control interface, applications can execute three types of request
for Type-Length-Value (TLV) data to a set of elements; read, write and
command. In ALSA control core, all of the requests are handled within read
lock to a counting semaphore, therefore several processes can run to access
to the data at the same time for any purposes. This has an issue because
write and command requests have side effect to change state of a set of
elements for the TLV data. Concurrent access should be controlled for each
of reference/change case.

This commit uses the counting semaphore as read lock for TLV read requests,
while use it as write lock for TLV write/command requests. The state of a
set of elements for the TLV data is maintained exclusively between read
requests and write/command requests, or between write and command requests.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 72 +++++++++++++++++++++++++++-------------------------
 1 file changed, 38 insertions(+), 34 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 31334abe673c..3648fff28138 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1414,7 +1414,6 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file,
 	struct snd_kcontrol *kctl;
 	struct snd_kcontrol_volatile *vd;
 	unsigned int len;
-	int err = 0;
 
 	if (copy_from_user(&tlv, _tlv, sizeof(tlv)))
 		return -EFAULT;
@@ -1422,53 +1421,49 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file,
 		return -EINVAL;
 	if (!tlv.numid)
 		return -EINVAL;
-	down_read(&card->controls_rwsem);
+
 	kctl = snd_ctl_find_numid(card, tlv.numid);
-	if (kctl == NULL) {
-		err = -ENOENT;
-		goto __kctl_end;
-	}
-	if (kctl->tlv.p == NULL) {
-		err = -ENXIO;
-		goto __kctl_end;
-	}
+	if (kctl == NULL)
+		return -ENOENT;
+
+	if (kctl->tlv.p == NULL)
+		return -ENXIO;
+
 	vd = &kctl->vd[tlv.numid - kctl->id.numid];
 	if ((op_flag == SNDRV_CTL_TLV_OP_READ &&
 	     (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_READ) == 0) ||
 	    (op_flag == SNDRV_CTL_TLV_OP_WRITE &&
 	     (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_WRITE) == 0) ||
 	    (op_flag == SNDRV_CTL_TLV_OP_CMD &&
-	     (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND) == 0)) {
-	    	err = -ENXIO;
-	    	goto __kctl_end;
-	}
+	     (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND) == 0))
+		return -ENXIO;
+
 	if (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) {
-		if (vd->owner != NULL && vd->owner != file) {
-			err = -EPERM;
-			goto __kctl_end;
-		}
+		int err;
+
+		if (vd->owner != NULL && vd->owner != file)
+			return -EPERM;
+
 		err = kctl->tlv.c(kctl, op_flag, tlv.length, _tlv->tlv);
+		if (err < 0)
+			return err;
 		if (err > 0) {
 			struct snd_ctl_elem_id id = kctl->id;
 			snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_TLV, &id);
-			err = 0;
 		}
 	} else {
-		if (op_flag != SNDRV_CTL_TLV_OP_READ) {
-			err = -ENXIO;
-			goto __kctl_end;
-		}
+		if (op_flag != SNDRV_CTL_TLV_OP_READ)
+			return -ENXIO;
+
 		len = kctl->tlv.p[1] + 2 * sizeof(unsigned int);
-		if (tlv.length < len) {
-			err = -ENOMEM;
-			goto __kctl_end;
-		}
+		if (tlv.length < len)
+			return -ENOMEM;
+
 		if (copy_to_user(_tlv->tlv, kctl->tlv.p, len))
-			err = -EFAULT;
+			return -EFAULT;
 	}
-      __kctl_end:
-	up_read(&card->controls_rwsem);
-	return err;
+
+	return 0;
 }
 
 static long snd_ctl_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
@@ -1510,11 +1505,20 @@ static long snd_ctl_ioctl(struct file *file, unsigned int cmd, unsigned long arg
 	case SNDRV_CTL_IOCTL_SUBSCRIBE_EVENTS:
 		return snd_ctl_subscribe_events(ctl, ip);
 	case SNDRV_CTL_IOCTL_TLV_READ:
-		return snd_ctl_tlv_ioctl(ctl, argp, SNDRV_CTL_TLV_OP_READ);
+		down_read(&ctl->card->controls_rwsem);
+		err = snd_ctl_tlv_ioctl(ctl, argp, SNDRV_CTL_TLV_OP_READ);
+		up_read(&ctl->card->controls_rwsem);
+		return err;
 	case SNDRV_CTL_IOCTL_TLV_WRITE:
-		return snd_ctl_tlv_ioctl(ctl, argp, SNDRV_CTL_TLV_OP_WRITE);
+		down_write(&ctl->card->controls_rwsem);
+		err = snd_ctl_tlv_ioctl(ctl, argp, SNDRV_CTL_TLV_OP_WRITE);
+		up_write(&ctl->card->controls_rwsem);
+		return err;
 	case SNDRV_CTL_IOCTL_TLV_COMMAND:
-		return snd_ctl_tlv_ioctl(ctl, argp, SNDRV_CTL_TLV_OP_CMD);
+		down_write(&ctl->card->controls_rwsem);
+		err = snd_ctl_tlv_ioctl(ctl, argp, SNDRV_CTL_TLV_OP_CMD);
+		up_write(&ctl->card->controls_rwsem);
+		return err;
 	case SNDRV_CTL_IOCTL_POWER:
 		return -ENOPROTOOPT;
 	case SNDRV_CTL_IOCTL_POWER_STATE:
-- 
2.11.0

[PATCH 3/5] ALSA: control: obsolete user_ctl_lock

[Takashi Sakamoto]

At a previous commit, concurrent requests for TLV data are maintained
exclusively between read requests and write/command requests. TLV
callback handlers in each driver has no risk from concurrent access for
reference/change.

In current implementation, 'struct snd_card' has a mutex to control
concurrent accesses to user-defined element sets. This commit obsoletes it.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 include/sound/core.h |  2 --
 sound/core/control.c | 37 +++++++++++++------------------------
 sound/core/init.c    |  1 -
 3 files changed, 13 insertions(+), 27 deletions(-)

diff --git a/include/sound/core.h b/include/sound/core.h
index 55385588eefa..357f36b5ee80 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -118,8 +118,6 @@ struct snd_card {
 	int user_ctl_count;		/* count of all user controls */
 	struct list_head controls;	/* all controls for this card */
 	struct list_head ctl_files;	/* active control files */
-	struct mutex user_ctl_lock;	/* protects user controls against
-					   concurrent access */
 
 	struct snd_info_entry *proc_root;	/* root for soundcard specific files */
 	struct snd_info_entry *proc_id;	/* the card id */
diff --git a/sound/core/control.c b/sound/core/control.c
index 3648fff28138..c1795b42796e 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1095,9 +1095,7 @@ static int snd_ctl_elem_user_get(struct snd_kcontrol *kcontrol,
 	char *src = ue->elem_data +
 			snd_ctl_get_ioff(kcontrol, &ucontrol->id) * size;
 
-	mutex_lock(&ue->card->user_ctl_lock);
 	memcpy(&ucontrol->value, src, size);
-	mutex_unlock(&ue->card->user_ctl_lock);
 	return 0;
 }
 
@@ -1110,11 +1108,9 @@ static int snd_ctl_elem_user_put(struct snd_kcontrol *kcontrol,
 	char *dst = ue->elem_data +
 			snd_ctl_get_ioff(kcontrol, &ucontrol->id) * size;
 
-	mutex_lock(&ue->card->user_ctl_lock);
 	change = memcmp(&ucontrol->value, dst, size) != 0;
 	if (change)
 		memcpy(dst, &ucontrol->value, size);
-	mutex_unlock(&ue->card->user_ctl_lock);
 	return change;
 }
 
@@ -1124,44 +1120,37 @@ static int snd_ctl_elem_user_tlv(struct snd_kcontrol *kcontrol,
 				 unsigned int __user *tlv)
 {
 	struct user_element *ue = kcontrol->private_data;
-	int change = 0;
-	void *new_data;
 
 	if (op_flag == SNDRV_CTL_TLV_OP_WRITE) {
+		int change;
+		void *new_data;
+
 		if (size > 1024 * 128)	/* sane value */
 			return -EINVAL;
 
 		new_data = memdup_user(tlv, size);
 		if (IS_ERR(new_data))
 			return PTR_ERR(new_data);
-		mutex_lock(&ue->card->user_ctl_lock);
 		change = ue->tlv_data_size != size;
 		if (!change)
 			change = memcmp(ue->tlv_data, new_data, size);
 		kfree(ue->tlv_data);
 		ue->tlv_data = new_data;
 		ue->tlv_data_size = size;
-		mutex_unlock(&ue->card->user_ctl_lock);
+
+		return change;
 	} else {
-		int ret = 0;
+		if (!ue->tlv_data_size || !ue->tlv_data)
+			return -ENXIO;
+
+		if (size < ue->tlv_data_size)
+			return -ENOSPC;
 
-		mutex_lock(&ue->card->user_ctl_lock);
-		if (!ue->tlv_data_size || !ue->tlv_data) {
-			ret = -ENXIO;
-			goto err_unlock;
-		}
-		if (size < ue->tlv_data_size) {
-			ret = -ENOSPC;
-			goto err_unlock;
-		}
 		if (copy_to_user(tlv, ue->tlv_data, ue->tlv_data_size))
-			ret = -EFAULT;
-err_unlock:
-		mutex_unlock(&ue->card->user_ctl_lock);
-		if (ret)
-			return ret;
+			return -EFAULT;
+
+		return 0;
 	}
-	return change;
 }
 
 static int snd_ctl_elem_init_enum_names(struct user_element *ue)
diff --git a/sound/core/init.c b/sound/core/init.c
index 00f2cbb76e69..f1425f9af703 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -248,7 +248,6 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
 	INIT_LIST_HEAD(&card->devices);
 	init_rwsem(&card->controls_rwsem);
 	rwlock_init(&card->ctl_files_rwlock);
-	mutex_init(&card->user_ctl_lock);
 	INIT_LIST_HEAD(&card->controls);
 	INIT_LIST_HEAD(&card->ctl_files);
 	spin_lock_init(&card->files_lock);
-- 
2.11.0

[PATCH 4/5] ALSA: control: code refactoring TLV ioctl handler

[Takashi Sakamoto]

In a design of ALSA control core, execution path bifurcates depending on
target element. When a set with the target element has a handler, it's
called. Else, registered buffer is copied to user space. These two
operations are apparently different.  In current implementation, they're
on the same function with a condition statement. This makes it a bit hard
to understand conditions of each case.

This commit splits codes for these two cases.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 132 +++++++++++++++++++++++++++++++++++----------------
 1 file changed, 91 insertions(+), 41 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index c1795b42796e..d8caf3745dea 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1394,65 +1394,115 @@ static int snd_ctl_subscribe_events(struct snd_ctl_file *file, int __user *ptr)
 	return 0;
 }
 
+static int call_tlv_handler(struct snd_ctl_file *file, int op_flag,
+			    struct snd_kcontrol *kctl,
+			    struct snd_ctl_elem_id *id,
+			    unsigned int __user *buf, unsigned int size)
+{
+	static const struct {
+		int op;
+		int perm;
+	} pairs[] = {
+		{SNDRV_CTL_TLV_OP_READ,  SNDRV_CTL_ELEM_ACCESS_TLV_READ},
+		{SNDRV_CTL_TLV_OP_WRITE, SNDRV_CTL_ELEM_ACCESS_TLV_WRITE},
+		{SNDRV_CTL_TLV_OP_CMD,   SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND},
+	};
+	struct snd_kcontrol_volatile *vd = &kctl->vd[snd_ctl_get_ioff(kctl, id)];
+	int i;
+	int err;
+
+	/* Check support of the request for this element. */
+	for (i = 0; i < ARRAY_SIZE(pairs); ++i) {
+		if (op_flag == pairs[i].op && (vd->access & pairs[i].perm))
+			break;
+	}
+	if (i == ARRAY_SIZE(pairs))
+		return -ENXIO;
+
+	if (kctl->tlv.c == NULL)
+		return -ENXIO;
+
+	/* When locked, this is unavailable. */
+	if (vd->owner != NULL && vd->owner != file)
+		return -EPERM;
+
+	err = kctl->tlv.c(kctl, op_flag, size, buf);
+	if (err < 0)
+		return err;
+
+	if (err > 0)
+		snd_ctl_notify(file->card, SNDRV_CTL_EVENT_MASK_TLV, id);
+
+	return 0;
+}
+
+static int read_tlv_buf(struct snd_kcontrol *kctl, struct snd_ctl_elem_id *id,
+			unsigned int __user *buf, unsigned int size)
+{
+	struct snd_kcontrol_volatile *vd = &kctl->vd[snd_ctl_get_ioff(kctl, id)];
+	unsigned int len;
+
+	if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_READ))
+		return -ENXIO;
+
+	if (kctl->tlv.p == NULL)
+		return -ENXIO;
+
+	len = sizeof(unsigned int) * 2 + kctl->tlv.p[1];
+	if (size < len)
+		return -ENOMEM;
+
+	if (copy_to_user(buf, kctl->tlv.p, len))
+		return -EFAULT;
+
+	return 0;
+}
+
 static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file,
-                             struct snd_ctl_tlv __user *_tlv,
+			     struct snd_ctl_tlv __user *buf,
                              int op_flag)
 {
-	struct snd_card *card = file->card;
-	struct snd_ctl_tlv tlv;
+	struct snd_ctl_tlv header;
+	unsigned int *container;
+	unsigned int container_size;
 	struct snd_kcontrol *kctl;
+	struct snd_ctl_elem_id id;
 	struct snd_kcontrol_volatile *vd;
-	unsigned int len;
 
-	if (copy_from_user(&tlv, _tlv, sizeof(tlv)))
+	if (copy_from_user(&header, buf, sizeof(header)))
 		return -EFAULT;
-	if (tlv.length < sizeof(unsigned int) * 2)
+
+	/* In design of control core, numerical ID starts at 1. */
+	if (header.numid == 0)
 		return -EINVAL;
-	if (!tlv.numid)
+
+	/* At least, container should include type and length fields.  */
+	if (header.length < sizeof(unsigned int) * 2)
 		return -EINVAL;
+	container_size = header.length;
+	container = buf->tlv;
 
-	kctl = snd_ctl_find_numid(card, tlv.numid);
+	kctl = snd_ctl_find_numid(file->card, header.numid);
 	if (kctl == NULL)
 		return -ENOENT;
 
-	if (kctl->tlv.p == NULL)
-		return -ENXIO;
-
-	vd = &kctl->vd[tlv.numid - kctl->id.numid];
-	if ((op_flag == SNDRV_CTL_TLV_OP_READ &&
-	     (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_READ) == 0) ||
-	    (op_flag == SNDRV_CTL_TLV_OP_WRITE &&
-	     (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_WRITE) == 0) ||
-	    (op_flag == SNDRV_CTL_TLV_OP_CMD &&
-	     (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND) == 0))
-		return -ENXIO;
+	/* Calculate index of the element in this set. */
+	id = kctl->id;
+	snd_ctl_build_ioff(&id, kctl, header.numid - id.numid);
+	vd = &kctl->vd[snd_ctl_get_ioff(kctl, &id)];
 
 	if (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) {
-		int err;
-
-		if (vd->owner != NULL && vd->owner != file)
-			return -EPERM;
-
-		err = kctl->tlv.c(kctl, op_flag, tlv.length, _tlv->tlv);
-		if (err < 0)
-			return err;
-		if (err > 0) {
-			struct snd_ctl_elem_id id = kctl->id;
-			snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_TLV, &id);
-		}
+		return call_tlv_handler(file, op_flag, kctl, &id, container,
+					container_size);
 	} else {
-		if (op_flag != SNDRV_CTL_TLV_OP_READ)
-			return -ENXIO;
-
-		len = kctl->tlv.p[1] + 2 * sizeof(unsigned int);
-		if (tlv.length < len)
-			return -ENOMEM;
-
-		if (copy_to_user(_tlv->tlv, kctl->tlv.p, len))
-			return -EFAULT;
+		if (op_flag == SNDRV_CTL_TLV_OP_READ) {
+			return read_tlv_buf(kctl, &id, container,
+					    container_size);
+		}
 	}
 
-	return 0;
+	/* Not supported. */
+	return -ENXIO;
 }
 
 static long snd_ctl_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
-- 
2.11.0

[PATCH 5/5] ALSA: control: code refactoring for TLV request handler to user element set

[Takashi Sakamoto]

User-defined element set registers own handler to get callbacks from TLV
ioctl handler. In the handler, execution path bifurcates depending on
requests from user space. At write request, container in given buffer is
registered to the element set, or replaced old TLV data. At the read
request, the registered data is copied to user space. The command request
is not allowed.  In current implementation, function of the handler
includes codes for the two cases.

This commit adds two helper functions for these cases so that readers can
easily get the above design.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 76 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 46 insertions(+), 30 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index d8caf3745dea..1d411b83f758 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1114,43 +1114,59 @@ static int snd_ctl_elem_user_put(struct snd_kcontrol *kcontrol,
 	return change;
 }
 
-static int snd_ctl_elem_user_tlv(struct snd_kcontrol *kcontrol,
-				 int op_flag,
-				 unsigned int size,
-				 unsigned int __user *tlv)
+static int replace_user_tlv(struct snd_kcontrol *kctl, unsigned int __user *buf,
+			    unsigned int size)
 {
-	struct user_element *ue = kcontrol->private_data;
+	struct user_element *ue = kctl->private_data;
+	unsigned int *container;
+	int change;
 
-	if (op_flag == SNDRV_CTL_TLV_OP_WRITE) {
-		int change;
-		void *new_data;
+	if (size > 1024 * 128)	/* sane value */
+		return -EINVAL;
 
-		if (size > 1024 * 128)	/* sane value */
-			return -EINVAL;
+	container = memdup_user(buf, size);
+	if (IS_ERR(container))
+		return PTR_ERR(container);
 
-		new_data = memdup_user(tlv, size);
-		if (IS_ERR(new_data))
-			return PTR_ERR(new_data);
-		change = ue->tlv_data_size != size;
-		if (!change)
-			change = memcmp(ue->tlv_data, new_data, size);
-		kfree(ue->tlv_data);
-		ue->tlv_data = new_data;
-		ue->tlv_data_size = size;
-
-		return change;
-	} else {
-		if (!ue->tlv_data_size || !ue->tlv_data)
-			return -ENXIO;
+	change = ue->tlv_data_size != size;
+	if (!change)
+		change = memcmp(ue->tlv_data, container, size);
+	if (!change) {
+		kfree(container);
+		return 0;
+	}
 
-		if (size < ue->tlv_data_size)
-			return -ENOSPC;
+	kfree(ue->tlv_data);
+	ue->tlv_data = container;
+	ue->tlv_data_size = size;
 
-		if (copy_to_user(tlv, ue->tlv_data, ue->tlv_data_size))
-			return -EFAULT;
+	return change;
+}
 
-		return 0;
-	}
+static int read_user_tlv(struct snd_kcontrol *kctl, unsigned int __user *buf,
+			 unsigned int size)
+{
+	struct user_element *ue = kctl->private_data;
+
+	if (ue->tlv_data_size == 0 || ue->tlv_data == NULL)
+		return -ENXIO;
+
+	if (size < ue->tlv_data_size)
+		return -ENOSPC;
+
+	if (copy_to_user(buf, ue->tlv_data, ue->tlv_data_size))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int snd_ctl_elem_user_tlv(struct snd_kcontrol *kctl, int op_flag,
+				 unsigned int size, unsigned int __user *buf)
+{
+	if (op_flag == SNDRV_CTL_TLV_OP_WRITE)
+		return replace_user_tlv(kctl, buf, size);
+	else
+		return read_user_tlv(kctl, buf, size);
 }
 
 static int snd_ctl_elem_init_enum_names(struct user_element *ue)
-- 
2.11.0

Re: [PATCH 0/5] ALSA: control: fix issue of concurrent access for TLV operations

[Takashi Iwai]

On Thu, 03 Aug 2017 13:20:39 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> In a design of ALSA control core, each set of elements have data of
> Type-Length-Value shape. This is called as TLV data. In ALSA control
> interface, applications can three types of requests to elements about
> TLV data; read, write and command.
> 
> In current implementation of ALSA control core, these three requests
> are handled with read lock of a counting semaphore. However, this has an
> issue of concurrent access because in theory write/command requests have
> sub-effects to change state of the set of elements in a point of TLV data.
> Read requests and write/command requests are handled exclusively as well
> as each write/command requests.
> 
> This patchset is for this purpose. Additionally, this applies code
> refactoring to TLV ioctl handler and TLV handler for user-defined element
> set so that they get better shape for readers.

Taking the rw mutex outside snd_ctl_tlv_ioctl() is a good idea.
It simplified things a lot, indeed.

Now applied all patches.  Thanks!


Takashi

Re: [PATCH 0/5] ALSA: control: fix issue of concurrent access for TLV operations

[Takashi Sakamoto]

Hi,

On 2017年08月04日 23:52, Takashi Iwai wrote:
> On Thu, 03 Aug 2017 13:20:39 +0200,
> Takashi Sakamoto wrote:
>>
>> Hi,
>>
>> In a design of ALSA control core, each set of elements have data of
>> Type-Length-Value shape. This is called as TLV data. In ALSA control
>> interface, applications can three types of requests to elements about
>> TLV data; read, write and command.
>>
>> In current implementation of ALSA control core, these three requests
>> are handled with read lock of a counting semaphore. However, this has an
>> issue of concurrent access because in theory write/command requests have
>> sub-effects to change state of the set of elements in a point of TLV data.
>> Read requests and write/command requests are handled exclusively as well
>> as each write/command requests.
>>
>> This patchset is for this purpose. Additionally, this applies code
>> refactoring to TLV ioctl handler and TLV handler for user-defined element
>> set so that they get better shape for readers.
> 
> Taking the rw mutex outside snd_ctl_tlv_ioctl() is a good idea.
> It simplified things a lot, indeed.
> 
> Now applied all patches.  Thanks!

Thanks. But I found that commit 30d8340b5857 ("ALSA: control: obsolete 
user_ctl_lock") brought another concurrent access issue between 
ELEM_READ/ELEM_WRITE for user-defined element sets, due to a generic bug 
in handlers of these two requests in ALSA control core. I pushed my 
commits into my repository to fix this. The latest three commits.
https://github.com/takaswie/sound/commits/topic/fix-ctl-writelock

I'll post them after enough tests.


Thanks

Takashi Sakamoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel