[alsa-devel] [PATCH 0/2] ALSA: fireworks/firewire-tascam: accessing to user space outside spinlock
Hi,
In ALSA fireworks driver and firewire-tascam driver, current implementation for hwdep interface have issues for page fault handling. For example, when using single core processor, after executing lock_page() in page fault handler and call task scheduler, then the context never run again, because any interrupts are disabled by these drivers and the context can't catch waiting event. This patchset fixes the issues.
The issues were reported by Vaishali Thakkar, with a help of coccinelle. Thanks a lot for his care to these minor drivers. http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/111887.html
Takashi Sakamoto (2): ALSA: fireworks: accessing to user space outside spinlock ALSA: firewire-tascam: accessing to user space outside spinlock
sound/firewire/fireworks/fireworks_hwdep.c | 20 ++++++++++++++----- sound/firewire/tascam/tascam-hwdep.c | 32 ++++++++++-------------------- 2 files changed, 25 insertions(+), 27 deletions(-)
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.
Reported-by: Vaishali Thakkar vaishali.thakkar@oracle.com Cc: stable@vger.kernel.org Fixes: 555e8a8f7f14('ALSA: fireworks: Add command/response functionality into hwdep interface') Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/fireworks/fireworks_hwdep.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/sound/firewire/fireworks/fireworks_hwdep.c b/sound/firewire/fireworks/fireworks_hwdep.c index 33df865..b4c3f57 100644 --- a/sound/firewire/fireworks/fireworks_hwdep.c +++ b/sound/firewire/fireworks/fireworks_hwdep.c @@ -38,6 +38,7 @@ hwdep_read_resp_buf(struct snd_efw *efw, char __user *buf, long remained, buf += sizeof(type);
/* write into buffer as many responses as possible */ + spin_lock_irq(&efw->lock); while (efw->resp_queues > 0) { t = (struct snd_efw_transaction *)(efw->pull_ptr); length = be32_to_cpu(t->length) * sizeof(__be32); @@ -52,9 +53,13 @@ hwdep_read_resp_buf(struct snd_efw *efw, char __user *buf, long remained, (unsigned int)(efw->pull_ptr - efw->resp_buf); till_end = min_t(unsigned int, length, till_end);
+ spin_unlock_irq(&efw->lock); + if (copy_to_user(buf, efw->pull_ptr, till_end)) return -EFAULT;
+ spin_lock_irq(&efw->lock); + efw->pull_ptr += till_end; if (efw->pull_ptr >= efw->resp_buf + snd_efw_resp_buf_size) @@ -69,6 +74,8 @@ hwdep_read_resp_buf(struct snd_efw *efw, char __user *buf, long remained, efw->resp_queues--; }
+ spin_unlock_irq(&efw->lock); + return count; }
@@ -76,14 +83,17 @@ static long hwdep_read_locked(struct snd_efw *efw, char __user *buf, long count, loff_t *offset) { - union snd_firewire_event event; - - memset(&event, 0, sizeof(event)); + union snd_firewire_event event = {0};
event.lock_status.type = SNDRV_FIREWIRE_EVENT_LOCK_STATUS; + + spin_lock_irq(&efw->lock); + event.lock_status.status = (efw->dev_lock_count > 0); efw->dev_lock_changed = false;
+ spin_unlock_irq(&efw->lock); + count = min_t(long, count, sizeof(event.lock_status));
if (copy_to_user(buf, &event, count)) @@ -111,13 +121,13 @@ hwdep_read(struct snd_hwdep *hwdep, char __user *buf, long count, spin_lock_irq(&efw->lock); }
+ spin_unlock_irq(&efw->lock); + if (efw->dev_lock_changed) count = hwdep_read_locked(efw, buf, count, offset); else if (efw->resp_queues > 0) count = hwdep_read_resp_buf(efw, buf, count, offset);
- spin_unlock_irq(&efw->lock); - return count; }
In hwdep interface of firewire-tascam 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 handle 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 doesn't handle usual interrupts from hardware.
This commit fixes this bug, by performing the accessing outside spinlock.
Reported-by: Vaishali Thakkar vaishali.thakkar@oracle.com Cc: stable@vger.kernel.org Fixes: e5e0c3dd257b('ALSA: firewire-tascam: add hwdep interface') Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/tascam/tascam-hwdep.c | 32 ++++++++++---------------------- 1 file changed, 10 insertions(+), 22 deletions(-)
diff --git a/sound/firewire/tascam/tascam-hwdep.c b/sound/firewire/tascam/tascam-hwdep.c index 131267c..edf20c7 100644 --- a/sound/firewire/tascam/tascam-hwdep.c +++ b/sound/firewire/tascam/tascam-hwdep.c @@ -16,31 +16,12 @@
#include "tascam.h"
-static long hwdep_read_locked(struct snd_tscm *tscm, char __user *buf, - long count) -{ - union snd_firewire_event event; - - memset(&event, 0, sizeof(event)); - - event.lock_status.type = SNDRV_FIREWIRE_EVENT_LOCK_STATUS; - event.lock_status.status = (tscm->dev_lock_count > 0); - tscm->dev_lock_changed = false; - - count = min_t(long, count, sizeof(event.lock_status)); - - if (copy_to_user(buf, &event, count)) - return -EFAULT; - - return count; -} - static long hwdep_read(struct snd_hwdep *hwdep, char __user *buf, long count, loff_t *offset) { struct snd_tscm *tscm = hwdep->private_data; DEFINE_WAIT(wait); - union snd_firewire_event event; + union snd_firewire_event event = {0};
spin_lock_irq(&tscm->lock);
@@ -54,10 +35,17 @@ static long hwdep_read(struct snd_hwdep *hwdep, char __user *buf, long count, spin_lock_irq(&tscm->lock); }
- memset(&event, 0, sizeof(event)); - count = hwdep_read_locked(tscm, buf, count); + event.lock_status.type = SNDRV_FIREWIRE_EVENT_LOCK_STATUS; + event.lock_status.status = (tscm->dev_lock_count > 0); + tscm->dev_lock_changed = false; + spin_unlock_irq(&tscm->lock);
+ count = min_t(long, count, sizeof(event.lock_status)); + + if (copy_to_user(buf, &event, count)) + return -EFAULT; + return count; }
On Saturday 27 August 2016 02:20 AM, Takashi Sakamoto wrote:
Hi,
In ALSA fireworks driver and firewire-tascam driver, current implementation for hwdep interface have issues for page fault handling. For example, when using single core processor, after executing lock_page() in page fault handler and call task scheduler, then the context never run again, because any interrupts are disabled by these drivers and the context can't catch waiting event. This patchset fixes the issues.
The issues were reported by Vaishali Thakkar, with a help of coccinelle. Thanks a lot for his care to these minor drivers. http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/111887.html
Hi,
Just a minor correction here. 'his -> her' :)
Takashi Sakamoto (2): ALSA: fireworks: accessing to user space outside spinlock ALSA: firewire-tascam: accessing to user space outside spinlock
sound/firewire/fireworks/fireworks_hwdep.c | 20 ++++++++++++++----- sound/firewire/tascam/tascam-hwdep.c | 32 ++++++++++-------------------- 2 files changed, 25 insertions(+), 27 deletions(-)
On Aug 30 2016 02:32, Vaishali Thakkar wrote:
On Saturday 27 August 2016 02:20 AM, Takashi Sakamoto wrote:
Hi,
In ALSA fireworks driver and firewire-tascam driver, current implementation for hwdep interface have issues for page fault handling. For example, when using single core processor, after executing lock_page() in page fault handler and call task scheduler, then the context never run again, because any interrupts are disabled by these drivers and the context can't catch waiting event. This patchset fixes the issues.
The issues were reported by Vaishali Thakkar, with a help of coccinelle. Thanks a lot for his care to these minor drivers. http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/111887.html
Hi,
Just a minor correction here. 'his -> her' :)
I apologize sincerely for my discourtesy...
Takashi Sakamoto (2): ALSA: fireworks: accessing to user space outside spinlock ALSA: firewire-tascam: accessing to user space outside spinlock
sound/firewire/fireworks/fireworks_hwdep.c | 20 ++++++++++++++----- sound/firewire/tascam/tascam-hwdep.c | 32 ++++++++++-------------------- 2 files changed, 25 insertions(+), 27 deletions(-)
This weekend, alsa-project.org is out of service, so I cannot post revised version. As of this morning, it seems to start services so I'll post them soon.
Thanks :)
Takashi Sakamoto
participants (2)
-
Takashi Sakamoto
-
Vaishali Thakkar