[alsa-devel] Patch for possible race with /proc/asound symlinks in core/info.c
Jaroslav,
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.
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. 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.
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.
--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 */
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@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
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@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
At Tue, 24 Feb 2009 10:50:35 -0600, Brad Bosch wrote:
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]
Unfortunately, not. It's somewhere in snd_mixer_oss_notify_handler() called in the delayed snd_card_do_free().
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.
Yep.
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.
Well, 2.6.29 has a feature to rename id string dynamically, and this refers to the proc_root_link field. Thus your patch isn't applicable, unfortunately.
We need a workaround for that, e.g. call snd_info_card_id_change() before changing card->id...
thanks,
(BTW, it'd be appreciated if you make a diff with -p1 at the next time.)
Takashi
Thanks!
--Brad
Signed-off-by: Bradley Bosch bradbosch@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
participants (2)
-
Brad Bosch -
Takashi Iwai