[alsa-devel] [PATCH] possible race with /proc/asound symlinks in core/info.c

[Brad Bosch]

Takashi Iwai writes:
 > Thanks for the patch!  I'm very interested in whether the problem
 > happens with the very same scenario in the latest driver code.
 > There are still a few (but rare) bug reports, supposedly happening
 > with the same use-case.  But, the disconnection code was largely
 > changed since 2.6.5, it's not clear at all whether it's the same
 > cause or not.

If the oops stack trace for these bug reports looks something like
this, I'd guess there may still be a race which allows this bug:

Unable to handle kernel NULL pointer dereference at virtual address 0000004c    
 printing eip:                                                                  
02166027                                                                        
*pde = 00000000                                                                 
Oops: 0000 [#1]                                                                 
PREEMPT                                                                         
CPU:    0                                                                       
EIP:    0060:[<02166027>]    Tainted: P                                         
EFLAGS: 00010286   (2.6.5-1.358pt)                                              
EIP is at remove_proc_entry+0x34/0xe5                                           
eax: 00000000   ebx: 3f87bc94   ecx: ffffffff   edx: 0000004c                   
esi: 154fca64   edi: 0000004c   ebp: 41fa9ed8   esp: 41fa9ec8                   
ds: 007b   es: 007b   ss: 0068                                                  
Process events/0 (pid: 3, threadinfo=41fa9000 task=03f2eb50)                    
Stack: 0000004c 3f87bc94 154fca64 00000001 41fa9ef8 431c9b09 0000004c 37ddd8c0  
       431cece5 3f87bc94 0000004c 154fca64 41fa9f0c 431ca648 37ddd8c0 3f87bc94  
       154fca64 41fa9f40 431c8cad 154fca64 00000000 03f2eb50 02111205 00100100  
Call Trace:                                                                     
 [<431c9b09>] snd_remove_proc_entry+0x24/0x3c [snd]                             
 [<431ca648>] snd_info_card_free+0x7d/0xab [snd]                                
 [<431c8cad>] snd_card_free+0x17e/0x1f9 [snd]                                   


 > Well, the sound core checks the duplicated id string, so I'm wondering
 > where the same id string comes from.  Also, the recent kobject handler
 > checks the duplicated entry, and will spew the errors at
 > registration.

Then I wouldn't expect to see the oops above in the latest kernels
unless something else is deleting the symlink entries in /proc/asound
But as you point out below, this patch shouldn't cause any trouble and
I think it is an improvement anyway.

 > 
 > Yes, the fix sounds sane and likely applicable to the latest code.
 > At least it should be harmless...
 > 
 > > This patch won't apply directly to newer kernels because of minor
 > > changes, but it's trivial to apply by hand.  Let me know if you need
 > > me to re-base the patch.
 > 
 > Yes, please.  Then let's queue to the devel tree and see whether
 > any regression occurs actually :)

OK.  Here is the patch based on 2.6.28.7.  I've applied, built, and
tested it here and seen no ill effects.

Thanks!

--Brad

Signed-off-by: Bradley Bosch <bradbosch at comcast.net>

ChangeLog:
Avoid caching a pointer to struct proc_dir_entry for the card symbolic
links in /proc/asound and eliminate the proc_root_link member of
struct snd_card.  This avoids a potential oops should the link ever be
freed by another card.

--- sound/core/info.c~	2009-02-20 16:41:27.000000000 -0600
+++ sound/core/info.c	2009-02-23 18:09:44.000000000 -0600
@@ -648,7 +648,6 @@ int snd_info_card_register(struct snd_ca
 	p = proc_symlink(card->id, snd_proc_root, card->proc_root->name);
 	if (p == NULL)
 		return -ENOMEM;
-	card->proc_root_link = p;
 	return 0;
 }
 
@@ -661,10 +660,7 @@ void snd_info_card_disconnect(struct snd
 	if (!card)
 		return;
 	mutex_lock(&info_mutex);
-	if (card->proc_root_link) {
-		snd_remove_proc_entry(snd_proc_root, card->proc_root_link);
-		card->proc_root_link = NULL;
-	}
+	remove_proc_entry(card->id, snd_proc_root);
 	if (card->proc_root)
 		snd_info_disconnect(card->proc_root);
 	mutex_unlock(&info_mutex);
--- include/sound/core.h~	2009-02-20 16:41:27.000000000 -0600
+++ include/sound/core.h	2009-02-23 18:10:34.000000000 -0600
@@ -132,7 +132,6 @@ struct snd_card {
 
 	struct snd_info_entry *proc_root;	/* root for soundcard specific files */
 	struct snd_info_entry *proc_id;	/* the card id */
-	struct proc_dir_entry *proc_root_link;	/* number link to real id */
 
 	struct snd_monitor_file *files; /* all files associated to this card */
 	struct snd_shutdown_f_ops *s_f_ops; /* file operations in the shutdown