From fd7ae83de11a597cbe15770382f101c784a79b1c Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Thu, 28 Mar 2019 16:09:45 +0100 Subject: ALSA: seq: Use kvmalloc() for cell pools Use kvmalloc() for allocating cell pools since the pool size can be relatively small that may be covered better by slab. Signed-off-by: Takashi Iwai --- sound/core/seq/seq_memory.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'sound/core/seq') diff --git a/sound/core/seq/seq_memory.c b/sound/core/seq/seq_memory.c index 5b0388202bac..6ea4d8a5a71e 100644 --- a/sound/core/seq/seq_memory.c +++ b/sound/core/seq/seq_memory.c @@ -24,7 +24,7 @@ #include #include #include -#include +#include #include #include @@ -389,8 +389,8 @@ int snd_seq_pool_init(struct snd_seq_pool *pool) if (snd_BUG_ON(!pool)) return -EINVAL; - cellptr = vmalloc(array_size(sizeof(struct snd_seq_event_cell), - pool->size)); + cellptr = kvmalloc_array(sizeof(struct snd_seq_event_cell), pool->size, + GFP_KERNEL); if (!cellptr) return -ENOMEM; @@ -398,7 +398,7 @@ int snd_seq_pool_init(struct snd_seq_pool *pool) spin_lock_irqsave(&pool->lock, flags); if (pool->ptr) { spin_unlock_irqrestore(&pool->lock, flags); - vfree(cellptr); + kvfree(cellptr); return 0; } @@ -456,7 +456,7 @@ int snd_seq_pool_done(struct snd_seq_pool *pool) pool->total_elements = 0; spin_unlock_irqrestore(&pool->lock, flags); - vfree(ptr); + kvfree(ptr); spin_lock_irqsave(&pool->lock, flags); pool->closing = 0; -- cgit v1.2.3 From 4b24b960b10b6a4e30beba3ce097fa867b4a085f Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Thu, 28 Mar 2019 15:55:08 +0100 Subject: ALSA: seq: Align temporary re-locking with irqsave version In a few places in sequencer core, we temporarily unlock / re-lock the pool spin lock while waiting for the allocation in the blocking mode. There spin_unlock_irq() / spin_lock_irq() pairs are called while initially spin_lock_irqsave() is used (and spin_lock_irqrestore() at the end of the function again). This is likely OK for now, but it's a bit confusing and error-prone. This patch replaces these temporary relocking lines with the irqsave variant to make the lock/unlock sequence more consistently. Signed-off-by: Takashi Iwai --- sound/core/seq/seq_fifo.c | 4 ++-- sound/core/seq/seq_memory.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) (limited to 'sound/core/seq') diff --git a/sound/core/seq/seq_fifo.c b/sound/core/seq/seq_fifo.c index 72c0302a55d2..613ae10d33b8 100644 --- a/sound/core/seq/seq_fifo.c +++ b/sound/core/seq/seq_fifo.c @@ -195,9 +195,9 @@ int snd_seq_fifo_cell_out(struct snd_seq_fifo *f, } set_current_state(TASK_INTERRUPTIBLE); add_wait_queue(&f->input_sleep, &wait); - spin_unlock_irq(&f->lock); + spin_unlock_irqrestore(&f->lock, flags); schedule(); - spin_lock_irq(&f->lock); + spin_lock_irqsave(&f->lock, flags); remove_wait_queue(&f->input_sleep, &wait); if (signal_pending(current)) { spin_unlock_irqrestore(&f->lock, flags); diff --git a/sound/core/seq/seq_memory.c b/sound/core/seq/seq_memory.c index 6ea4d8a5a71e..ae0b8971f6ce 100644 --- a/sound/core/seq/seq_memory.c +++ b/sound/core/seq/seq_memory.c @@ -244,13 +244,13 @@ static int snd_seq_cell_alloc(struct snd_seq_pool *pool, set_current_state(TASK_INTERRUPTIBLE); add_wait_queue(&pool->output_sleep, &wait); - spin_unlock_irq(&pool->lock); + spin_unlock_irqrestore(&pool->lock, flags); if (mutexp) mutex_unlock(mutexp); schedule(); if (mutexp) mutex_lock(mutexp); - spin_lock_irq(&pool->lock); + spin_lock_irqsave(&pool->lock, flags); remove_wait_queue(&pool->output_sleep, &wait); /* interrupted? */ if (signal_pending(current)) { -- cgit v1.2.3 From f823b8a75527dca0b93cf577bbabbe47fd79b2a8 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Thu, 28 Mar 2019 16:21:01 +0100 Subject: ALSA: seq: Remove superfluous irqsave flags spin_lock_irqsave() is used unnecessarily in various places in sequencer core code although it's pretty obvious that the context is sleepable. Remove irqsave and use the plain spin_lock_irq() in such places for simplicity. Signed-off-by: Takashi Iwai --- sound/core/seq/seq_clientmgr.c | 19 ++++++++----------- sound/core/seq/seq_fifo.c | 10 ++++------ sound/core/seq/seq_memory.c | 16 +++++++--------- sound/core/seq/seq_ports.c | 15 ++++++--------- 4 files changed, 25 insertions(+), 35 deletions(-) (limited to 'sound/core/seq') diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index 7d4640d1fe9f..933bde3843d9 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -203,7 +203,6 @@ int __init client_init_data(void) static struct snd_seq_client *seq_create_client1(int client_index, int poolsize) { - unsigned long flags; int c; struct snd_seq_client *client; @@ -224,7 +223,7 @@ static struct snd_seq_client *seq_create_client1(int client_index, int poolsize) mutex_init(&client->ioctl_mutex); /* find free slot in the client table */ - spin_lock_irqsave(&clients_lock, flags); + spin_lock_irq(&clients_lock); if (client_index < 0) { for (c = SNDRV_SEQ_DYNAMIC_CLIENTS_BEGIN; c < SNDRV_SEQ_MAX_CLIENTS; @@ -232,17 +231,17 @@ static struct snd_seq_client *seq_create_client1(int client_index, int poolsize) if (clienttab[c] || clienttablock[c]) continue; clienttab[client->number = c] = client; - spin_unlock_irqrestore(&clients_lock, flags); + spin_unlock_irq(&clients_lock); return client; } } else { if (clienttab[client_index] == NULL && !clienttablock[client_index]) { clienttab[client->number = client_index] = client; - spin_unlock_irqrestore(&clients_lock, flags); + spin_unlock_irq(&clients_lock); return client; } } - spin_unlock_irqrestore(&clients_lock, flags); + spin_unlock_irq(&clients_lock); snd_seq_pool_delete(&client->pool); kfree(client); return NULL; /* no free slot found or busy, return failure code */ @@ -251,23 +250,21 @@ static struct snd_seq_client *seq_create_client1(int client_index, int poolsize) static int seq_free_client1(struct snd_seq_client *client) { - unsigned long flags; - if (!client) return 0; - spin_lock_irqsave(&clients_lock, flags); + spin_lock_irq(&clients_lock); clienttablock[client->number] = 1; clienttab[client->number] = NULL; - spin_unlock_irqrestore(&clients_lock, flags); + spin_unlock_irq(&clients_lock); snd_seq_delete_all_ports(client); snd_seq_queue_client_leave(client->number); snd_use_lock_sync(&client->use_lock); snd_seq_queue_client_termination(client->number); if (client->pool) snd_seq_pool_delete(&client->pool); - spin_lock_irqsave(&clients_lock, flags); + spin_lock_irq(&clients_lock); clienttablock[client->number] = 0; - spin_unlock_irqrestore(&clients_lock, flags); + spin_unlock_irq(&clients_lock); return 0; } diff --git a/sound/core/seq/seq_fifo.c b/sound/core/seq/seq_fifo.c index 613ae10d33b8..97ee89cb6426 100644 --- a/sound/core/seq/seq_fifo.c +++ b/sound/core/seq/seq_fifo.c @@ -98,18 +98,17 @@ static struct snd_seq_event_cell *fifo_cell_out(struct snd_seq_fifo *f); void snd_seq_fifo_clear(struct snd_seq_fifo *f) { struct snd_seq_event_cell *cell; - unsigned long flags; /* clear overflow flag */ atomic_set(&f->overflow, 0); snd_use_lock_sync(&f->use_lock); - spin_lock_irqsave(&f->lock, flags); + spin_lock_irq(&f->lock); /* drain the fifo */ while ((cell = fifo_cell_out(f)) != NULL) { snd_seq_cell_free(cell); } - spin_unlock_irqrestore(&f->lock, flags); + spin_unlock_irq(&f->lock); } @@ -239,7 +238,6 @@ int snd_seq_fifo_poll_wait(struct snd_seq_fifo *f, struct file *file, /* change the size of pool; all old events are removed */ int snd_seq_fifo_resize(struct snd_seq_fifo *f, int poolsize) { - unsigned long flags; struct snd_seq_pool *newpool, *oldpool; struct snd_seq_event_cell *cell, *next, *oldhead; @@ -255,7 +253,7 @@ int snd_seq_fifo_resize(struct snd_seq_fifo *f, int poolsize) return -ENOMEM; } - spin_lock_irqsave(&f->lock, flags); + spin_lock_irq(&f->lock); /* remember old pool */ oldpool = f->pool; oldhead = f->head; @@ -265,7 +263,7 @@ int snd_seq_fifo_resize(struct snd_seq_fifo *f, int poolsize) f->tail = NULL; f->cells = 0; /* NOTE: overflow flag is not cleared */ - spin_unlock_irqrestore(&f->lock, flags); + spin_unlock_irq(&f->lock); /* close the old pool and wait until all users are gone */ snd_seq_pool_mark_closing(oldpool); diff --git a/sound/core/seq/seq_memory.c b/sound/core/seq/seq_memory.c index ae0b8971f6ce..19b718e871c5 100644 --- a/sound/core/seq/seq_memory.c +++ b/sound/core/seq/seq_memory.c @@ -384,7 +384,6 @@ int snd_seq_pool_init(struct snd_seq_pool *pool) { int cell; struct snd_seq_event_cell *cellptr; - unsigned long flags; if (snd_BUG_ON(!pool)) return -EINVAL; @@ -395,9 +394,9 @@ int snd_seq_pool_init(struct snd_seq_pool *pool) return -ENOMEM; /* add new cells to the free cell list */ - spin_lock_irqsave(&pool->lock, flags); + spin_lock_irq(&pool->lock); if (pool->ptr) { - spin_unlock_irqrestore(&pool->lock, flags); + spin_unlock_irq(&pool->lock); kvfree(cellptr); return 0; } @@ -416,7 +415,7 @@ int snd_seq_pool_init(struct snd_seq_pool *pool) /* init statistics */ pool->max_used = 0; pool->total_elements = pool->size; - spin_unlock_irqrestore(&pool->lock, flags); + spin_unlock_irq(&pool->lock); return 0; } @@ -435,7 +434,6 @@ void snd_seq_pool_mark_closing(struct snd_seq_pool *pool) /* remove events */ int snd_seq_pool_done(struct snd_seq_pool *pool) { - unsigned long flags; struct snd_seq_event_cell *ptr; if (snd_BUG_ON(!pool)) @@ -449,18 +447,18 @@ int snd_seq_pool_done(struct snd_seq_pool *pool) schedule_timeout_uninterruptible(1); /* release all resources */ - spin_lock_irqsave(&pool->lock, flags); + spin_lock_irq(&pool->lock); ptr = pool->ptr; pool->ptr = NULL; pool->free = NULL; pool->total_elements = 0; - spin_unlock_irqrestore(&pool->lock, flags); + spin_unlock_irq(&pool->lock); kvfree(ptr); - spin_lock_irqsave(&pool->lock, flags); + spin_lock_irq(&pool->lock); pool->closing = 0; - spin_unlock_irqrestore(&pool->lock, flags); + spin_unlock_irq(&pool->lock); return 0; } diff --git a/sound/core/seq/seq_ports.c b/sound/core/seq/seq_ports.c index 24d90abfc64d..1e2239240f21 100644 --- a/sound/core/seq/seq_ports.c +++ b/sound/core/seq/seq_ports.c @@ -128,7 +128,6 @@ static void port_subs_info_init(struct snd_seq_port_subs_info *grp) struct snd_seq_client_port *snd_seq_create_port(struct snd_seq_client *client, int port) { - unsigned long flags; struct snd_seq_client_port *new_port, *p; int num = -1; @@ -157,7 +156,7 @@ struct snd_seq_client_port *snd_seq_create_port(struct snd_seq_client *client, num = port >= 0 ? port : 0; mutex_lock(&client->ports_mutex); - write_lock_irqsave(&client->ports_lock, flags); + write_lock_irq(&client->ports_lock); list_for_each_entry(p, &client->ports_list_head, list) { if (p->addr.port > num) break; @@ -169,7 +168,7 @@ struct snd_seq_client_port *snd_seq_create_port(struct snd_seq_client *client, client->num_ports++; new_port->addr.port = num; /* store the port number in the port */ sprintf(new_port->name, "port-%d", num); - write_unlock_irqrestore(&client->ports_lock, flags); + write_unlock_irq(&client->ports_lock); mutex_unlock(&client->ports_mutex); return new_port; @@ -283,11 +282,10 @@ static int port_delete(struct snd_seq_client *client, /* delete a port with the given port id */ int snd_seq_delete_port(struct snd_seq_client *client, int port) { - unsigned long flags; struct snd_seq_client_port *found = NULL, *p; mutex_lock(&client->ports_mutex); - write_lock_irqsave(&client->ports_lock, flags); + write_lock_irq(&client->ports_lock); list_for_each_entry(p, &client->ports_list_head, list) { if (p->addr.port == port) { /* ok found. delete from the list at first */ @@ -297,7 +295,7 @@ int snd_seq_delete_port(struct snd_seq_client *client, int port) break; } } - write_unlock_irqrestore(&client->ports_lock, flags); + write_unlock_irq(&client->ports_lock); mutex_unlock(&client->ports_mutex); if (found) return port_delete(client, found); @@ -308,7 +306,6 @@ int snd_seq_delete_port(struct snd_seq_client *client, int port) /* delete the all ports belonging to the given client */ int snd_seq_delete_all_ports(struct snd_seq_client *client) { - unsigned long flags; struct list_head deleted_list; struct snd_seq_client_port *port, *tmp; @@ -316,7 +313,7 @@ int snd_seq_delete_all_ports(struct snd_seq_client *client) * clear the port list in the client data. */ mutex_lock(&client->ports_mutex); - write_lock_irqsave(&client->ports_lock, flags); + write_lock_irq(&client->ports_lock); if (! list_empty(&client->ports_list_head)) { list_add(&deleted_list, &client->ports_list_head); list_del_init(&client->ports_list_head); @@ -324,7 +321,7 @@ int snd_seq_delete_all_ports(struct snd_seq_client *client) INIT_LIST_HEAD(&deleted_list); } client->num_ports = 0; - write_unlock_irqrestore(&client->ports_lock, flags); + write_unlock_irq(&client->ports_lock); /* remove each port in deleted_list */ list_for_each_entry_safe(port, tmp, &deleted_list, list) { -- cgit v1.2.3 From feb689025fbb6f0aa6297d3ddf97de945ea4ad32 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Tue, 9 Apr 2019 17:35:22 +0200 Subject: ALSA: seq: Protect in-kernel ioctl calls with mutex ALSA OSS sequencer calls the ioctl function indirectly via snd_seq_kernel_client_ctl(). While we already applied the protection against races between the normal ioctls and writes via the client's ioctl_mutex, this code path was left untouched. And this seems to be the cause of still remaining some rare UAF as spontaneously triggered by syzkaller. For the sake of robustness, wrap the ioctl_mutex also for the call via snd_seq_kernel_client_ctl(), too. Reported-by: syzbot+e4c8abb920efa77bace9@syzkaller.appspotmail.com Signed-off-by: Takashi Iwai --- sound/core/seq/seq_clientmgr.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) (limited to 'sound/core/seq') diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index 933bde3843d9..976404691261 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -2340,14 +2340,19 @@ int snd_seq_kernel_client_ctl(int clientid, unsigned int cmd, void *arg) { const struct ioctl_handler *handler; struct snd_seq_client *client; + int err; client = clientptr(clientid); if (client == NULL) return -ENXIO; for (handler = ioctl_handlers; handler->cmd > 0; ++handler) { - if (handler->cmd == cmd) - return handler->func(client, arg); + if (handler->cmd == cmd) { + mutex_lock(&client->ioctl_mutex); + err = handler->func(client, arg); + mutex_unlock(&client->ioctl_mutex); + return err; + } } pr_debug("ALSA: seq unknown ioctl() 0x%x (type='%c', number=0x%02x)\n", -- cgit v1.2.3 From 2eabc5ec8ab4d4748a82050dfcb994119b983750 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Tue, 9 Apr 2019 18:04:17 +0200 Subject: ALSA: seq: Fix race of get-subscription call vs port-delete ioctls The snd_seq_ioctl_get_subscription() retrieves the port subscriber information as a pointer, while the object isn't protected, hence it may be deleted before the actual reference. This race was spotted by syzkaller and may lead to a UAF. The fix is simply copying the data in the lookup function that performs in the rwsem to protect against the deletion. Reported-by: syzbot+9437020c82413d00222d@syzkaller.appspotmail.com Signed-off-by: Takashi Iwai --- sound/core/seq/seq_clientmgr.c | 10 ++-------- sound/core/seq/seq_ports.c | 13 ++++++++----- sound/core/seq/seq_ports.h | 5 +++-- 3 files changed, 13 insertions(+), 15 deletions(-) (limited to 'sound/core/seq') diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index 976404691261..f256704dc401 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -1897,20 +1897,14 @@ static int snd_seq_ioctl_get_subscription(struct snd_seq_client *client, int result; struct snd_seq_client *sender = NULL; struct snd_seq_client_port *sport = NULL; - struct snd_seq_subscribers *p; result = -EINVAL; if ((sender = snd_seq_client_use_ptr(subs->sender.client)) == NULL) goto __end; if ((sport = snd_seq_port_use_ptr(sender, subs->sender.port)) == NULL) goto __end; - p = snd_seq_port_get_subscription(&sport->c_src, &subs->dest); - if (p) { - result = 0; - *subs = p->info; - } else - result = -ENOENT; - + result = snd_seq_port_get_subscription(&sport->c_src, &subs->dest, + subs); __end: if (sport) snd_seq_port_unlock(sport); diff --git a/sound/core/seq/seq_ports.c b/sound/core/seq/seq_ports.c index 1e2239240f21..d964d728681e 100644 --- a/sound/core/seq/seq_ports.c +++ b/sound/core/seq/seq_ports.c @@ -632,20 +632,23 @@ int snd_seq_port_disconnect(struct snd_seq_client *connector, /* get matched subscriber */ -struct snd_seq_subscribers *snd_seq_port_get_subscription(struct snd_seq_port_subs_info *src_grp, - struct snd_seq_addr *dest_addr) +int snd_seq_port_get_subscription(struct snd_seq_port_subs_info *src_grp, + struct snd_seq_addr *dest_addr, + struct snd_seq_port_subscribe *subs) { - struct snd_seq_subscribers *s, *found = NULL; + struct snd_seq_subscribers *s; + int err = -ENOENT; down_read(&src_grp->list_mutex); list_for_each_entry(s, &src_grp->list_head, src_list) { if (addr_match(dest_addr, &s->info.dest)) { - found = s; + *subs = s->info; + err = 0; break; } } up_read(&src_grp->list_mutex); - return found; + return err; } /* diff --git a/sound/core/seq/seq_ports.h b/sound/core/seq/seq_ports.h index 26bd71f36c41..06003b36652e 100644 --- a/sound/core/seq/seq_ports.h +++ b/sound/core/seq/seq_ports.h @@ -135,7 +135,8 @@ int snd_seq_port_subscribe(struct snd_seq_client_port *port, struct snd_seq_port_subscribe *info); /* get matched subscriber */ -struct snd_seq_subscribers *snd_seq_port_get_subscription(struct snd_seq_port_subs_info *src_grp, - struct snd_seq_addr *dest_addr); +int snd_seq_port_get_subscription(struct snd_seq_port_subs_info *src_grp, + struct snd_seq_addr *dest_addr, + struct snd_seq_port_subscribe *subs); #endif -- cgit v1.2.3 From f0654ba94e33699b295ce4f3dc73094db6209035 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Thu, 11 Apr 2019 19:58:32 +0200 Subject: Revert "ALSA: seq: Protect in-kernel ioctl calls with mutex" This reverts commit feb689025fbb6f0aa6297d3ddf97de945ea4ad32. The fix attempt was incorrect, leading to the mutex deadlock through the close of OSS sequencer client. The proper fix needs more consideration, so let's revert it now. Fixes: feb689025fbb ("ALSA: seq: Protect in-kernel ioctl calls with mutex") Reported-by: syzbot+47ded6c0f23016cde310@syzkaller.appspotmail.com Signed-off-by: Takashi Iwai --- sound/core/seq/seq_clientmgr.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) (limited to 'sound/core/seq') diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index f256704dc401..de320b1b90de 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -2334,19 +2334,14 @@ int snd_seq_kernel_client_ctl(int clientid, unsigned int cmd, void *arg) { const struct ioctl_handler *handler; struct snd_seq_client *client; - int err; client = clientptr(clientid); if (client == NULL) return -ENXIO; for (handler = ioctl_handlers; handler->cmd > 0; ++handler) { - if (handler->cmd == cmd) { - mutex_lock(&client->ioctl_mutex); - err = handler->func(client, arg); - mutex_unlock(&client->ioctl_mutex); - return err; - } + if (handler->cmd == cmd) + return handler->func(client, arg); } pr_debug("ALSA: seq unknown ioctl() 0x%x (type='%c', number=0x%02x)\n", -- cgit v1.2.3 From 7c32ae35fbf9cffb7aa3736f44dec10c944ca18e Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Fri, 12 Apr 2019 11:37:19 +0200 Subject: ALSA: seq: Cover unsubscribe_port() in list_mutex The call of unsubscribe_port() which manages the group count and module refcount from delete_and_unsubscribe_port() looks racy; it's not covered by the group list lock, and it's likely a cause of the reported unbalance at port deletion. Let's move the call inside the group list_mutex to plug the hole. Reported-by: syzbot+e4c8abb920efa77bace9@syzkaller.appspotmail.com Signed-off-by: Takashi Iwai --- sound/core/seq/seq_ports.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'sound/core/seq') diff --git a/sound/core/seq/seq_ports.c b/sound/core/seq/seq_ports.c index d964d728681e..ac7556ab531c 100644 --- a/sound/core/seq/seq_ports.c +++ b/sound/core/seq/seq_ports.c @@ -547,10 +547,10 @@ static void delete_and_unsubscribe_port(struct snd_seq_client *client, list_del_init(list); grp->exclusive = 0; write_unlock_irq(&grp->list_lock); - up_write(&grp->list_mutex); if (!empty) unsubscribe_port(client, port, grp, &subs->info, ack); + up_write(&grp->list_mutex); } /* connect two ports */ -- cgit v1.2.3 From 6740ea6776e97274627e3c261469d1c4ba0a0cb5 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Fri, 12 Apr 2019 12:10:14 +0200 Subject: ALSA: seq: Simplify snd_seq_kernel_client_enqueue() helper We have two helpers for queuing a sequencer event from the kernel client, and both are used only from OSS sequencer layer without any hop and atomic set. Let's simplify and unify two helpers into one. No functional change, just a call pattern change. Signed-off-by: Takashi Iwai --- include/sound/seq_kernel.h | 3 ++- sound/core/seq/oss/seq_oss_rw.c | 11 ++++------- sound/core/seq/oss/seq_oss_writeq.c | 2 +- sound/core/seq/seq_clientmgr.c | 37 +++++++------------------------------ sound/core/seq/seq_clientmgr.h | 4 ---- 5 files changed, 14 insertions(+), 43 deletions(-) (limited to 'sound/core/seq') diff --git a/include/sound/seq_kernel.h b/include/sound/seq_kernel.h index 4b9ee3009aa0..c7a5433e109a 100644 --- a/include/sound/seq_kernel.h +++ b/include/sound/seq_kernel.h @@ -73,7 +73,8 @@ __printf(3, 4) int snd_seq_create_kernel_client(struct snd_card *card, int client_index, const char *name_fmt, ...); int snd_seq_delete_kernel_client(int client); -int snd_seq_kernel_client_enqueue(int client, struct snd_seq_event *ev, int atomic, int hop); +int snd_seq_kernel_client_enqueue(int client, struct snd_seq_event *ev, + struct file *file, bool blocking); int snd_seq_kernel_client_dispatch(int client, struct snd_seq_event *ev, int atomic, int hop); int snd_seq_kernel_client_ctl(int client, unsigned int cmd, void *arg); diff --git a/sound/core/seq/oss/seq_oss_rw.c b/sound/core/seq/oss/seq_oss_rw.c index 30886f5fb100..eb1ef12181f3 100644 --- a/sound/core/seq/oss/seq_oss_rw.c +++ b/sound/core/seq/oss/seq_oss_rw.c @@ -180,14 +180,11 @@ insert_queue(struct seq_oss_devinfo *dp, union evrec *rec, struct file *opt) return 0; /* invalid event - no need to insert queue */ event.time.tick = snd_seq_oss_timer_cur_tick(dp->timer); - if (dp->timer->realtime || !dp->timer->running) { + if (dp->timer->realtime || !dp->timer->running) snd_seq_oss_dispatch(dp, &event, 0, 0); - } else { - if (is_nonblock_mode(dp->file_mode)) - rc = snd_seq_kernel_client_enqueue(dp->cseq, &event, 0, 0); - else - rc = snd_seq_kernel_client_enqueue_blocking(dp->cseq, &event, opt, 0, 0); - } + else + rc = snd_seq_kernel_client_enqueue(dp->cseq, &event, opt, + !is_nonblock_mode(dp->file_mode)); return rc; } diff --git a/sound/core/seq/oss/seq_oss_writeq.c b/sound/core/seq/oss/seq_oss_writeq.c index 5e04f4df10e4..b2f69617591f 100644 --- a/sound/core/seq/oss/seq_oss_writeq.c +++ b/sound/core/seq/oss/seq_oss_writeq.c @@ -116,7 +116,7 @@ snd_seq_oss_writeq_sync(struct seq_oss_writeq *q) rec->t.code = SEQ_SYNCTIMER; rec->t.time = time; q->sync_event_put = 1; - snd_seq_kernel_client_enqueue_blocking(dp->cseq, &ev, NULL, 0, 0); + snd_seq_kernel_client_enqueue(dp->cseq, &ev, NULL, true); } wait_event_interruptible_timeout(q->sync_sleep, ! q->sync_event_put, HZ); diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index de320b1b90de..0af5b1440b33 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -2218,12 +2218,13 @@ int snd_seq_delete_kernel_client(int client) } EXPORT_SYMBOL(snd_seq_delete_kernel_client); -/* skeleton to enqueue event, called from snd_seq_kernel_client_enqueue - * and snd_seq_kernel_client_enqueue_blocking +/* + * exported, called by kernel clients to enqueue events (w/o blocking) + * + * RETURN VALUE: zero if succeed, negative if error */ -static int kernel_client_enqueue(int client, struct snd_seq_event *ev, - struct file *file, int blocking, - int atomic, int hop) +int snd_seq_kernel_client_enqueue(int client, struct snd_seq_event *ev, + struct file *file, bool blocking) { struct snd_seq_client *cptr; int result; @@ -2250,37 +2251,13 @@ static int kernel_client_enqueue(int client, struct snd_seq_event *ev, result = -EPERM; else /* send it */ result = snd_seq_client_enqueue_event(cptr, ev, file, blocking, - atomic, hop, NULL); + false, 0, NULL); snd_seq_client_unlock(cptr); return result; } - -/* - * exported, called by kernel clients to enqueue events (w/o blocking) - * - * RETURN VALUE: zero if succeed, negative if error - */ -int snd_seq_kernel_client_enqueue(int client, struct snd_seq_event * ev, - int atomic, int hop) -{ - return kernel_client_enqueue(client, ev, NULL, 0, atomic, hop); -} EXPORT_SYMBOL(snd_seq_kernel_client_enqueue); -/* - * exported, called by kernel clients to enqueue events (with blocking) - * - * RETURN VALUE: zero if succeed, negative if error - */ -int snd_seq_kernel_client_enqueue_blocking(int client, struct snd_seq_event * ev, - struct file *file, - int atomic, int hop) -{ - return kernel_client_enqueue(client, ev, file, 1, atomic, hop); -} -EXPORT_SYMBOL(snd_seq_kernel_client_enqueue_blocking); - /* * exported, called by kernel clients to dispatch events directly to other * clients, bypassing the queues. Event time-stamp will be updated. diff --git a/sound/core/seq/seq_clientmgr.h b/sound/core/seq/seq_clientmgr.h index 0611e1e0ed5b..66ad3d547916 100644 --- a/sound/core/seq/seq_clientmgr.h +++ b/sound/core/seq/seq_clientmgr.h @@ -93,10 +93,6 @@ struct snd_seq_client *snd_seq_client_use_ptr(int clientid); /* dispatch event to client(s) */ int snd_seq_dispatch_event(struct snd_seq_event_cell *cell, int atomic, int hop); -/* exported to other modules */ -int snd_seq_kernel_client_enqueue(int client, struct snd_seq_event *ev, int atomic, int hop); -int snd_seq_kernel_client_enqueue_blocking(int client, struct snd_seq_event * ev, - struct file *file, int atomic, int hop); int snd_seq_kernel_client_write_poll(int clientid, struct file *file, poll_table *wait); int snd_seq_client_notify_subscription(int client, int port, struct snd_seq_port_subscribe *info, int evtype); -- cgit v1.2.3 From 6b580f523172f2c738b661069a57c23c74a75f88 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Fri, 12 Apr 2019 12:44:39 +0200 Subject: ALSA: seq: Protect racy pool manipulation from OSS sequencer OSS sequencer emulation still allows to queue and issue the events that manipulate the client pool concurrently in a racy way. This patch serializes the access like the normal sequencer write / ioctl via taking the client ioctl_mutex. Since the access to the sequencer client is done indirectly via a client id number, a new helper to take/release the mutex is introduced. Signed-off-by: Takashi Iwai --- sound/core/seq/oss/seq_oss_device.h | 10 ++++++++-- sound/core/seq/seq_clientmgr.c | 40 ++++++++++++++++++++++++++++++++++--- sound/core/seq/seq_clientmgr.h | 4 ++++ 3 files changed, 49 insertions(+), 5 deletions(-) (limited to 'sound/core/seq') diff --git a/sound/core/seq/oss/seq_oss_device.h b/sound/core/seq/oss/seq_oss_device.h index 2d0e9eaf13aa..77eb1fe1155c 100644 --- a/sound/core/seq/oss/seq_oss_device.h +++ b/sound/core/seq/oss/seq_oss_device.h @@ -30,6 +30,7 @@ #include #include #include +#include "../seq_clientmgr.h" /* max. applications */ #define SNDRV_SEQ_OSS_MAX_CLIENTS 16 @@ -150,11 +151,16 @@ snd_seq_oss_dispatch(struct seq_oss_devinfo *dp, struct snd_seq_event *ev, int a return snd_seq_kernel_client_dispatch(dp->cseq, ev, atomic, hop); } -/* ioctl */ +/* ioctl for writeq */ static inline int snd_seq_oss_control(struct seq_oss_devinfo *dp, unsigned int type, void *arg) { - return snd_seq_kernel_client_ctl(dp->cseq, type, arg); + int err; + + snd_seq_client_ioctl_lock(dp->cseq); + err = snd_seq_kernel_client_ctl(dp->cseq, type, arg); + snd_seq_client_ioctl_unlock(dp->cseq); + return err; } /* fill the addresses in header */ diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index 0af5b1440b33..a5c9d59eb5b8 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -179,6 +179,36 @@ struct snd_seq_client *snd_seq_client_use_ptr(int clientid) return client; } +/* Take refcount and perform ioctl_mutex lock on the given client; + * used only for OSS sequencer + * Unlock via snd_seq_client_ioctl_unlock() below + */ +bool snd_seq_client_ioctl_lock(int clientid) +{ + struct snd_seq_client *client; + + client = snd_seq_client_use_ptr(clientid); + if (!client) + return false; + mutex_lock(&client->ioctl_mutex); + return true; +} +EXPORT_SYMBOL_GPL(snd_seq_client_ioctl_lock); + +/* Unlock and unref the given client; for OSS sequencer use only */ +void snd_seq_client_ioctl_unlock(int clientid) +{ + struct snd_seq_client *client; + + client = snd_seq_client_use_ptr(clientid); + if (WARN_ON(!client)) + return; + mutex_unlock(&client->ioctl_mutex); + snd_use_lock_free(&client->use_lock); + snd_seq_client_unlock(client); +} +EXPORT_SYMBOL_GPL(snd_seq_client_ioctl_unlock); + static void usage_alloc(struct snd_seq_usage *res, int num) { res->cur += num; @@ -2247,11 +2277,15 @@ int snd_seq_kernel_client_enqueue(int client, struct snd_seq_event *ev, if (cptr == NULL) return -EINVAL; - if (! cptr->accept_output) + if (!cptr->accept_output) { result = -EPERM; - else /* send it */ + } else { /* send it */ + mutex_lock(&cptr->ioctl_mutex); result = snd_seq_client_enqueue_event(cptr, ev, file, blocking, - false, 0, NULL); + false, 0, + &cptr->ioctl_mutex); + mutex_unlock(&cptr->ioctl_mutex); + } snd_seq_client_unlock(cptr); return result; diff --git a/sound/core/seq/seq_clientmgr.h b/sound/core/seq/seq_clientmgr.h index 66ad3d547916..28a51dcc0190 100644 --- a/sound/core/seq/seq_clientmgr.h +++ b/sound/core/seq/seq_clientmgr.h @@ -97,6 +97,10 @@ int snd_seq_kernel_client_write_poll(int clientid, struct file *file, poll_table int snd_seq_client_notify_subscription(int client, int port, struct snd_seq_port_subscribe *info, int evtype); +/* only for OSS sequencer */ +bool snd_seq_client_ioctl_lock(int clientid); +void snd_seq_client_ioctl_unlock(int clientid); + extern int seq_client_load[15]; #endif -- cgit v1.2.3 From b5fd12d6c0fc64c2c2b5ae095e63824083d27151 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Mon, 15 Apr 2019 09:03:01 +0200 Subject: ALSA: seq: Correct unlock sequence at snd_seq_client_ioctl_unlock() The doubly unlock sequence at snd_seq_client_ioctl_unlock() is tricky. I took a direct unref call since I thought it would avoid misunderstanding, but rather this seems more confusing. Let's use snd_seq_client_unlock() consistently even if they look strange to be called twice, and add more comments for avoiding reader's confusion. Fixes: 6b580f523172 ("ALSA: seq: Protect racy pool manipulation from OSS sequencer") Reviewed-by: Kai Vehmanen Signed-off-by: Takashi Iwai --- sound/core/seq/seq_clientmgr.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'sound/core/seq') diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index 3acd80e718f2..c0227a672442 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -191,6 +191,7 @@ bool snd_seq_client_ioctl_lock(int clientid) if (!client) return false; mutex_lock(&client->ioctl_mutex); + /* The client isn't unrefed here; see snd_seq_client_ioctl_unlock() */ return true; } EXPORT_SYMBOL_GPL(snd_seq_client_ioctl_lock); @@ -204,7 +205,11 @@ void snd_seq_client_ioctl_unlock(int clientid) if (WARN_ON(!client)) return; mutex_unlock(&client->ioctl_mutex); - snd_use_lock_free(&client->use_lock); + /* The doubly unrefs below are intentional; the first one releases the + * leftover from snd_seq_client_ioctl_lock() above, and the second one + * is for releasing snd_seq_client_use_ptr() in this function + */ + snd_seq_client_unlock(client); snd_seq_client_unlock(client); } EXPORT_SYMBOL_GPL(snd_seq_client_ioctl_unlock); -- cgit v1.2.3