[alsa-devel] [PATCH 0/5] ALSA: control: fix issue of concurrent access for TLV operations
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(-)
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) {
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:
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);
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)
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)
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
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
participants (2)
-
Takashi Iwai -
Takashi Sakamoto