Re: usb audio race at disconnect time

[Matthieu CASTET <matthieu.castet-ITF29qwbsa/QT0dZR+AlfA@public.gmane.org>]

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

Takashi Iwai a écrit :
> At Mon, 15 Oct 2012 19:41:40 +0200,
> Matthieu CASTET wrote:
>> Hi Takashi,
>>
>> But I believe I found other races in the alsa char device handling. With the
>> attached patch, if you disconnect the usb audio device between "msleep o" and
>> "msleep o+", you will free the card resource (snd_card_do_free) and then use it [1].
>>
>> I did in in snd_ctl_open, but the same thing could be done in snd_pcm_open, ...
> 
> OK, we'd need a generic open/close protection.
> For PCM, there is already a fix in my last patchset, so it should
> work, but for other devices, the paths are still uncovered.
> 
I don't think it will work for pcm : the begin of snd_pcm_open is not protected
by any lock even with your patch
In snd_open we release sound_mutex before calling file->f_op->open.

The snd_lookup_minor_data and snd_card_file_add should be protected by a lock.
Attached a patch (pcm.crash) that help to trigger [1].


One idea could be to do the snd_card_file_add in snd_lookup_minor_data : it will
be protected by sound_mutex.

That what rfc attachement does (with debug printk and delay) : it add a
snd_lookup_minor_data2 that do the snd_card_file_add.


Matthieu



[1]

# cat /dev/pcmC0D0c
[  572.187866] msleep o cf1e1200
[  573.534454] usb 1-2: USB disconnect, device number 2
[  573.543670] usb 1-2.1: USB disconnect, device number 3
[  573.561645] usb 1-2.1.2: USB disconnect, device number 4
[  577.337005] msleep o+
[  577.339477] Unable to handle kernel paging request at virtual address 6b6b6d87
[  577.347106] pgd = cccf4000
[  577.349975] [6b6b6d87] *pgd=00000000
[  577.353759] Internal error: Oops: 5 [#1] PREEMPT SMP ARM
[  577.359374] Modules linked in:
[  577.362640] CPU: 0    Not tainted  (3.7.0-rc1-00004-gd6355f5-dirty #27)
[  577.369628] PC is at __lock_acquire.clone.19+0x150/0xd40
[  577.375274] LR is at lock_acquire+0x5c/0x70
[  577.379699] pc : [<c0088b10>]    lr : [<c0089cd0>]    psr: 80000093
[  577.379699] sp : cca01c28  ip : 00000001  fp : cca01c84
[  577.391784] r10: c05d5814  r9 : 6b6b6d83  r8 : 00000080
[  577.397308] r7 : cf15f480  r6 : 00000000  r5 : c05b67bc  r4 : cca00000
[  577.404205] r3 : 00000000  r2 : 00000000  r1 : 00000000  r0 : 6b6b6d83
[  577.411102] Flags: Nzcv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
[  577.418731] Control: 10c5387d  Table: 8ccf4019  DAC: 00000015
[  577.424804] Process cat (pid: 642, stack limit = 0xcca00240)
[  577.430786] Stack: (0xcca01c28 to 0xcca02000)
[  577.435394] 1c20:                   00000001 20000093 000000c4 00000000
c05ae4a8 00000001
[  577.444030] 1c40: 00000000 c05d4b40 00000009 c05ae478 60000013 00000004
cca01ccc 00000000
[  577.452667] 1c60: cca00000 60000013 cf15f480 00000001 00000024 cf7bcd40
cca01cb4 cca01c88
[  577.461334] 1c80: c0089cd0 c00889cc 00000000 00000000 c02eacec 00000000
6b6b6d73 c02eacec
[  577.469970] 1ca0: ccd12f00 6b6b6d73 cca01cdc cca01cb8 c03e5234 c0089c80
00000001 00000000
[  577.478607] 1cc0: c02eacec cf0980a8 6b6b6b6b cf0980a8 cca01cfc cca01ce0
c02eacec c03e51f8
[  577.487243] 1ce0: cf0980a8 ffffffff c05b65d4 cf1e1200 cca01d4c cca01d00
c02fbb84 c02eaca8
[  577.495880] 1d00: 22222222 cf1e1200 00000018 cd9d0120 07400018 c09d9af8
cca01d34 cca01d28
[  577.504547] 1d20: c03e33fc cf0980a8 cd9d0120 cd9d0120 07400018 c09d9af8
00000024 cf7bcd40
[  577.513183] 1d40: cca01d64 cca01d50 c02fbd90 c02fbb20 c0411378 cf0980a8
cca01d8c cca01d68
[  577.521820] 1d60: c02ea810 c02fbd58 cd9d0120 cf0980a8 cf1fb1c0 00000000
c00f505c 00000000
[  577.530456] 1d80: cca01db4 cca01d90 c00f50f4 c02ea774 c00f25cc 00000018
cf0980a8 cd9d0120
[  577.539093] 1da0: cf0980b0 00020001 cca01ddc cca01db8 c00ef404 c00f5068
cca01ea0 cca01f60
[  577.547729] 1dc0: 00000000 00020000 cca01e98 00000000 cca01df4 cca01de0
c00ef4c0 c00ef240
[  577.556365] 1de0: cca01ea0 cca01ee0 cca01e6c cca01df8 c00fe348 c00ef4a4
cca01e6c cca01e08
[  577.565002] 1e00: c00fb704 c00fb4a8 cca01e3c cca01e18 c004b2c8 c007d99c
00000000 cca01ee8
[  577.573638] 1e20: 00000000 00000000 ceb2f178 cf0980a8 cf016e58 ceb2f178
cd9d0120 cf2fa830
[  577.582275] 1e40: 386d45b8 cca01ee0 ceb42bf8 cca01e98 cca01f60 00000000
cca01ea0 cca00000
[  577.590911] 1e60: cca01ed4 cca01e70 c00fec00 c00fdd70 cca01ea0 00000000
00000004 00000000
[  577.599548] 1e80: 000000e5 00000000 cf0980a8 cca01e90 cf016e58 ceb42bf8
cf016e58 ceb40d38
[  577.608215] 1ea0: 00000000 00000000 cca01ee4 cca01f60 00000001 ffffff9c
cf30f000 ffffff9c
[  577.616851] 1ec0: cca00000 00000000 cca01f54 cca01ed8 c00ff0f8 c00fe958
00000041 cf0235e0
[  577.625488] 1ee0: cf016e58 ceb40d38 d24edc20 00000008 ce487d44 cf7b3d90
cf016318 ce8021f8
[  577.634124] 1f00: cd9d0120 00000101 00000004 00000000 00000000 ce487d40
00020000 cf30f000
[  577.642761] 1f20: 00020000 be9ecf92 00000001 ffffff9c cca00000 00000000
cf30f000 00020000
[  577.651397] 1f40: 00000003 00000001 cca01f94 cca01f58 c00f044c c00ff0d0
c0060598 c03e44d0
[  577.660034] 1f60: 00020000 cca00000 00000024 00000100 00000000 00000000
be9ecf92 00000005
[  577.668701] 1f80: c0014808 00000000 cca01fa4 cca01f98 c00f0504 c00f036c
00000000 cca01fa8
[  577.677337] 1fa0: c0014660 c00f04e8 00000000 00000000 be9ecf92 00020000
00000000 000030ec
[  577.685974] 1fc0: 00000000 00000000 be9ecf92 00000005 00000008 00000000
b6f5f000 00000000
[  577.694610] 1fe0: b6e163b0 be9eccd0 0000d898 b6e16400 60000010 be9ecf92
00000000 00000000
[  577.703247] Backtrace:
[  577.705841] [<c00889c0>] (__lock_acquire.clone.19+0x0/0xd40) from
[<c0089cd0>] (lock_acquire+0x5c/0x70)
[  577.715789] [<c0089c74>] (lock_acquire+0x0/0x70) from [<c03e5234>]
(_raw_spin_lock+0x48/0x58)
[  577.724792]  r7:6b6b6d73 r6:ccd12f00 r5:c02eacec r4:6b6b6d73
[  577.730834] [<c03e51ec>] (_raw_spin_lock+0x0/0x58) from [<c02eacec>]
(snd_card_file_add+0x50/0xac)
[  577.740295]  r5:cf0980a8 r4:6b6b6b6b
[  577.744110] [<c02eac9c>] (snd_card_file_add+0x0/0xac) from [<c02fbb84>]
(snd_pcm_open+0x70/0x238)
[  577.753479]  r7:cf1e1200 r6:c05b65d4 r5:ffffffff r4:cf0980a8
[  577.759521] [<c02fbb14>] (snd_pcm_open+0x0/0x238) from [<c02fbd90>]
(snd_pcm_capture_open+0x44/0x48)
[  577.769165] [<c02fbd4c>] (snd_pcm_capture_open+0x0/0x48) from [<c02ea810>]
(snd_open+0xa8/0x1a0)
[  577.778442]  r5:cf0980a8 r4:c0411378
[  577.782257] [<c02ea768>] (snd_open+0x0/0x1a0) from [<c00f50f4>]
(chrdev_open+0x98/0x160)
[  577.790832] [<c00f505c>] (chrdev_open+0x0/0x160) from [<c00ef404>]
(do_dentry_open.clone.16+0x1d0/0x264)
[  577.800842]  r7:00020001 r6:cf0980b0 r5:cd9d0120 r4:cf0980a8
[  577.806884] [<c00ef234>] (do_dentry_open.clone.16+0x0/0x264) from
[<c00ef4c0>] (finish_open+0x28/0x40)
[  577.816711] [<c00ef498>] (finish_open+0x0/0x40) from [<c00fe348>]
(do_last.clone.47+0x5e4/0xbe8)
[  577.825988]  r4:cca01ee0 r3:cca01ea0
[  577.829803] [<c00fdd64>] (do_last.clone.47+0x0/0xbe8) from [<c00fec00>]
(path_openat.clone.48+0x2b4/0x488)
[  577.839996] [<c00fe94c>] (path_openat.clone.48+0x0/0x488) from [<c00ff0f8>]
(do_filp_open+0x34/0x88)
[  577.849670] [<c00ff0c4>] (do_filp_open+0x0/0x88) from [<c00f044c>]
(do_sys_open+0xec/0x17c)
[  577.858489]  r7:00000001 r6:00000003 r5:00020000 r4:cf30f000
[  577.864532] [<c00f0360>] (do_sys_open+0x0/0x17c) from [<c00f0504>]
(sys_open+0x28/0x2c)
[  577.872985] [<c00f04dc>] (sys_open+0x0/0x2c) from [<c0014660>]
(ret_fast_syscall+0x0/0x30)
[  577.881744] Code: ebfeebea e1a00004 ea000038 e0890106 (e5908004)
[  577.888183] ---[ end trace 5c43a835149de569 ]---




[-- Attachment #2: pcm.crash --]
[-- Type: text/plain, Size: 586 bytes --]

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 8753c89..0cbe471 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2130,11 +2130,16 @@ static int snd_pcm_capture_open(struct inode *inode, struct file *file)
 	return snd_pcm_open(file, pcm, SNDRV_PCM_STREAM_CAPTURE);
 }
 
+#include <linux/delay.h>
 static int snd_pcm_open(struct file *file, struct snd_pcm *pcm, int stream)
 {
 	int err;
 	wait_queue_t wait;
 
+	printk("msleep o %p\n", pcm);
+	mdelay(5000);
+	printk("msleep o+\n");
+
 	if (pcm == NULL) {
 		err = -ENODEV;
 		goto __error1;

[-- Attachment #3: rfc --]
[-- Type: text/plain, Size: 3632 bytes --]

diff --git a/include/sound/core.h b/include/sound/core.h
index bc05668..9869087 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -185,6 +185,7 @@ static inline int snd_power_wait(struct snd_card *card, unsigned int state) { re
 struct snd_minor {
 	int type;			/* SNDRV_DEVICE_TYPE_XXX */
 	int card;			/* card number */
+	struct snd_card *cardp;
 	int device;			/* device number */
 	const struct file_operations *f_ops;	/* file operations */
 	void *private_data;		/* private data for f_ops->open */
@@ -241,6 +242,7 @@ static inline int snd_register_device(int type, struct snd_card *card, int dev,
 
 int snd_unregister_device(int type, struct snd_card *card, int dev);
 void *snd_lookup_minor_data(unsigned int minor, int type);
+void *snd_lookup_minor_data2(unsigned int minor, int type, struct file *file);
 int snd_add_device_sysfs_file(int type, struct snd_card *card, int dev,
 			      struct device_attribute *attr);
 
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 8753c89..c2445c0 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2114,8 +2114,8 @@ static int snd_pcm_playback_open(struct inode *inode, struct file *file)
 	int err = nonseekable_open(inode, file);
 	if (err < 0)
 		return err;
-	pcm = snd_lookup_minor_data(iminor(inode),
-				    SNDRV_DEVICE_TYPE_PCM_PLAYBACK);
+	pcm = snd_lookup_minor_data2(iminor(inode),
+				    SNDRV_DEVICE_TYPE_PCM_PLAYBACK, file);
 	return snd_pcm_open(file, pcm, SNDRV_PCM_STREAM_PLAYBACK);
 }
 
@@ -2125,23 +2125,24 @@ static int snd_pcm_capture_open(struct inode *inode, struct file *file)
 	int err = nonseekable_open(inode, file);
 	if (err < 0)
 		return err;
-	pcm = snd_lookup_minor_data(iminor(inode),
-				    SNDRV_DEVICE_TYPE_PCM_CAPTURE);
+	pcm = snd_lookup_minor_data2(iminor(inode),
+				    SNDRV_DEVICE_TYPE_PCM_CAPTURE, file);
 	return snd_pcm_open(file, pcm, SNDRV_PCM_STREAM_CAPTURE);
 }
 
+#include <linux/delay.h>
 static int snd_pcm_open(struct file *file, struct snd_pcm *pcm, int stream)
 {
 	int err;
 	wait_queue_t wait;
 
+	printk("msleep o2 %p\n", pcm);
+	mdelay(5000);
+	printk("msleep o2+\n");
 	if (pcm == NULL) {
 		err = -ENODEV;
 		goto __error1;
 	}
-	err = snd_card_file_add(pcm->card, file);
-	if (err < 0)
-		goto __error1;
 	if (!try_module_get(pcm->card->module)) {
 		err = -EFAULT;
 		goto __error2;
diff --git a/sound/core/sound.c b/sound/core/sound.c
index 6439760..5ea8010 100644
--- a/sound/core/sound.c
+++ b/sound/core/sound.c
@@ -118,6 +118,35 @@ void *snd_lookup_minor_data(unsigned int minor, int type)
 
 EXPORT_SYMBOL(snd_lookup_minor_data);
 
+#include <linux/delay.h>
+void *snd_lookup_minor_data2(unsigned int minor, int type, struct file *file)
+{
+	struct snd_minor *mreg;
+	void *private_data;
+
+	if (minor >= ARRAY_SIZE(snd_minors))
+		return NULL;
+	mutex_lock(&sound_mutex);
+	mreg = snd_minors[minor];
+	if (mreg && mreg->type == type)
+		private_data = mreg->private_data;
+	else
+		private_data = NULL;
+	printk("msleep o %p\n", NULL);
+	mdelay(5000);
+	printk("msleep o+\n");
+
+
+	if (private_data) {
+		if (snd_card_file_add(mreg->cardp, file) < 0)
+			private_data = NULL;
+	}
+	mutex_unlock(&sound_mutex);
+	return private_data;
+}
+
+EXPORT_SYMBOL(snd_lookup_minor_data2);
+
 #ifdef CONFIG_MODULES
 static struct snd_minor *autoload_device(unsigned int minor)
 {
@@ -272,6 +301,7 @@ int snd_register_device_for_dev(int type, struct snd_card *card, int dev,
 		return -ENOMEM;
 	preg->type = type;
 	preg->card = card ? card->number : -1;
+	preg->cardp = card;
 	preg->device = dev;
 	preg->f_ops = f_ops;
 	preg->private_data = private_data;