[PATCH 0/2] ALSA: fireworks/firewire-tascam: accessing to user space outside spinlock

[PATCH 0/2] ALSA: fireworks/firewire-tascam: accessing to user space outside spinlock

[Takashi Sakamoto]

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(-)

-- 
2.7.4

[PATCH 1/2] ALSA: fireworks: accessing to user space outside spinlock

[Takashi Sakamoto]

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;
 }
 
-- 
2.7.4

Re: [alsa-devel] [PATCH 1/2] ALSA: fireworks: accessing to user space outside spinlock

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2195 bytes --]

Hi Takashi,

[auto build test WARNING on sound/for-next]
[also build test WARNING on v4.8-rc3 next-20160825]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Takashi-Sakamoto/ALSA-fireworks-accessing-to-user-space-outside-spinlock/20160827-045250
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
config: tile-allyesconfig (attached as .config)
compiler: tilegx-linux-gcc (GCC) 4.6.2
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=tile 

All warnings (new ones prefixed by >>):

   sound/firewire/fireworks/fireworks_hwdep.c: In function 'hwdep_read_locked':
>> sound/firewire/fireworks/fireworks_hwdep.c:86:8: warning: missing braces around initializer [-Wmissing-braces]
   sound/firewire/fireworks/fireworks_hwdep.c:86:8: warning: (near initialization for 'event.common') [-Wmissing-braces]

vim +86 sound/firewire/fireworks/fireworks_hwdep.c

    70				count += till_end;
    71				remained -= till_end;
    72			}
    73	
    74			efw->resp_queues--;
    75		}
    76	
    77		spin_unlock_irq(&efw->lock);
    78	
    79		return count;
    80	}
    81	
    82	static long
    83	hwdep_read_locked(struct snd_efw *efw, char __user *buf, long count,
    84			  loff_t *offset)
    85	{
  > 86		union snd_firewire_event event = {0};
    87	
    88		event.lock_status.type = SNDRV_FIREWIRE_EVENT_LOCK_STATUS;
    89	
    90		spin_lock_irq(&efw->lock);
    91	
    92		event.lock_status.status = (efw->dev_lock_count > 0);
    93		efw->dev_lock_changed = false;
    94	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 45493 bytes --]

Re: [PATCH 1/2] ALSA: fireworks: accessing to user space outside spinlock

[Takashi Sakamoto]

On Aug 27 2016 06:34, kbuild test robot wrote:
> Hi Takashi,
> 
> [auto build test WARNING on sound/for-next]
> [also build test WARNING on v4.8-rc3 next-20160825]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> [Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
> [Check https://git-scm.com/docs/git-format-patch for more information]
> 
> url:    https://github.com/0day-ci/linux/commits/Takashi-Sakamoto/ALSA-fireworks-accessing-to-user-space-outside-spinlock/20160827-045250
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
> config: tile-allyesconfig (attached as .config)
> compiler: tilegx-linux-gcc (GCC) 4.6.2
> reproduce:
>         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=tile 
> 
> All warnings (new ones prefixed by >>):
> 
>    sound/firewire/fireworks/fireworks_hwdep.c: In function 'hwdep_read_locked':
>>> sound/firewire/fireworks/fireworks_hwdep.c:86:8: warning: missing braces around initializer [-Wmissing-braces]
>    sound/firewire/fireworks/fireworks_hwdep.c:86:8: warning: (near initialization for 'event.common') [-Wmissing-braces]
> 
> vim +86 sound/firewire/fireworks/fireworks_hwdep.c
> 
>     70				count += till_end;
>     71				remained -= till_end;
>     72			}
>     73	
>     74			efw->resp_queues--;
>     75		}
>     76	
>     77		spin_unlock_irq(&efw->lock);
>     78	
>     79		return count;
>     80	}
>     81	
>     82	static long
>     83	hwdep_read_locked(struct snd_efw *efw, char __user *buf, long count,
>     84			  loff_t *offset)
>     85	{
>   > 86		union snd_firewire_event event = {0};

Mm. I should have indicate actual data type of this union for desinated
initialization.
https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html

So as for firewire-tascam module. I'll post revised version of them.
Sorry for stable developers but please ignore this patchset...

>     87	
>     88		event.lock_status.type = SNDRV_FIREWIRE_EVENT_LOCK_STATUS;
>     89	
>     90		spin_lock_irq(&efw->lock);
>     91	
>     92		event.lock_status.status = (efw->dev_lock_count > 0);
>     93		efw->dev_lock_changed = false;
>     94	


Regards

Takashi Sakamoto

[PATCH 2/2] ALSA: firewire-tascam: accessing to user space outside spinlock

[Takashi Sakamoto]

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;
 }
 
-- 
2.7.4

Re: [alsa-devel] [PATCH 2/2] ALSA: firewire-tascam: accessing to user space outside spinlock

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2329 bytes --]

Hi Takashi,

[auto build test WARNING on sound/for-next]
[also build test WARNING on v4.8-rc3 next-20160825]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Takashi-Sakamoto/ALSA-fireworks-accessing-to-user-space-outside-spinlock/20160827-045250
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
config: tile-allyesconfig (attached as .config)
compiler: tilegx-linux-gcc (GCC) 4.6.2
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=tile 

All warnings (new ones prefixed by >>):

   sound/firewire/tascam/tascam-hwdep.c: In function 'hwdep_read':
>> sound/firewire/tascam/tascam-hwdep.c:24:8: warning: missing braces around initializer [-Wmissing-braces]
   sound/firewire/tascam/tascam-hwdep.c:24:8: warning: (near initialization for 'event.common') [-Wmissing-braces]

vim +24 sound/firewire/tascam/tascam-hwdep.c

     8	
     9	/*
    10	 * This codes give three functionality.
    11	 *
    12	 * 1.get firewire node information
    13	 * 2.get notification about starting/stopping stream
    14	 * 3.lock/unlock stream
    15	 */
    16	
    17	#include "tascam.h"
    18	
    19	static long hwdep_read(struct snd_hwdep *hwdep, char __user *buf, long count,
    20			       loff_t *offset)
    21	{
    22		struct snd_tscm *tscm = hwdep->private_data;
    23		DEFINE_WAIT(wait);
  > 24		union snd_firewire_event event = {0};
    25	
    26		spin_lock_irq(&tscm->lock);
    27	
    28		while (!tscm->dev_lock_changed) {
    29			prepare_to_wait(&tscm->hwdep_wait, &wait, TASK_INTERRUPTIBLE);
    30			spin_unlock_irq(&tscm->lock);
    31			schedule();
    32			finish_wait(&tscm->hwdep_wait, &wait);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 45493 bytes --]

Re: [PATCH 0/2] ALSA: fireworks/firewire-tascam: accessing to user space outside spinlock

[Vaishali Thakkar]

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(-)
> 

-- 
Vaishali

Re: [PATCH 0/2] ALSA: fireworks/firewire-tascam: accessing to user space outside spinlock

[Takashi Sakamoto]

[-- Attachment #1.1.1: Type: text/plain, Size: 1475 bytes --]

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]