[PATCH 0/5] ALSA: rawmidi: Make code robust for external calls
Hi,
here is a small patch set for ALSA core rawmidi code to make the code a bit more robust, especially for the case where the exported functions get called from the external drivers. Currently most of those functions assume naively that they aren't called at a wrong timing. With the patch set, it tries to harden a bit not to hit serious breakage.
Takashi
===
Takashi Iwai (5): ALSA: rawmidi: Make internal functions local static ALSA: rawmidi: Move lock to snd_rawmidi_substream ALSA: rawmidi: Take open_mutex around parameter changes ALSA: rawmidi: Check stream state at exported functions ALSA: rawmidi: Take buffer refcount while draining output
include/sound/rawmidi.h | 6 +- sound/core/rawmidi.c | 274 +++++++++++++++++++++++++--------------- 2 files changed, 170 insertions(+), 110 deletions(-)
__snd_rawmidi_transmit_peek() and __snd_rawmidi_transmit_ack() are never called from the outside. Let's make them local static and unexport them.
Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/rawmidi.h | 4 ---- sound/core/rawmidi.c | 13 ++++++------- 2 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/include/sound/rawmidi.h b/include/sound/rawmidi.h index 7a08ed2acd60..9402c25ae9ba 100644 --- a/include/sound/rawmidi.h +++ b/include/sound/rawmidi.h @@ -156,10 +156,6 @@ int snd_rawmidi_transmit_peek(struct snd_rawmidi_substream *substream, int snd_rawmidi_transmit_ack(struct snd_rawmidi_substream *substream, int count); int snd_rawmidi_transmit(struct snd_rawmidi_substream *substream, unsigned char *buffer, int count); -int __snd_rawmidi_transmit_peek(struct snd_rawmidi_substream *substream, - unsigned char *buffer, int count); -int __snd_rawmidi_transmit_ack(struct snd_rawmidi_substream *substream, - int count); int snd_rawmidi_proceed(struct snd_rawmidi_substream *substream);
/* main midi functions */ diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c index befa9809ff00..82e8f656bbb2 100644 --- a/sound/core/rawmidi.c +++ b/sound/core/rawmidi.c @@ -1258,7 +1258,7 @@ int snd_rawmidi_transmit_empty(struct snd_rawmidi_substream *substream) } EXPORT_SYMBOL(snd_rawmidi_transmit_empty);
-/** +/* * __snd_rawmidi_transmit_peek - copy data from the internal buffer * @substream: the rawmidi substream * @buffer: the buffer pointer @@ -1266,8 +1266,8 @@ EXPORT_SYMBOL(snd_rawmidi_transmit_empty); * * This is a variant of snd_rawmidi_transmit_peek() without spinlock. */ -int __snd_rawmidi_transmit_peek(struct snd_rawmidi_substream *substream, - unsigned char *buffer, int count) +static int __snd_rawmidi_transmit_peek(struct snd_rawmidi_substream *substream, + unsigned char *buffer, int count) { int result, count1; struct snd_rawmidi_runtime *runtime = substream->runtime; @@ -1304,7 +1304,6 @@ int __snd_rawmidi_transmit_peek(struct snd_rawmidi_substream *substream, __skip: return result; } -EXPORT_SYMBOL(__snd_rawmidi_transmit_peek);
/** * snd_rawmidi_transmit_peek - copy data from the internal buffer @@ -1334,14 +1333,15 @@ int snd_rawmidi_transmit_peek(struct snd_rawmidi_substream *substream, } EXPORT_SYMBOL(snd_rawmidi_transmit_peek);
-/** +/* * __snd_rawmidi_transmit_ack - acknowledge the transmission * @substream: the rawmidi substream * @count: the transferred count * * This is a variant of __snd_rawmidi_transmit_ack() without spinlock. */ -int __snd_rawmidi_transmit_ack(struct snd_rawmidi_substream *substream, int count) +static int __snd_rawmidi_transmit_ack(struct snd_rawmidi_substream *substream, + int count) { struct snd_rawmidi_runtime *runtime = substream->runtime;
@@ -1361,7 +1361,6 @@ int __snd_rawmidi_transmit_ack(struct snd_rawmidi_substream *substream, int coun } return count; } -EXPORT_SYMBOL(__snd_rawmidi_transmit_ack);
/** * snd_rawmidi_transmit_ack - acknowledge the transmission
Having a lock in snd_rawmidi_runtime can be a problem especially when a substream is accessed from the outside, as the runtime creation might be racy with the external calls. As a first step for hardening, move the spinlock from snd_rawmidi_runtime to snd_rawmidi_substream.
This patch just replaces the lock calls, no real functional change is put yet.
Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/rawmidi.h | 2 +- sound/core/rawmidi.c | 131 ++++++++++++++++++++-------------------- 2 files changed, 65 insertions(+), 68 deletions(-)
diff --git a/include/sound/rawmidi.h b/include/sound/rawmidi.h index 9402c25ae9ba..e1f59b2940af 100644 --- a/include/sound/rawmidi.h +++ b/include/sound/rawmidi.h @@ -63,7 +63,6 @@ struct snd_rawmidi_runtime { size_t xruns; /* over/underruns counter */ int buffer_ref; /* buffer reference count */ /* misc */ - spinlock_t lock; wait_queue_head_t sleep; /* event handler (new bytes, input only) */ void (*event)(struct snd_rawmidi_substream *substream); @@ -85,6 +84,7 @@ struct snd_rawmidi_substream { unsigned int clock_type; /* clock source to use for input framing */ int use_count; /* use counter (for output) */ size_t bytes; + spinlock_t lock; struct snd_rawmidi *rmidi; struct snd_rawmidi_str *pstr; char name[32]; diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c index 82e8f656bbb2..0a00f37d8c42 100644 --- a/sound/core/rawmidi.c +++ b/sound/core/rawmidi.c @@ -102,13 +102,12 @@ static inline bool __snd_rawmidi_ready(struct snd_rawmidi_runtime *runtime)
static bool snd_rawmidi_ready(struct snd_rawmidi_substream *substream) { - struct snd_rawmidi_runtime *runtime = substream->runtime; unsigned long flags; bool ready;
- spin_lock_irqsave(&runtime->lock, flags); - ready = __snd_rawmidi_ready(runtime); - spin_unlock_irqrestore(&runtime->lock, flags); + spin_lock_irqsave(&substream->lock, flags); + ready = __snd_rawmidi_ready(substream->runtime); + spin_unlock_irqrestore(&substream->lock, flags); return ready; }
@@ -130,7 +129,7 @@ static void snd_rawmidi_input_event_work(struct work_struct *work) runtime->event(runtime->substream); }
-/* buffer refcount management: call with runtime->lock held */ +/* buffer refcount management: call with substream->lock held */ static inline void snd_rawmidi_buffer_ref(struct snd_rawmidi_runtime *runtime) { runtime->buffer_ref++; @@ -149,7 +148,6 @@ static int snd_rawmidi_runtime_create(struct snd_rawmidi_substream *substream) if (!runtime) return -ENOMEM; runtime->substream = substream; - spin_lock_init(&runtime->lock); init_waitqueue_head(&runtime->sleep); INIT_WORK(&runtime->event_work, snd_rawmidi_input_event_work); runtime->event = NULL; @@ -203,20 +201,20 @@ static void __reset_runtime_ptrs(struct snd_rawmidi_runtime *runtime, runtime->avail = is_input ? 0 : runtime->buffer_size; }
-static void reset_runtime_ptrs(struct snd_rawmidi_runtime *runtime, +static void reset_runtime_ptrs(struct snd_rawmidi_substream *substream, bool is_input) { unsigned long flags;
- spin_lock_irqsave(&runtime->lock, flags); - __reset_runtime_ptrs(runtime, is_input); - spin_unlock_irqrestore(&runtime->lock, flags); + spin_lock_irqsave(&substream->lock, flags); + __reset_runtime_ptrs(substream->runtime, is_input); + spin_unlock_irqrestore(&substream->lock, flags); }
int snd_rawmidi_drop_output(struct snd_rawmidi_substream *substream) { snd_rawmidi_output_trigger(substream, 0); - reset_runtime_ptrs(substream->runtime, false); + reset_runtime_ptrs(substream, false); return 0; } EXPORT_SYMBOL(snd_rawmidi_drop_output); @@ -256,7 +254,7 @@ EXPORT_SYMBOL(snd_rawmidi_drain_output); int snd_rawmidi_drain_input(struct snd_rawmidi_substream *substream) { snd_rawmidi_input_trigger(substream, 0); - reset_runtime_ptrs(substream->runtime, true); + reset_runtime_ptrs(substream, true); return 0; } EXPORT_SYMBOL(snd_rawmidi_drain_input); @@ -676,10 +674,11 @@ static int snd_rawmidi_info_select_user(struct snd_card *card, return 0; }
-static int resize_runtime_buffer(struct snd_rawmidi_runtime *runtime, +static int resize_runtime_buffer(struct snd_rawmidi_substream *substream, struct snd_rawmidi_params *params, bool is_input) { + struct snd_rawmidi_runtime *runtime = substream->runtime; char *newbuf, *oldbuf; unsigned int framing = params->mode & SNDRV_RAWMIDI_MODE_FRAMING_MASK;
@@ -693,9 +692,9 @@ static int resize_runtime_buffer(struct snd_rawmidi_runtime *runtime, newbuf = kvzalloc(params->buffer_size, GFP_KERNEL); if (!newbuf) return -ENOMEM; - spin_lock_irq(&runtime->lock); + spin_lock_irq(&substream->lock); if (runtime->buffer_ref) { - spin_unlock_irq(&runtime->lock); + spin_unlock_irq(&substream->lock); kvfree(newbuf); return -EBUSY; } @@ -703,7 +702,7 @@ static int resize_runtime_buffer(struct snd_rawmidi_runtime *runtime, runtime->buffer = newbuf; runtime->buffer_size = params->buffer_size; __reset_runtime_ptrs(runtime, is_input); - spin_unlock_irq(&runtime->lock); + spin_unlock_irq(&substream->lock); kvfree(oldbuf); } runtime->avail_min = params->avail_min; @@ -717,7 +716,7 @@ int snd_rawmidi_output_params(struct snd_rawmidi_substream *substream, return -EBUSY; snd_rawmidi_drain_output(substream); substream->active_sensing = !params->no_active_sensing; - return resize_runtime_buffer(substream->runtime, params, false); + return resize_runtime_buffer(substream, params, false); } EXPORT_SYMBOL(snd_rawmidi_output_params);
@@ -735,7 +734,7 @@ int snd_rawmidi_input_params(struct snd_rawmidi_substream *substream, if (framing > SNDRV_RAWMIDI_MODE_FRAMING_TSTAMP) return -EINVAL; snd_rawmidi_drain_input(substream); - err = resize_runtime_buffer(substream->runtime, params, true); + err = resize_runtime_buffer(substream, params, true); if (err < 0) return err;
@@ -752,9 +751,9 @@ static int snd_rawmidi_output_status(struct snd_rawmidi_substream *substream,
memset(status, 0, sizeof(*status)); status->stream = SNDRV_RAWMIDI_STREAM_OUTPUT; - spin_lock_irq(&runtime->lock); + spin_lock_irq(&substream->lock); status->avail = runtime->avail; - spin_unlock_irq(&runtime->lock); + spin_unlock_irq(&substream->lock); return 0; }
@@ -765,11 +764,11 @@ static int snd_rawmidi_input_status(struct snd_rawmidi_substream *substream,
memset(status, 0, sizeof(*status)); status->stream = SNDRV_RAWMIDI_STREAM_INPUT; - spin_lock_irq(&runtime->lock); + spin_lock_irq(&substream->lock); status->avail = runtime->avail; status->xruns = runtime->xruns; runtime->xruns = 0; - spin_unlock_irq(&runtime->lock); + spin_unlock_irq(&substream->lock); return 0; }
@@ -1074,7 +1073,7 @@ int snd_rawmidi_receive(struct snd_rawmidi_substream *substream, return -EINVAL; }
- spin_lock_irqsave(&runtime->lock, flags); + spin_lock_irqsave(&substream->lock, flags); if (substream->framing == SNDRV_RAWMIDI_MODE_FRAMING_TSTAMP) { result = receive_with_tstamp_framing(substream, buffer, count, &ts64); } else if (count == 1) { /* special case, faster code */ @@ -1121,7 +1120,7 @@ int snd_rawmidi_receive(struct snd_rawmidi_substream *substream, else if (__snd_rawmidi_ready(runtime)) wake_up(&runtime->sleep); } - spin_unlock_irqrestore(&runtime->lock, flags); + spin_unlock_irqrestore(&substream->lock, flags); return result; } EXPORT_SYMBOL(snd_rawmidi_receive); @@ -1136,7 +1135,7 @@ static long snd_rawmidi_kernel_read1(struct snd_rawmidi_substream *substream, unsigned long appl_ptr; int err = 0;
- spin_lock_irqsave(&runtime->lock, flags); + spin_lock_irqsave(&substream->lock, flags); snd_rawmidi_buffer_ref(runtime); while (count > 0 && runtime->avail) { count1 = runtime->buffer_size - runtime->appl_ptr; @@ -1154,11 +1153,11 @@ static long snd_rawmidi_kernel_read1(struct snd_rawmidi_substream *substream, if (kernelbuf) memcpy(kernelbuf + result, runtime->buffer + appl_ptr, count1); if (userbuf) { - spin_unlock_irqrestore(&runtime->lock, flags); + spin_unlock_irqrestore(&substream->lock, flags); if (copy_to_user(userbuf + result, runtime->buffer + appl_ptr, count1)) err = -EFAULT; - spin_lock_irqsave(&runtime->lock, flags); + spin_lock_irqsave(&substream->lock, flags); if (err) goto out; } @@ -1167,7 +1166,7 @@ static long snd_rawmidi_kernel_read1(struct snd_rawmidi_substream *substream, } out: snd_rawmidi_buffer_unref(runtime); - spin_unlock_irqrestore(&runtime->lock, flags); + spin_unlock_irqrestore(&substream->lock, flags); return result > 0 ? result : err; }
@@ -1196,31 +1195,31 @@ static ssize_t snd_rawmidi_read(struct file *file, char __user *buf, size_t coun snd_rawmidi_input_trigger(substream, 1); result = 0; while (count > 0) { - spin_lock_irq(&runtime->lock); + spin_lock_irq(&substream->lock); while (!__snd_rawmidi_ready(runtime)) { wait_queue_entry_t wait;
if ((file->f_flags & O_NONBLOCK) != 0 || result > 0) { - spin_unlock_irq(&runtime->lock); + spin_unlock_irq(&substream->lock); return result > 0 ? result : -EAGAIN; } init_waitqueue_entry(&wait, current); add_wait_queue(&runtime->sleep, &wait); set_current_state(TASK_INTERRUPTIBLE); - spin_unlock_irq(&runtime->lock); + spin_unlock_irq(&substream->lock); schedule(); remove_wait_queue(&runtime->sleep, &wait); if (rfile->rmidi->card->shutdown) return -ENODEV; if (signal_pending(current)) return result > 0 ? result : -ERESTARTSYS; - spin_lock_irq(&runtime->lock); + spin_lock_irq(&substream->lock); if (!runtime->avail) { - spin_unlock_irq(&runtime->lock); + spin_unlock_irq(&substream->lock); return result > 0 ? result : -EIO; } } - spin_unlock_irq(&runtime->lock); + spin_unlock_irq(&substream->lock); count1 = snd_rawmidi_kernel_read1(substream, (unsigned char __user *)buf, NULL/*kernelbuf*/, @@ -1251,9 +1250,9 @@ int snd_rawmidi_transmit_empty(struct snd_rawmidi_substream *substream) "snd_rawmidi_transmit_empty: output is not active!!!\n"); return 1; } - spin_lock_irqsave(&runtime->lock, flags); + spin_lock_irqsave(&substream->lock, flags); result = runtime->avail >= runtime->buffer_size; - spin_unlock_irqrestore(&runtime->lock, flags); + spin_unlock_irqrestore(&substream->lock, flags); return result; } EXPORT_SYMBOL(snd_rawmidi_transmit_empty); @@ -1322,13 +1321,12 @@ static int __snd_rawmidi_transmit_peek(struct snd_rawmidi_substream *substream, int snd_rawmidi_transmit_peek(struct snd_rawmidi_substream *substream, unsigned char *buffer, int count) { - struct snd_rawmidi_runtime *runtime = substream->runtime; int result; unsigned long flags;
- spin_lock_irqsave(&runtime->lock, flags); + spin_lock_irqsave(&substream->lock, flags); result = __snd_rawmidi_transmit_peek(substream, buffer, count); - spin_unlock_irqrestore(&runtime->lock, flags); + spin_unlock_irqrestore(&substream->lock, flags); return result; } EXPORT_SYMBOL(snd_rawmidi_transmit_peek); @@ -1375,13 +1373,12 @@ static int __snd_rawmidi_transmit_ack(struct snd_rawmidi_substream *substream, */ int snd_rawmidi_transmit_ack(struct snd_rawmidi_substream *substream, int count) { - struct snd_rawmidi_runtime *runtime = substream->runtime; int result; unsigned long flags;
- spin_lock_irqsave(&runtime->lock, flags); + spin_lock_irqsave(&substream->lock, flags); result = __snd_rawmidi_transmit_ack(substream, count); - spin_unlock_irqrestore(&runtime->lock, flags); + spin_unlock_irqrestore(&substream->lock, flags); return result; } EXPORT_SYMBOL(snd_rawmidi_transmit_ack); @@ -1399,11 +1396,10 @@ EXPORT_SYMBOL(snd_rawmidi_transmit_ack); int snd_rawmidi_transmit(struct snd_rawmidi_substream *substream, unsigned char *buffer, int count) { - struct snd_rawmidi_runtime *runtime = substream->runtime; int result; unsigned long flags;
- spin_lock_irqsave(&runtime->lock, flags); + spin_lock_irqsave(&substream->lock, flags); if (!substream->opened) result = -EBADFD; else { @@ -1413,7 +1409,7 @@ int snd_rawmidi_transmit(struct snd_rawmidi_substream *substream, else result = __snd_rawmidi_transmit_ack(substream, count); } - spin_unlock_irqrestore(&runtime->lock, flags); + spin_unlock_irqrestore(&substream->lock, flags); return result; } EXPORT_SYMBOL(snd_rawmidi_transmit); @@ -1430,12 +1426,12 @@ int snd_rawmidi_proceed(struct snd_rawmidi_substream *substream) unsigned long flags; int count = 0;
- spin_lock_irqsave(&runtime->lock, flags); + spin_lock_irqsave(&substream->lock, flags); if (runtime->avail < runtime->buffer_size) { count = runtime->buffer_size - runtime->avail; __snd_rawmidi_transmit_ack(substream, count); } - spin_unlock_irqrestore(&runtime->lock, flags); + spin_unlock_irqrestore(&substream->lock, flags); return count; } EXPORT_SYMBOL(snd_rawmidi_proceed); @@ -1456,10 +1452,10 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream, return -EINVAL;
result = 0; - spin_lock_irqsave(&runtime->lock, flags); + spin_lock_irqsave(&substream->lock, flags); if (substream->append) { if ((long)runtime->avail < count) { - spin_unlock_irqrestore(&runtime->lock, flags); + spin_unlock_irqrestore(&substream->lock, flags); return -EAGAIN; } } @@ -1481,14 +1477,14 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream, memcpy(runtime->buffer + appl_ptr, kernelbuf + result, count1); else if (userbuf) { - spin_unlock_irqrestore(&runtime->lock, flags); + spin_unlock_irqrestore(&substream->lock, flags); if (copy_from_user(runtime->buffer + appl_ptr, userbuf + result, count1)) { - spin_lock_irqsave(&runtime->lock, flags); + spin_lock_irqsave(&substream->lock, flags); result = result > 0 ? result : -EFAULT; goto __end; } - spin_lock_irqsave(&runtime->lock, flags); + spin_lock_irqsave(&substream->lock, flags); } result += count1; count -= count1; @@ -1496,7 +1492,7 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream, __end: count1 = runtime->avail < runtime->buffer_size; snd_rawmidi_buffer_unref(runtime); - spin_unlock_irqrestore(&runtime->lock, flags); + spin_unlock_irqrestore(&substream->lock, flags); if (count1) snd_rawmidi_output_trigger(substream, 1); return result; @@ -1526,31 +1522,31 @@ static ssize_t snd_rawmidi_write(struct file *file, const char __user *buf, return -EIO; result = 0; while (count > 0) { - spin_lock_irq(&runtime->lock); + spin_lock_irq(&substream->lock); while (!snd_rawmidi_ready_append(substream, count)) { wait_queue_entry_t wait;
if (file->f_flags & O_NONBLOCK) { - spin_unlock_irq(&runtime->lock); + spin_unlock_irq(&substream->lock); return result > 0 ? result : -EAGAIN; } init_waitqueue_entry(&wait, current); add_wait_queue(&runtime->sleep, &wait); set_current_state(TASK_INTERRUPTIBLE); - spin_unlock_irq(&runtime->lock); + spin_unlock_irq(&substream->lock); timeout = schedule_timeout(30 * HZ); remove_wait_queue(&runtime->sleep, &wait); if (rfile->rmidi->card->shutdown) return -ENODEV; if (signal_pending(current)) return result > 0 ? result : -ERESTARTSYS; - spin_lock_irq(&runtime->lock); + spin_lock_irq(&substream->lock); if (!runtime->avail && !timeout) { - spin_unlock_irq(&runtime->lock); + spin_unlock_irq(&substream->lock); return result > 0 ? result : -EIO; } } - spin_unlock_irq(&runtime->lock); + spin_unlock_irq(&substream->lock); count1 = snd_rawmidi_kernel_write1(substream, buf, NULL, count); if (count1 < 0) return result > 0 ? result : count1; @@ -1561,7 +1557,7 @@ static ssize_t snd_rawmidi_write(struct file *file, const char __user *buf, count -= count1; } if (file->f_flags & O_DSYNC) { - spin_lock_irq(&runtime->lock); + spin_lock_irq(&substream->lock); while (runtime->avail != runtime->buffer_size) { wait_queue_entry_t wait; unsigned int last_avail = runtime->avail; @@ -1569,16 +1565,16 @@ static ssize_t snd_rawmidi_write(struct file *file, const char __user *buf, init_waitqueue_entry(&wait, current); add_wait_queue(&runtime->sleep, &wait); set_current_state(TASK_INTERRUPTIBLE); - spin_unlock_irq(&runtime->lock); + spin_unlock_irq(&substream->lock); timeout = schedule_timeout(30 * HZ); remove_wait_queue(&runtime->sleep, &wait); if (signal_pending(current)) return result > 0 ? result : -ERESTARTSYS; if (runtime->avail == last_avail && !timeout) return result > 0 ? result : -EIO; - spin_lock_irq(&runtime->lock); + spin_lock_irq(&substream->lock); } - spin_unlock_irq(&runtime->lock); + spin_unlock_irq(&substream->lock); } return result; } @@ -1649,10 +1645,10 @@ static void snd_rawmidi_proc_info_read(struct snd_info_entry *entry, " Owner PID : %d\n", pid_vnr(substream->pid)); runtime = substream->runtime; - spin_lock_irq(&runtime->lock); + spin_lock_irq(&substream->lock); buffer_size = runtime->buffer_size; avail = runtime->avail; - spin_unlock_irq(&runtime->lock); + spin_unlock_irq(&substream->lock); snd_iprintf(buffer, " Mode : %s\n" " Buffer size : %lu\n" @@ -1676,11 +1672,11 @@ static void snd_rawmidi_proc_info_read(struct snd_info_entry *entry, " Owner PID : %d\n", pid_vnr(substream->pid)); runtime = substream->runtime; - spin_lock_irq(&runtime->lock); + spin_lock_irq(&substream->lock); buffer_size = runtime->buffer_size; avail = runtime->avail; xruns = runtime->xruns; - spin_unlock_irq(&runtime->lock); + spin_unlock_irq(&substream->lock); snd_iprintf(buffer, " Buffer size : %lu\n" " Avail : %lu\n" @@ -1732,6 +1728,7 @@ static int snd_rawmidi_alloc_substreams(struct snd_rawmidi *rmidi, substream->number = idx; substream->rmidi = rmidi; substream->pstr = stream; + spin_lock_init(&substream->lock); list_add_tail(&substream->list, &stream->substreams); stream->substream_count++; }
The input/output parameter changes are pretty intrusive, possibly involving with the buffer resizing operation. Hence those should be performed exclusively; otherwise some ugly race could happen.
This patch puts the existing open_mutex for snd_rawmidi_input_params() and *_output_params() for protecting the concurrent calls. Since those are exported, it's also meant for hardening from the external calls, too.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/rawmidi.c | 39 +++++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 14 deletions(-)
diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c index 0a00f37d8c42..7fd6b369d46f 100644 --- a/sound/core/rawmidi.c +++ b/sound/core/rawmidi.c @@ -712,11 +712,19 @@ static int resize_runtime_buffer(struct snd_rawmidi_substream *substream, int snd_rawmidi_output_params(struct snd_rawmidi_substream *substream, struct snd_rawmidi_params *params) { - if (substream->append && substream->use_count > 1) - return -EBUSY; + int err; + snd_rawmidi_drain_output(substream); - substream->active_sensing = !params->no_active_sensing; - return resize_runtime_buffer(substream, params, false); + mutex_lock(&substream->rmidi->open_mutex); + if (substream->append && substream->use_count > 1) + err = -EBUSY; + else + err = resize_runtime_buffer(substream, params, false); + + if (!err) + substream->active_sensing = !params->no_active_sensing; + mutex_unlock(&substream->rmidi->open_mutex); + return err; } EXPORT_SYMBOL(snd_rawmidi_output_params);
@@ -727,19 +735,22 @@ int snd_rawmidi_input_params(struct snd_rawmidi_substream *substream, unsigned int clock_type = params->mode & SNDRV_RAWMIDI_MODE_CLOCK_MASK; int err;
+ snd_rawmidi_drain_input(substream); + mutex_lock(&substream->rmidi->open_mutex); if (framing == SNDRV_RAWMIDI_MODE_FRAMING_NONE && clock_type != SNDRV_RAWMIDI_MODE_CLOCK_NONE) - return -EINVAL; + err = -EINVAL; else if (clock_type > SNDRV_RAWMIDI_MODE_CLOCK_MONOTONIC_RAW) - return -EINVAL; - if (framing > SNDRV_RAWMIDI_MODE_FRAMING_TSTAMP) - return -EINVAL; - snd_rawmidi_drain_input(substream); - err = resize_runtime_buffer(substream, params, true); - if (err < 0) - return err; + err = -EINVAL; + else if (framing > SNDRV_RAWMIDI_MODE_FRAMING_TSTAMP) + err = -EINVAL; + else + err = resize_runtime_buffer(substream, params, true);
- substream->framing = framing; - substream->clock_type = clock_type; + if (!err) { + substream->framing = framing; + substream->clock_type = clock_type; + } + mutex_unlock(&substream->rmidi->open_mutex); return 0; } EXPORT_SYMBOL(snd_rawmidi_input_params);
The rawmidi interface provides some exported functions to be called from outside, and currently there is no state check for those calls whether the stream is properly opened and running. Although such an invalid call shouldn't happen, but who knows.
This patch adds the proper rawmidi stream state checks with spinlocks for avoiding unexpected accesses when such exported functions are called in an invalid state. After this patch, with the substream->opened and substream->runtime are always tied and guaranteed to be set under substream->lock.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/rawmidi.c | 56 ++++++++++++++++++++++++++++++-------------- 1 file changed, 38 insertions(+), 18 deletions(-)
diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c index 7fd6b369d46f..889fa4747dad 100644 --- a/sound/core/rawmidi.c +++ b/sound/core/rawmidi.c @@ -207,7 +207,8 @@ static void reset_runtime_ptrs(struct snd_rawmidi_substream *substream, unsigned long flags;
spin_lock_irqsave(&substream->lock, flags); - __reset_runtime_ptrs(substream->runtime, is_input); + if (substream->opened && substream->runtime) + __reset_runtime_ptrs(substream->runtime, is_input); spin_unlock_irqrestore(&substream->lock, flags); }
@@ -309,12 +310,14 @@ static int open_substream(struct snd_rawmidi *rmidi, snd_rawmidi_runtime_free(substream); return err; } + spin_lock_irq(&substream->lock); substream->opened = 1; substream->active_sensing = 0; if (mode & SNDRV_RAWMIDI_LFLG_APPEND) substream->append = 1; substream->pid = get_pid(task_pid(current)); rmidi->streams[substream->stream].substream_opened++; + spin_unlock_irq(&substream->lock); } substream->use_count++; return 0; @@ -520,12 +523,14 @@ static void close_substream(struct snd_rawmidi *rmidi, snd_rawmidi_output_trigger(substream, 0); } } + spin_lock_irq(&substream->lock); + substream->opened = 0; + substream->append = 0; + spin_unlock_irq(&substream->lock); substream->ops->close(substream); if (substream->runtime->private_free) substream->runtime->private_free(substream); snd_rawmidi_runtime_free(substream); - substream->opened = 0; - substream->append = 0; put_pid(substream->pid); substream->pid = NULL; rmidi->streams[substream->stream].substream_opened--; @@ -1074,17 +1079,21 @@ int snd_rawmidi_receive(struct snd_rawmidi_substream *substream, unsigned long flags; struct timespec64 ts64 = get_framing_tstamp(substream); int result = 0, count1; - struct snd_rawmidi_runtime *runtime = substream->runtime; + struct snd_rawmidi_runtime *runtime;
- if (!substream->opened) - return -EBADFD; - if (runtime->buffer == NULL) { + spin_lock_irqsave(&substream->lock, flags); + if (!substream->opened) { + result = -EBADFD; + goto unlock; + } + runtime = substream->runtime; + if (!runtime || !runtime->buffer) { rmidi_dbg(substream->rmidi, "snd_rawmidi_receive: input is not active!!!\n"); - return -EINVAL; + result = -EINVAL; + goto unlock; }
- spin_lock_irqsave(&substream->lock, flags); if (substream->framing == SNDRV_RAWMIDI_MODE_FRAMING_TSTAMP) { result = receive_with_tstamp_framing(substream, buffer, count, &ts64); } else if (count == 1) { /* special case, faster code */ @@ -1131,6 +1140,7 @@ int snd_rawmidi_receive(struct snd_rawmidi_substream *substream, else if (__snd_rawmidi_ready(runtime)) wake_up(&runtime->sleep); } + unlock: spin_unlock_irqrestore(&substream->lock, flags); return result; } @@ -1252,17 +1262,19 @@ static ssize_t snd_rawmidi_read(struct file *file, char __user *buf, size_t coun */ int snd_rawmidi_transmit_empty(struct snd_rawmidi_substream *substream) { - struct snd_rawmidi_runtime *runtime = substream->runtime; + struct snd_rawmidi_runtime *runtime; int result; unsigned long flags;
- if (runtime->buffer == NULL) { + spin_lock_irqsave(&substream->lock, flags); + runtime = substream->runtime; + if (!substream->opened || !runtime || !runtime->buffer) { rmidi_dbg(substream->rmidi, "snd_rawmidi_transmit_empty: output is not active!!!\n"); - return 1; + result = 1; + } else { + result = runtime->avail >= runtime->buffer_size; } - spin_lock_irqsave(&substream->lock, flags); - result = runtime->avail >= runtime->buffer_size; spin_unlock_irqrestore(&substream->lock, flags); return result; } @@ -1336,7 +1348,10 @@ int snd_rawmidi_transmit_peek(struct snd_rawmidi_substream *substream, unsigned long flags;
spin_lock_irqsave(&substream->lock, flags); - result = __snd_rawmidi_transmit_peek(substream, buffer, count); + if (!substream->opened || !substream->runtime) + result = -EBADFD; + else + result = __snd_rawmidi_transmit_peek(substream, buffer, count); spin_unlock_irqrestore(&substream->lock, flags); return result; } @@ -1388,7 +1403,10 @@ int snd_rawmidi_transmit_ack(struct snd_rawmidi_substream *substream, int count) unsigned long flags;
spin_lock_irqsave(&substream->lock, flags); - result = __snd_rawmidi_transmit_ack(substream, count); + if (!substream->opened || !substream->runtime) + result = -EBADFD; + else + result = __snd_rawmidi_transmit_ack(substream, count); spin_unlock_irqrestore(&substream->lock, flags); return result; } @@ -1433,12 +1451,14 @@ EXPORT_SYMBOL(snd_rawmidi_transmit); */ int snd_rawmidi_proceed(struct snd_rawmidi_substream *substream) { - struct snd_rawmidi_runtime *runtime = substream->runtime; + struct snd_rawmidi_runtime *runtime; unsigned long flags; int count = 0;
spin_lock_irqsave(&substream->lock, flags); - if (runtime->avail < runtime->buffer_size) { + runtime = substream->runtime; + if (substream->opened && runtime && + runtime->avail < runtime->buffer_size) { count = runtime->buffer_size - runtime->avail; __snd_rawmidi_transmit_ack(substream, count); }
Although snd_rawmidi_drain_output() may take some long time, it has no protection and intrusive operations like the buffer resize may happen meanwhile. For making the operation a bit more robust, this patch takes the buffer refcount for blocking the buffer resize.
Also, as this function is exported, in theory, it might be called asynchronously from the stream open/close state. For avoiding the missing refcount, now the close call checks the buffer refcount, too.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/rawmidi.c | 45 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 41 insertions(+), 4 deletions(-)
diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c index 889fa4747dad..6963d5a487b3 100644 --- a/sound/core/rawmidi.c +++ b/sound/core/rawmidi.c @@ -140,6 +140,23 @@ static inline void snd_rawmidi_buffer_unref(struct snd_rawmidi_runtime *runtime) runtime->buffer_ref--; }
+static void snd_rawmidi_buffer_ref_sync(struct snd_rawmidi_substream *substream) +{ + int loop = HZ; + + spin_lock_irq(&substream->lock); + while (substream->runtime->buffer_ref) { + spin_unlock_irq(&substream->lock); + if (!--loop) { + rmidi_err(substream->rmidi, "Buffer ref sync timeout\n"); + return; + } + schedule_timeout_uninterruptible(1); + spin_lock_irq(&substream->lock); + } + spin_unlock_irq(&substream->lock); +} + static int snd_rawmidi_runtime_create(struct snd_rawmidi_substream *substream) { struct snd_rawmidi_runtime *runtime; @@ -222,15 +239,27 @@ EXPORT_SYMBOL(snd_rawmidi_drop_output);
int snd_rawmidi_drain_output(struct snd_rawmidi_substream *substream) { - int err; + int err = 0; long timeout; - struct snd_rawmidi_runtime *runtime = substream->runtime; + struct snd_rawmidi_runtime *runtime; + + spin_lock_irq(&substream->lock); + runtime = substream->runtime; + if (!substream->opened || !runtime || !runtime->buffer) { + err = -EINVAL; + } else { + snd_rawmidi_buffer_ref(runtime); + runtime->drain = 1; + } + spin_unlock_irq(&substream->lock); + if (err < 0) + return err;
- err = 0; - runtime->drain = 1; timeout = wait_event_interruptible_timeout(runtime->sleep, (runtime->avail >= runtime->buffer_size), 10*HZ); + + spin_lock_irq(&substream->lock); if (signal_pending(current)) err = -ERESTARTSYS; if (runtime->avail < runtime->buffer_size && !timeout) { @@ -240,6 +269,8 @@ int snd_rawmidi_drain_output(struct snd_rawmidi_substream *substream) err = -EIO; } runtime->drain = 0; + spin_unlock_irq(&substream->lock); + if (err != -ERESTARTSYS) { /* we need wait a while to make sure that Tx FIFOs are empty */ if (substream->ops->drain) @@ -248,6 +279,11 @@ int snd_rawmidi_drain_output(struct snd_rawmidi_substream *substream) msleep(50); snd_rawmidi_drop_output(substream); } + + spin_lock_irq(&substream->lock); + snd_rawmidi_buffer_unref(runtime); + spin_unlock_irq(&substream->lock); + return err; } EXPORT_SYMBOL(snd_rawmidi_drain_output); @@ -522,6 +558,7 @@ static void close_substream(struct snd_rawmidi *rmidi, if (snd_rawmidi_drain_output(substream) == -ERESTARTSYS) snd_rawmidi_output_trigger(substream, 0); } + snd_rawmidi_buffer_ref_sync(substream); } spin_lock_irq(&substream->lock); substream->opened = 0;
participants (1)
-
Takashi Iwai