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

[Takashi Iwai]

At Sat, 21 Feb 2009 10:55:30 -0600,
Brad Bosch wrote:
> 
> I discovered and fixed an ALSA oops in a very old kernel (2.6.5) used
> in an embedded application and looking at the latest kernels, it's not
> clear to me that this was ever completely fixed in newer kernels.  I'm
> sending this email so someone who understands the interactions between
> the USB and ALSA subsystems better than I do can determine if a
> similar fix might be required in the newer kernels.

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.

> In the old kernel, the oops often happens when a USB device is
> unplugged and then a device with the same ID (or the same device) is
> immediately plugged back in before the old device is completely
> cleaned up.  The included patch resolves the problem in that kernel.
> 
> The problem was that two cards were creating symbolic links in
> /proc/asound with the same name (id) but pointing to differen't cards.

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.

> The proc subsystem apparently allowed this.  Later, when one of the
> entries is removed (using it's name), the wrong entry may be removed,
> freeing the wrong struct proc_dir_entry and leaving a pointer to the
> freed memory in the other card's proc_root_link member.  Later when
> that USB device is unplugged and snd_info_card_disconnect() is called,
> snd_remove_proc_entry() will reference the name member of struct
> proc_dir_entry which by now may be changed, or even null.  It turns
> out to be trivial to avoid this problem by simply not storing the
> pointer to the proc_dir_entry as we don't need it anyway to determine
> the name of the link to remove.  Instead, for the case of the symbolic
> link, we call remove_proc_entry directly using the card id for the
> link name (just as we did when it was created).
> 
> I don't know if two symbolic links are ever created in /proc/asound
> with the same name in the newer kernels or newer ALSA code and in fact, I
> haven't been able to reproduce the problem there.  However, since the
> patch reduces both the data and code size, and eliminates a potential
> crash, you may want to apply it 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 :)


thanks,

Takashi


> 
> --Brad
> 
> Index: sound/core/info.c
> ===================================================================
> RCS file: /cvs/kernels/kernel_2_6_5/sound/core/info.c,v
> retrieving revision 1.1.4.1.10.1
> diff -u -r1.1.4.1.10.1 info.c
> --- sound/core/info.c	22 Feb 2006 14:06:25 -0000	1.1.4.1.10.1
> +++ sound/core/info.c	20 Feb 2009 02:05:01 -0000
> @@ -618,7 +618,6 @@
>  	p = proc_symlink(card->id, snd_proc_root, card->proc_root->name);
>  	if (p == NULL)
>  		return -ENOMEM;
> -	card->proc_root_link = p;
>  	return 0;
>  }
>  
> @@ -629,10 +628,7 @@
>  int snd_info_card_free(snd_card_t * card)
>  {
>  	snd_assert(card != NULL, return -ENXIO);
> -	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_unregister(card->proc_root);
>  		card->proc_root = NULL;
> Index: include/sound/core.h
> ===================================================================
> RCS file: /cvs/kernels/kernel_2_6_5/include/sound/core.h,v
> retrieving revision 1.1.4.1
> diff -u -r1.1.4.1 core.h
> --- include/sound/core.h	15 Apr 2005 21:45:13 -0000	1.1.4.1
> +++ include/sound/core.h	20 Feb 2009 02:05:02 -0000
> @@ -152,7 +152,6 @@
>  
>  	snd_info_entry_t *proc_root;	/* root for soundcard specific files */
>  	snd_info_entry_t *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 state */
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>