* snd_card_disconnect race (sound/core/init.c)
@ 2011-03-24 1:40 Russ Dill
2011-03-24 6:43 ` Takashi Iwai
0 siblings, 1 reply; 7+ messages in thread
From: Russ Dill @ 2011-03-24 1:40 UTC (permalink / raw)
To: linux-kernel; +Cc: Takashi Iwai
With slub poisoning on, I'm seeing an oops in snd_disconnect_release
with slub poison (6b6b6b6b). Investigating, snd_card_disconnect looks
to be the possible culprit:
333 spin_lock(&card->files_lock);
334 mfile = card->files;
335 while (mfile) {
336 file = mfile->file;
337
338 /* it's critical part, use endless loop */
339 /* we have no room to fail */
340 mfile->disconnected_f_op = mfile->file->f_op;
341
342 spin_lock(&shutdown_lock);
343 list_add(&mfile->shutdown_list, &shutdown_files);
344 spin_unlock(&shutdown_lock);
345
346 mfile->file->f_op = &snd_shutdown_f_ops;
347 fops_get(mfile->file->f_op);
348
349 mfile = mfile->next;
350 }
351 spin_unlock(&card->files_lock);
I'm not aware of any locking that would prevent the original release
function running concurrently if it were started before line 346 was
executed. The original release functions (such as snd_hwdep_release)
call snd_card_remove_file which frees the mfile object which would
have been just added to the shutdown_files list leading to a use of
freed (or in my case poisoned) memory later on down the line when the
shutdown_files gets walked again.
It seems that 118dd6bf (ALSA: Clean up snd_monitor_file management,
v2.6.30) may have either introduced this problem or made it worse
since snd_card_file_remove previously removed items from the
shutdown_files list before freeing them.
I'm currently experiencing these crashes on 2.6.32.26, but I can't
find any changes that would cause them not to occur on later kernel
versions.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: snd_card_disconnect race (sound/core/init.c) 2011-03-24 1:40 snd_card_disconnect race (sound/core/init.c) Russ Dill @ 2011-03-24 6:43 ` Takashi Iwai 2011-03-24 8:19 ` Russ Dill 0 siblings, 1 reply; 7+ messages in thread From: Takashi Iwai @ 2011-03-24 6:43 UTC (permalink / raw) To: Russ Dill; +Cc: linux-kernel At Wed, 23 Mar 2011 18:40:58 -0700, Russ Dill wrote: > > With slub poisoning on, I'm seeing an oops in snd_disconnect_release > with slub poison (6b6b6b6b). Investigating, snd_card_disconnect looks > to be the possible culprit: > > 333 spin_lock(&card->files_lock); > 334 mfile = card->files; > 335 while (mfile) { > 336 file = mfile->file; > 337 > 338 /* it's critical part, use endless loop */ > 339 /* we have no room to fail */ > 340 mfile->disconnected_f_op = mfile->file->f_op; > 341 > 342 spin_lock(&shutdown_lock); > 343 list_add(&mfile->shutdown_list, &shutdown_files); > 344 spin_unlock(&shutdown_lock); > 345 > 346 mfile->file->f_op = &snd_shutdown_f_ops; > 347 fops_get(mfile->file->f_op); > 348 > 349 mfile = mfile->next; > 350 } > 351 spin_unlock(&card->files_lock); > > I'm not aware of any locking that would prevent the original release > function running concurrently if it were started before line 346 was > executed. The original release functions (such as snd_hwdep_release) > call snd_card_remove_file which frees the mfile object which would > have been just added to the shutdown_files list leading to a use of > freed (or in my case poisoned) memory later on down the line when the > shutdown_files gets walked again. > > It seems that 118dd6bf (ALSA: Clean up snd_monitor_file management, > v2.6.30) may have either introduced this problem or made it worse > since snd_card_file_remove previously removed items from the > shutdown_files list before freeing them. > > I'm currently experiencing these crashes on 2.6.32.26, but I can't > find any changes that would cause them not to occur on later kernel > versions. Which device are you using? USB-audio had a known problem at disconnection, for example, and it was fixed recently. Takashi ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: snd_card_disconnect race (sound/core/init.c) 2011-03-24 6:43 ` Takashi Iwai @ 2011-03-24 8:19 ` Russ Dill 2011-03-24 8:58 ` Takashi Iwai 0 siblings, 1 reply; 7+ messages in thread From: Russ Dill @ 2011-03-24 8:19 UTC (permalink / raw) To: Takashi Iwai; +Cc: linux-kernel On Wed, Mar 23, 2011 at 11:43 PM, Takashi Iwai <tiwai@suse.de> wrote: > At Wed, 23 Mar 2011 18:40:58 -0700, > Russ Dill wrote: >> >> With slub poisoning on, I'm seeing an oops in snd_disconnect_release >> with slub poison (6b6b6b6b). Investigating, snd_card_disconnect looks >> to be the possible culprit: >> >> 333 spin_lock(&card->files_lock); >> 334 mfile = card->files; >> 335 while (mfile) { >> 336 file = mfile->file; >> 337 >> 338 /* it's critical part, use endless loop */ >> 339 /* we have no room to fail */ >> 340 mfile->disconnected_f_op = mfile->file->f_op; >> 341 >> 342 spin_lock(&shutdown_lock); >> 343 list_add(&mfile->shutdown_list, &shutdown_files); >> 344 spin_unlock(&shutdown_lock); >> 345 >> 346 mfile->file->f_op = &snd_shutdown_f_ops; >> 347 fops_get(mfile->file->f_op); >> 348 >> 349 mfile = mfile->next; >> 350 } >> 351 spin_unlock(&card->files_lock); >> >> I'm not aware of any locking that would prevent the original release >> function running concurrently if it were started before line 346 was >> executed. The original release functions (such as snd_hwdep_release) >> call snd_card_remove_file which frees the mfile object which would >> have been just added to the shutdown_files list leading to a use of >> freed (or in my case poisoned) memory later on down the line when the >> shutdown_files gets walked again. >> >> It seems that 118dd6bf (ALSA: Clean up snd_monitor_file management, >> v2.6.30) may have either introduced this problem or made it worse >> since snd_card_file_remove previously removed items from the >> shutdown_files list before freeing them. >> >> I'm currently experiencing these crashes on 2.6.32.26, but I can't >> find any changes that would cause them not to occur on later kernel >> versions. > > Which device are you using? USB-audio had a known problem at > disconnection, for example, and it was fixed recently. > I'm using USB-audio. I tried back-porting a couple of patches, to no avail: ALSA: usb-audio: fix oops due to cleanup race when disconnecting http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=382225e6 ALSA: usb - Release capture substream URBs properly http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=76195fb0 Backing out 118dd6bf fixes the problem. The app that is running when this crash occurs does full duplex streaming and segfaults while the USB-audio device is going away. I'd have to check, but I think its actually segfaulting it response to a disconnect of another piece of the composite device the USB-audio device is part of, so the release may actually be happening at the time the snd_card_disconnect is occurring. I don't see any locking that would prevent bad things from happening in this case, and the shutdown_files list is definitely poisoned. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: snd_card_disconnect race (sound/core/init.c) 2011-03-24 8:19 ` Russ Dill @ 2011-03-24 8:58 ` Takashi Iwai 2011-03-24 17:23 ` Russ Dill 0 siblings, 1 reply; 7+ messages in thread From: Takashi Iwai @ 2011-03-24 8:58 UTC (permalink / raw) To: Russ Dill; +Cc: linux-kernel At Thu, 24 Mar 2011 01:19:43 -0700, Russ Dill wrote: > > On Wed, Mar 23, 2011 at 11:43 PM, Takashi Iwai <tiwai@suse.de> wrote: > > At Wed, 23 Mar 2011 18:40:58 -0700, > > Russ Dill wrote: > >> > >> With slub poisoning on, I'm seeing an oops in snd_disconnect_release > >> with slub poison (6b6b6b6b). Investigating, snd_card_disconnect looks > >> to be the possible culprit: > >> > >> 333 spin_lock(&card->files_lock); > >> 334 mfile = card->files; > >> 335 while (mfile) { > >> 336 file = mfile->file; > >> 337 > >> 338 /* it's critical part, use endless loop */ > >> 339 /* we have no room to fail */ > >> 340 mfile->disconnected_f_op = mfile->file->f_op; > >> 341 > >> 342 spin_lock(&shutdown_lock); > >> 343 list_add(&mfile->shutdown_list, &shutdown_files); > >> 344 spin_unlock(&shutdown_lock); > >> 345 > >> 346 mfile->file->f_op = &snd_shutdown_f_ops; > >> 347 fops_get(mfile->file->f_op); > >> 348 > >> 349 mfile = mfile->next; > >> 350 } > >> 351 spin_unlock(&card->files_lock); > >> > >> I'm not aware of any locking that would prevent the original release > >> function running concurrently if it were started before line 346 was > >> executed. The original release functions (such as snd_hwdep_release) > >> call snd_card_remove_file which frees the mfile object which would > >> have been just added to the shutdown_files list leading to a use of > >> freed (or in my case poisoned) memory later on down the line when the > >> shutdown_files gets walked again. > >> > >> It seems that 118dd6bf (ALSA: Clean up snd_monitor_file management, > >> v2.6.30) may have either introduced this problem or made it worse > >> since snd_card_file_remove previously removed items from the > >> shutdown_files list before freeing them. > >> > >> I'm currently experiencing these crashes on 2.6.32.26, but I can't > >> find any changes that would cause them not to occur on later kernel > >> versions. > > > > Which device are you using? USB-audio had a known problem at > > disconnection, for example, and it was fixed recently. > > > > I'm using USB-audio. I tried back-porting a couple of patches, to no avail: > > ALSA: usb-audio: fix oops due to cleanup race when disconnecting > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=382225e6 > > ALSA: usb - Release capture substream URBs properly > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=76195fb0 > > Backing out 118dd6bf fixes the problem. The app that is running when > this crash occurs does full duplex streaming and segfaults while the > USB-audio device is going away. I'd have to check, but I think its > actually segfaulting it response to a disconnect of another piece of > the composite device the USB-audio device is part of, so the release > may actually be happening at the time the snd_card_disconnect is > occurring. I don't see any locking that would prevent bad things from > happening in this case, and the shutdown_files list is definitely > poisoned. OK, what about the patch below? Takashi --- From: Takashi Iwai <tiwai@suse.de> Subject: [PATCH] ALSA: Fix yet another race in disconnection This patch fixes a race between snd_card_file_remove() and snd_card_disconnect(). When the card is added to shutdown_files list in snd_card_disconnect(), but it's freed in snd_card_file_remove() at the same time, the shutdown_files list gets corrupted. The list member must be freed in snd_card_file_remove() as well. Reported-by: Russ Dill <russ.dill@gmail.com> Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/core/init.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/sound/core/init.c b/sound/core/init.c index 3e65da2..a0080aa 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -848,6 +848,7 @@ int snd_card_file_add(struct snd_card *card, struct file *file) return -ENOMEM; mfile->file = file; mfile->disconnected_f_op = NULL; + INIT_LIST_HEAD(&mfile->shutdown_list); spin_lock(&card->files_lock); if (card->shutdown) { spin_unlock(&card->files_lock); @@ -883,6 +884,9 @@ int snd_card_file_remove(struct snd_card *card, struct file *file) list_for_each_entry(mfile, &card->files_list, list) { if (mfile->file == file) { list_del(&mfile->list); + spin_lock(&shutdown_lock); + list_del(&mfile->shutdown_list); + spin_unlock(&shutdown_lock); if (mfile->disconnected_f_op) fops_put(mfile->disconnected_f_op); found = mfile; -- 1.7.4.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: snd_card_disconnect race (sound/core/init.c) 2011-03-24 8:58 ` Takashi Iwai @ 2011-03-24 17:23 ` Russ Dill 2011-03-25 17:40 ` Russ Dill 0 siblings, 1 reply; 7+ messages in thread From: Russ Dill @ 2011-03-24 17:23 UTC (permalink / raw) To: Takashi Iwai; +Cc: linux-kernel On Thu, Mar 24, 2011 at 1:58 AM, Takashi Iwai <tiwai@suse.de> wrote: > At Thu, 24 Mar 2011 01:19:43 -0700, > Russ Dill wrote: >> >> On Wed, Mar 23, 2011 at 11:43 PM, Takashi Iwai <tiwai@suse.de> wrote: >> > At Wed, 23 Mar 2011 18:40:58 -0700, >> > Russ Dill wrote: >> >> >> >> With slub poisoning on, I'm seeing an oops in snd_disconnect_release >> >> with slub poison (6b6b6b6b). Investigating, snd_card_disconnect looks >> >> to be the possible culprit: >> >> >> >> 333 spin_lock(&card->files_lock); >> >> 334 mfile = card->files; >> >> 335 while (mfile) { >> >> 336 file = mfile->file; >> >> 337 >> >> 338 /* it's critical part, use endless loop */ >> >> 339 /* we have no room to fail */ >> >> 340 mfile->disconnected_f_op = mfile->file->f_op; >> >> 341 >> >> 342 spin_lock(&shutdown_lock); >> >> 343 list_add(&mfile->shutdown_list, &shutdown_files); >> >> 344 spin_unlock(&shutdown_lock); >> >> 345 >> >> 346 mfile->file->f_op = &snd_shutdown_f_ops; >> >> 347 fops_get(mfile->file->f_op); >> >> 348 >> >> 349 mfile = mfile->next; >> >> 350 } >> >> 351 spin_unlock(&card->files_lock); >> >> >> >> I'm not aware of any locking that would prevent the original release >> >> function running concurrently if it were started before line 346 was >> >> executed. The original release functions (such as snd_hwdep_release) >> >> call snd_card_remove_file which frees the mfile object which would >> >> have been just added to the shutdown_files list leading to a use of >> >> freed (or in my case poisoned) memory later on down the line when the >> >> shutdown_files gets walked again. >> >> >> >> It seems that 118dd6bf (ALSA: Clean up snd_monitor_file management, >> >> v2.6.30) may have either introduced this problem or made it worse >> >> since snd_card_file_remove previously removed items from the >> >> shutdown_files list before freeing them. >> >> >> >> I'm currently experiencing these crashes on 2.6.32.26, but I can't >> >> find any changes that would cause them not to occur on later kernel >> >> versions. >> > >> > Which device are you using? USB-audio had a known problem at >> > disconnection, for example, and it was fixed recently. >> > >> >> I'm using USB-audio. I tried back-porting a couple of patches, to no avail: >> >> ALSA: usb-audio: fix oops due to cleanup race when disconnecting >> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=382225e6 >> >> ALSA: usb - Release capture substream URBs properly >> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=76195fb0 >> >> Backing out 118dd6bf fixes the problem. The app that is running when >> this crash occurs does full duplex streaming and segfaults while the >> USB-audio device is going away. I'd have to check, but I think its >> actually segfaulting it response to a disconnect of another piece of >> the composite device the USB-audio device is part of, so the release >> may actually be happening at the time the snd_card_disconnect is >> occurring. I don't see any locking that would prevent bad things from >> happening in this case, and the shutdown_files list is definitely >> poisoned. > > OK, what about the patch below? > > > Takashi > > --- > From: Takashi Iwai <tiwai@suse.de> > Subject: [PATCH] ALSA: Fix yet another race in disconnection > > This patch fixes a race between snd_card_file_remove() and > snd_card_disconnect(). When the card is added to shutdown_files list > in snd_card_disconnect(), but it's freed in snd_card_file_remove() at > the same time, the shutdown_files list gets corrupted. The list member > must be freed in snd_card_file_remove() as well. > > Reported-by: Russ Dill <russ.dill@gmail.com> > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > sound/core/init.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/sound/core/init.c b/sound/core/init.c > index 3e65da2..a0080aa 100644 > --- a/sound/core/init.c > +++ b/sound/core/init.c > @@ -848,6 +848,7 @@ int snd_card_file_add(struct snd_card *card, struct file *file) > return -ENOMEM; > mfile->file = file; > mfile->disconnected_f_op = NULL; > + INIT_LIST_HEAD(&mfile->shutdown_list); > spin_lock(&card->files_lock); > if (card->shutdown) { > spin_unlock(&card->files_lock); > @@ -883,6 +884,9 @@ int snd_card_file_remove(struct snd_card *card, struct file *file) > list_for_each_entry(mfile, &card->files_list, list) { > if (mfile->file == file) { > list_del(&mfile->list); > + spin_lock(&shutdown_lock); > + list_del(&mfile->shutdown_list); > + spin_unlock(&shutdown_lock); > if (mfile->disconnected_f_op) > fops_put(mfile->disconnected_f_op); > found = mfile; > -- > 1.7.4.1 It looks sane, I'll run it through my torture (about 24hr) testing and let you know. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: snd_card_disconnect race (sound/core/init.c) 2011-03-24 17:23 ` Russ Dill @ 2011-03-25 17:40 ` Russ Dill 2011-03-25 18:24 ` Takashi Iwai 0 siblings, 1 reply; 7+ messages in thread From: Russ Dill @ 2011-03-25 17:40 UTC (permalink / raw) To: Takashi Iwai; +Cc: linux-kernel On Thu, Mar 24, 2011 at 10:23 AM, Russ Dill <russ.dill@gmail.com> wrote: > On Thu, Mar 24, 2011 at 1:58 AM, Takashi Iwai <tiwai@suse.de> wrote: >> At Thu, 24 Mar 2011 01:19:43 -0700, >> Russ Dill wrote: >>> >>> On Wed, Mar 23, 2011 at 11:43 PM, Takashi Iwai <tiwai@suse.de> wrote: >>> > At Wed, 23 Mar 2011 18:40:58 -0700, >>> > Russ Dill wrote: >>> >> >>> >> With slub poisoning on, I'm seeing an oops in snd_disconnect_release >>> >> with slub poison (6b6b6b6b). Investigating, snd_card_disconnect looks >>> >> to be the possible culprit: >>> >> >>> >> 333 spin_lock(&card->files_lock); >>> >> 334 mfile = card->files; >>> >> 335 while (mfile) { >>> >> 336 file = mfile->file; >>> >> 337 >>> >> 338 /* it's critical part, use endless loop */ >>> >> 339 /* we have no room to fail */ >>> >> 340 mfile->disconnected_f_op = mfile->file->f_op; >>> >> 341 >>> >> 342 spin_lock(&shutdown_lock); >>> >> 343 list_add(&mfile->shutdown_list, &shutdown_files); >>> >> 344 spin_unlock(&shutdown_lock); >>> >> 345 >>> >> 346 mfile->file->f_op = &snd_shutdown_f_ops; >>> >> 347 fops_get(mfile->file->f_op); >>> >> 348 >>> >> 349 mfile = mfile->next; >>> >> 350 } >>> >> 351 spin_unlock(&card->files_lock); >>> >> >>> >> I'm not aware of any locking that would prevent the original release >>> >> function running concurrently if it were started before line 346 was >>> >> executed. The original release functions (such as snd_hwdep_release) >>> >> call snd_card_remove_file which frees the mfile object which would >>> >> have been just added to the shutdown_files list leading to a use of >>> >> freed (or in my case poisoned) memory later on down the line when the >>> >> shutdown_files gets walked again. >>> >> >>> >> It seems that 118dd6bf (ALSA: Clean up snd_monitor_file management, >>> >> v2.6.30) may have either introduced this problem or made it worse >>> >> since snd_card_file_remove previously removed items from the >>> >> shutdown_files list before freeing them. >>> >> >>> >> I'm currently experiencing these crashes on 2.6.32.26, but I can't >>> >> find any changes that would cause them not to occur on later kernel >>> >> versions. >>> > >>> > Which device are you using? USB-audio had a known problem at >>> > disconnection, for example, and it was fixed recently. >>> > >>> >>> I'm using USB-audio. I tried back-porting a couple of patches, to no avail: >>> >>> ALSA: usb-audio: fix oops due to cleanup race when disconnecting >>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=382225e6 >>> >>> ALSA: usb - Release capture substream URBs properly >>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=76195fb0 >>> >>> Backing out 118dd6bf fixes the problem. The app that is running when >>> this crash occurs does full duplex streaming and segfaults while the >>> USB-audio device is going away. I'd have to check, but I think its >>> actually segfaulting it response to a disconnect of another piece of >>> the composite device the USB-audio device is part of, so the release >>> may actually be happening at the time the snd_card_disconnect is >>> occurring. I don't see any locking that would prevent bad things from >>> happening in this case, and the shutdown_files list is definitely >>> poisoned. >> >> OK, what about the patch below? >> >> >> Takashi >> >> --- >> From: Takashi Iwai <tiwai@suse.de> >> Subject: [PATCH] ALSA: Fix yet another race in disconnection >> >> This patch fixes a race between snd_card_file_remove() and >> snd_card_disconnect(). When the card is added to shutdown_files list >> in snd_card_disconnect(), but it's freed in snd_card_file_remove() at >> the same time, the shutdown_files list gets corrupted. The list member >> must be freed in snd_card_file_remove() as well. >> >> Reported-by: Russ Dill <russ.dill@gmail.com> >> Signed-off-by: Takashi Iwai <tiwai@suse.de> >> --- >> sound/core/init.c | 4 ++++ >> 1 files changed, 4 insertions(+), 0 deletions(-) >> >> diff --git a/sound/core/init.c b/sound/core/init.c >> index 3e65da2..a0080aa 100644 >> --- a/sound/core/init.c >> +++ b/sound/core/init.c >> @@ -848,6 +848,7 @@ int snd_card_file_add(struct snd_card *card, struct file *file) >> return -ENOMEM; >> mfile->file = file; >> mfile->disconnected_f_op = NULL; >> + INIT_LIST_HEAD(&mfile->shutdown_list); >> spin_lock(&card->files_lock); >> if (card->shutdown) { >> spin_unlock(&card->files_lock); >> @@ -883,6 +884,9 @@ int snd_card_file_remove(struct snd_card *card, struct file *file) >> list_for_each_entry(mfile, &card->files_list, list) { >> if (mfile->file == file) { >> list_del(&mfile->list); >> + spin_lock(&shutdown_lock); >> + list_del(&mfile->shutdown_list); >> + spin_unlock(&shutdown_lock); >> if (mfile->disconnected_f_op) >> fops_put(mfile->disconnected_f_op); >> found = mfile; >> -- >> 1.7.4.1 > > It looks sane, I'll run it through my torture (about 24hr) testing and > let you know. > Passes testing and looks good! Thanks. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: snd_card_disconnect race (sound/core/init.c) 2011-03-25 17:40 ` Russ Dill @ 2011-03-25 18:24 ` Takashi Iwai 0 siblings, 0 replies; 7+ messages in thread From: Takashi Iwai @ 2011-03-25 18:24 UTC (permalink / raw) To: Russ Dill; +Cc: linux-kernel At Fri, 25 Mar 2011 10:40:50 -0700, Russ Dill wrote: > > On Thu, Mar 24, 2011 at 10:23 AM, Russ Dill <russ.dill@gmail.com> wrote: > > On Thu, Mar 24, 2011 at 1:58 AM, Takashi Iwai <tiwai@suse.de> wrote: > >> At Thu, 24 Mar 2011 01:19:43 -0700, > >> Russ Dill wrote: > >>> > >>> On Wed, Mar 23, 2011 at 11:43 PM, Takashi Iwai <tiwai@suse.de> wrote: > >>> > At Wed, 23 Mar 2011 18:40:58 -0700, > >>> > Russ Dill wrote: > >>> >> > >>> >> With slub poisoning on, I'm seeing an oops in snd_disconnect_release > >>> >> with slub poison (6b6b6b6b). Investigating, snd_card_disconnect looks > >>> >> to be the possible culprit: > >>> >> > >>> >> 333 spin_lock(&card->files_lock); > >>> >> 334 mfile = card->files; > >>> >> 335 while (mfile) { > >>> >> 336 file = mfile->file; > >>> >> 337 > >>> >> 338 /* it's critical part, use endless loop */ > >>> >> 339 /* we have no room to fail */ > >>> >> 340 mfile->disconnected_f_op = mfile->file->f_op; > >>> >> 341 > >>> >> 342 spin_lock(&shutdown_lock); > >>> >> 343 list_add(&mfile->shutdown_list, &shutdown_files); > >>> >> 344 spin_unlock(&shutdown_lock); > >>> >> 345 > >>> >> 346 mfile->file->f_op = &snd_shutdown_f_ops; > >>> >> 347 fops_get(mfile->file->f_op); > >>> >> 348 > >>> >> 349 mfile = mfile->next; > >>> >> 350 } > >>> >> 351 spin_unlock(&card->files_lock); > >>> >> > >>> >> I'm not aware of any locking that would prevent the original release > >>> >> function running concurrently if it were started before line 346 was > >>> >> executed. The original release functions (such as snd_hwdep_release) > >>> >> call snd_card_remove_file which frees the mfile object which would > >>> >> have been just added to the shutdown_files list leading to a use of > >>> >> freed (or in my case poisoned) memory later on down the line when the > >>> >> shutdown_files gets walked again. > >>> >> > >>> >> It seems that 118dd6bf (ALSA: Clean up snd_monitor_file management, > >>> >> v2.6.30) may have either introduced this problem or made it worse > >>> >> since snd_card_file_remove previously removed items from the > >>> >> shutdown_files list before freeing them. > >>> >> > >>> >> I'm currently experiencing these crashes on 2.6.32.26, but I can't > >>> >> find any changes that would cause them not to occur on later kernel > >>> >> versions. > >>> > > >>> > Which device are you using? USB-audio had a known problem at > >>> > disconnection, for example, and it was fixed recently. > >>> > > >>> > >>> I'm using USB-audio. I tried back-porting a couple of patches, to no avail: > >>> > >>> ALSA: usb-audio: fix oops due to cleanup race when disconnecting > >>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=382225e6 > >>> > >>> ALSA: usb - Release capture substream URBs properly > >>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=76195fb0 > >>> > >>> Backing out 118dd6bf fixes the problem. The app that is running when > >>> this crash occurs does full duplex streaming and segfaults while the > >>> USB-audio device is going away. I'd have to check, but I think its > >>> actually segfaulting it response to a disconnect of another piece of > >>> the composite device the USB-audio device is part of, so the release > >>> may actually be happening at the time the snd_card_disconnect is > >>> occurring. I don't see any locking that would prevent bad things from > >>> happening in this case, and the shutdown_files list is definitely > >>> poisoned. > >> > >> OK, what about the patch below? > >> > >> > >> Takashi > >> > >> --- > >> From: Takashi Iwai <tiwai@suse.de> > >> Subject: [PATCH] ALSA: Fix yet another race in disconnection > >> > >> This patch fixes a race between snd_card_file_remove() and > >> snd_card_disconnect(). When the card is added to shutdown_files list > >> in snd_card_disconnect(), but it's freed in snd_card_file_remove() at > >> the same time, the shutdown_files list gets corrupted. The list member > >> must be freed in snd_card_file_remove() as well. > >> > >> Reported-by: Russ Dill <russ.dill@gmail.com> > >> Signed-off-by: Takashi Iwai <tiwai@suse.de> > >> --- > >> sound/core/init.c | 4 ++++ > >> 1 files changed, 4 insertions(+), 0 deletions(-) > >> > >> diff --git a/sound/core/init.c b/sound/core/init.c > >> index 3e65da2..a0080aa 100644 > >> --- a/sound/core/init.c > >> +++ b/sound/core/init.c > >> @@ -848,6 +848,7 @@ int snd_card_file_add(struct snd_card *card, struct file *file) > >> return -ENOMEM; > >> mfile->file = file; > >> mfile->disconnected_f_op = NULL; > >> + INIT_LIST_HEAD(&mfile->shutdown_list); > >> spin_lock(&card->files_lock); > >> if (card->shutdown) { > >> spin_unlock(&card->files_lock); > >> @@ -883,6 +884,9 @@ int snd_card_file_remove(struct snd_card *card, struct file *file) > >> list_for_each_entry(mfile, &card->files_list, list) { > >> if (mfile->file == file) { > >> list_del(&mfile->list); > >> + spin_lock(&shutdown_lock); > >> + list_del(&mfile->shutdown_list); > >> + spin_unlock(&shutdown_lock); > >> if (mfile->disconnected_f_op) > >> fops_put(mfile->disconnected_f_op); > >> found = mfile; > >> -- > >> 1.7.4.1 > > > > It looks sane, I'll run it through my torture (about 24hr) testing and > > let you know. > > > > Passes testing and looks good! Thanks. Thanks for testing. I merged the patch now. Takashi ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-03-25 18:24 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-03-24 1:40 snd_card_disconnect race (sound/core/init.c) Russ Dill 2011-03-24 6:43 ` Takashi Iwai 2011-03-24 8:19 ` Russ Dill 2011-03-24 8:58 ` Takashi Iwai 2011-03-24 17:23 ` Russ Dill 2011-03-25 17:40 ` Russ Dill 2011-03-25 18:24 ` Takashi Iwai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®