Commit 6b1ca4bc authored by Takashi Sakamoto's avatar Takashi Sakamoto Committed by Takashi Iwai

ALSA: fireworks: accessing to user space outside spinlock

In hwdep interface of fireworks driver, accessing to user space is in a
critical section with disabled local interrupt. Depending on architecture,
accessing to user space can cause page fault exception. Then local
processor stores machine status and handles the synchronous event. A
handler corresponding to the event can call task scheduler to wait for
preparing pages. In a case of usage of single core processor, the state to
disable local interrupt is worse because it don't handle usual interrupts
from hardware.

This commit fixes this bug, performing the accessing outside spinlock. This
commit also gives up counting the number of queued response messages to
simplify ring-buffer management.
Reported-by: default avatarVaishali Thakkar <vaishali.thakkar@oracle.com>
Cc: stable@vger.kernel.org
Fixes: 555e8a8f('ALSA: fireworks: Add command/response functionality into hwdep interface')
Signed-off-by: default avatarTakashi Sakamoto <o-takashi@sakamocchi.jp>
Signed-off-by: default avatarTakashi Iwai <tiwai@suse.de>
parent 04b2d9c9
...@@ -108,7 +108,6 @@ struct snd_efw { ...@@ -108,7 +108,6 @@ struct snd_efw {
u8 *resp_buf; u8 *resp_buf;
u8 *pull_ptr; u8 *pull_ptr;
u8 *push_ptr; u8 *push_ptr;
unsigned int resp_queues;
}; };
int snd_efw_transaction_cmd(struct fw_unit *unit, int snd_efw_transaction_cmd(struct fw_unit *unit,
......
...@@ -25,6 +25,7 @@ hwdep_read_resp_buf(struct snd_efw *efw, char __user *buf, long remained, ...@@ -25,6 +25,7 @@ hwdep_read_resp_buf(struct snd_efw *efw, char __user *buf, long remained,
{ {
unsigned int length, till_end, type; unsigned int length, till_end, type;
struct snd_efw_transaction *t; struct snd_efw_transaction *t;
u8 *pull_ptr;
long count = 0; long count = 0;
if (remained < sizeof(type) + sizeof(struct snd_efw_transaction)) if (remained < sizeof(type) + sizeof(struct snd_efw_transaction))
...@@ -38,8 +39,17 @@ hwdep_read_resp_buf(struct snd_efw *efw, char __user *buf, long remained, ...@@ -38,8 +39,17 @@ hwdep_read_resp_buf(struct snd_efw *efw, char __user *buf, long remained,
buf += sizeof(type); buf += sizeof(type);
/* write into buffer as many responses as possible */ /* write into buffer as many responses as possible */
while (efw->resp_queues > 0) { spin_lock_irq(&efw->lock);
t = (struct snd_efw_transaction *)(efw->pull_ptr);
/*
* When another task reaches here during this task's access to user
* space, it picks up current position in buffer and can read the same
* series of responses.
*/
pull_ptr = efw->pull_ptr;
while (efw->push_ptr != pull_ptr) {
t = (struct snd_efw_transaction *)(pull_ptr);
length = be32_to_cpu(t->length) * sizeof(__be32); length = be32_to_cpu(t->length) * sizeof(__be32);
/* confirm enough space for this response */ /* confirm enough space for this response */
...@@ -49,26 +59,39 @@ hwdep_read_resp_buf(struct snd_efw *efw, char __user *buf, long remained, ...@@ -49,26 +59,39 @@ hwdep_read_resp_buf(struct snd_efw *efw, char __user *buf, long remained,
/* copy from ring buffer to user buffer */ /* copy from ring buffer to user buffer */
while (length > 0) { while (length > 0) {
till_end = snd_efw_resp_buf_size - till_end = snd_efw_resp_buf_size -
(unsigned int)(efw->pull_ptr - efw->resp_buf); (unsigned int)(pull_ptr - efw->resp_buf);
till_end = min_t(unsigned int, length, till_end); till_end = min_t(unsigned int, length, till_end);
if (copy_to_user(buf, efw->pull_ptr, till_end)) spin_unlock_irq(&efw->lock);
if (copy_to_user(buf, pull_ptr, till_end))
return -EFAULT; return -EFAULT;
efw->pull_ptr += till_end; spin_lock_irq(&efw->lock);
if (efw->pull_ptr >= efw->resp_buf +
snd_efw_resp_buf_size) pull_ptr += till_end;
efw->pull_ptr -= snd_efw_resp_buf_size; if (pull_ptr >= efw->resp_buf + snd_efw_resp_buf_size)
pull_ptr -= snd_efw_resp_buf_size;
length -= till_end; length -= till_end;
buf += till_end; buf += till_end;
count += till_end; count += till_end;
remained -= till_end; remained -= till_end;
} }
efw->resp_queues--;
} }
/*
* All of tasks can read from the buffer nearly simultaneously, but the
* last position for each task is different depending on the length of
* given buffer. Here, for simplicity, a position of buffer is set by
* the latest task. It's better for a listening application to allow one
* thread to read from the buffer. Unless, each task can read different
* sequence of responses depending on variation of buffer length.
*/
efw->pull_ptr = pull_ptr;
spin_unlock_irq(&efw->lock);
return count; return count;
} }
...@@ -76,14 +99,17 @@ static long ...@@ -76,14 +99,17 @@ static long
hwdep_read_locked(struct snd_efw *efw, char __user *buf, long count, hwdep_read_locked(struct snd_efw *efw, char __user *buf, long count,
loff_t *offset) loff_t *offset)
{ {
union snd_firewire_event event; union snd_firewire_event event = {
.lock_status.type = SNDRV_FIREWIRE_EVENT_LOCK_STATUS,
};
memset(&event, 0, sizeof(event)); spin_lock_irq(&efw->lock);
event.lock_status.type = SNDRV_FIREWIRE_EVENT_LOCK_STATUS;
event.lock_status.status = (efw->dev_lock_count > 0); event.lock_status.status = (efw->dev_lock_count > 0);
efw->dev_lock_changed = false; efw->dev_lock_changed = false;
spin_unlock_irq(&efw->lock);
count = min_t(long, count, sizeof(event.lock_status)); count = min_t(long, count, sizeof(event.lock_status));
if (copy_to_user(buf, &event, count)) if (copy_to_user(buf, &event, count))
...@@ -98,10 +124,15 @@ hwdep_read(struct snd_hwdep *hwdep, char __user *buf, long count, ...@@ -98,10 +124,15 @@ hwdep_read(struct snd_hwdep *hwdep, char __user *buf, long count,
{ {
struct snd_efw *efw = hwdep->private_data; struct snd_efw *efw = hwdep->private_data;
DEFINE_WAIT(wait); DEFINE_WAIT(wait);
bool dev_lock_changed;
bool queued;
spin_lock_irq(&efw->lock); spin_lock_irq(&efw->lock);
while ((!efw->dev_lock_changed) && (efw->resp_queues == 0)) { dev_lock_changed = efw->dev_lock_changed;
queued = efw->push_ptr != efw->pull_ptr;
while (!dev_lock_changed && !queued) {
prepare_to_wait(&efw->hwdep_wait, &wait, TASK_INTERRUPTIBLE); prepare_to_wait(&efw->hwdep_wait, &wait, TASK_INTERRUPTIBLE);
spin_unlock_irq(&efw->lock); spin_unlock_irq(&efw->lock);
schedule(); schedule();
...@@ -109,15 +140,17 @@ hwdep_read(struct snd_hwdep *hwdep, char __user *buf, long count, ...@@ -109,15 +140,17 @@ hwdep_read(struct snd_hwdep *hwdep, char __user *buf, long count,
if (signal_pending(current)) if (signal_pending(current))
return -ERESTARTSYS; return -ERESTARTSYS;
spin_lock_irq(&efw->lock); spin_lock_irq(&efw->lock);
dev_lock_changed = efw->dev_lock_changed;
queued = efw->push_ptr != efw->pull_ptr;
} }
if (efw->dev_lock_changed) spin_unlock_irq(&efw->lock);
if (dev_lock_changed)
count = hwdep_read_locked(efw, buf, count, offset); count = hwdep_read_locked(efw, buf, count, offset);
else if (efw->resp_queues > 0) else if (queued)
count = hwdep_read_resp_buf(efw, buf, count, offset); count = hwdep_read_resp_buf(efw, buf, count, offset);
spin_unlock_irq(&efw->lock);
return count; return count;
} }
...@@ -160,7 +193,7 @@ hwdep_poll(struct snd_hwdep *hwdep, struct file *file, poll_table *wait) ...@@ -160,7 +193,7 @@ hwdep_poll(struct snd_hwdep *hwdep, struct file *file, poll_table *wait)
poll_wait(file, &efw->hwdep_wait, wait); poll_wait(file, &efw->hwdep_wait, wait);
spin_lock_irq(&efw->lock); spin_lock_irq(&efw->lock);
if (efw->dev_lock_changed || (efw->resp_queues > 0)) if (efw->dev_lock_changed || efw->pull_ptr != efw->push_ptr)
events = POLLIN | POLLRDNORM; events = POLLIN | POLLRDNORM;
else else
events = 0; events = 0;
......
...@@ -188,8 +188,8 @@ proc_read_queues_state(struct snd_info_entry *entry, ...@@ -188,8 +188,8 @@ proc_read_queues_state(struct snd_info_entry *entry,
else else
consumed = (unsigned int)(efw->push_ptr - efw->pull_ptr); consumed = (unsigned int)(efw->push_ptr - efw->pull_ptr);
snd_iprintf(buffer, "%d %d/%d\n", snd_iprintf(buffer, "%d/%d\n",
efw->resp_queues, consumed, snd_efw_resp_buf_size); consumed, snd_efw_resp_buf_size);
} }
static void static void
......
...@@ -121,11 +121,11 @@ copy_resp_to_buf(struct snd_efw *efw, void *data, size_t length, int *rcode) ...@@ -121,11 +121,11 @@ copy_resp_to_buf(struct snd_efw *efw, void *data, size_t length, int *rcode)
size_t capacity, till_end; size_t capacity, till_end;
struct snd_efw_transaction *t; struct snd_efw_transaction *t;
spin_lock_irq(&efw->lock);
t = (struct snd_efw_transaction *)data; t = (struct snd_efw_transaction *)data;
length = min_t(size_t, be32_to_cpu(t->length) * sizeof(u32), length); length = min_t(size_t, be32_to_cpu(t->length) * sizeof(u32), length);
spin_lock_irq(&efw->lock);
if (efw->push_ptr < efw->pull_ptr) if (efw->push_ptr < efw->pull_ptr)
capacity = (unsigned int)(efw->pull_ptr - efw->push_ptr); capacity = (unsigned int)(efw->pull_ptr - efw->push_ptr);
else else
...@@ -155,7 +155,6 @@ copy_resp_to_buf(struct snd_efw *efw, void *data, size_t length, int *rcode) ...@@ -155,7 +155,6 @@ copy_resp_to_buf(struct snd_efw *efw, void *data, size_t length, int *rcode)
} }
/* for hwdep */ /* for hwdep */
efw->resp_queues++;
wake_up(&efw->hwdep_wait); wake_up(&efw->hwdep_wait);
*rcode = RCODE_COMPLETE; *rcode = RCODE_COMPLETE;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment