Re: snd_card_disconnect race (sound/core/init.c)

[Russ Dill]

On Thu, Mar 24, 2011 at 10:23 AM, Russ Dill <russ.dill@xxxxxxxxx> wrote:
> On Thu, Mar 24, 2011 at 1:58 AM, Takashi Iwai <tiwai@xxxxxxx> 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@xxxxxxx> 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@xxxxxxx>
>> 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@xxxxxxxxx>
>> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
>> ---
>> Â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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/