Re: possible deadlock in snd_rawmidi_free

[Rondreis]

Thank you for your solution! I am sorry that I was not able to extract
the reproducer. But I am improving the solution to reproduce the crash.
If I succeed in reproducing it, I will check it for you as soon as possible!

Best Regards,
Rondreis

On Tue, Sep 20, 2022 at 7:35 PM Takashi Iwai <tiwai@xxxxxxx> wrote:
>
> On Mon, 19 Sep 2022 15:49:02 +0200,
> Takashi Iwai wrote:
> >
> > On Mon, 19 Sep 2022 14:46:13 +0200,
> > Rondreis wrote:
> > >
> > > Hello,
> > >
> > > When fuzzing the Linux kernel driver v6.0-rc4, the following crash was
> > > triggered.
> > >
> > > HEAD commit: 7e18e42e4b280c85b76967a9106a13ca61c16179
> > > git tree: upstream
> > >
> > > kernel config: https://pastebin.com/raw/xtrgsXP3
> > > console output: https://pastebin.com/raw/9tabWDtu
> > >
> > > Sorry for failing to extract the reproducer, and the crash occurred at
> > > the moment of disconnecting the midi device. On other versions of
> > > Linux, I also triggered this crash.
> > >
> > > I would appreciate it if you have any idea how to solve this bug.
> >
> > I think there are two ways to work around it.
> >
> > The first one is to move the unregister_sound*() calls out of the
> > sound_oss_mutex, something like:
> > -- 8< --
> >
> > --- a/sound/core/sound_oss.c
> > +++ b/sound/core/sound_oss.c
> > @@ -162,7 +162,6 @@ int snd_unregister_oss_device(int type, struct snd_card *card, int dev)
> >               mutex_unlock(&sound_oss_mutex);
> >               return -ENOENT;
> >       }
> > -     unregister_sound_special(minor);
> >       switch (SNDRV_MINOR_OSS_DEVICE(minor)) {
> >       case SNDRV_MINOR_OSS_PCM:
> >               track2 = SNDRV_MINOR_OSS(cidx, SNDRV_MINOR_OSS_AUDIO);
> > @@ -174,12 +173,18 @@ int snd_unregister_oss_device(int type, struct snd_card *card, int dev)
> >               track2 = SNDRV_MINOR_OSS(cidx, SNDRV_MINOR_OSS_DMMIDI1);
> >               break;
> >       }
> > -     if (track2 >= 0) {
> > -             unregister_sound_special(track2);
> > +     if (track2 >= 0)
> >               snd_oss_minors[track2] = NULL;
> > -     }
> >       snd_oss_minors[minor] = NULL;
> >       mutex_unlock(&sound_oss_mutex);
> > +
> > +     /* call unregister_sound_special() outside sound_oss_mutex;
> > +      * otherwise may deadlock, as it can trigger the release of a card
> > +      */
> > +     unregister_sound_special(minor);
> > +     if (track2 >= 0)
> > +             unregister_sound_special(track2);
> > +
> >       kfree(mptr);
> >       return 0;
> >  }
> > -- 8< --
> >
> > This should be OK, as the unregister_sound_*() itself can be called
> > concurrently.
> >
> > Another workaround would be just to remove the register_mutex call at
> > snd_rawmidi_free(), e.g. something like:
> >
> > -- 8< --
> > --- a/sound/core/rawmidi.c
> > +++ b/sound/core/rawmidi.c
> > @@ -1899,10 +1899,8 @@ static int snd_rawmidi_free(struct snd_rawmidi *rmidi)
> >
> >       snd_info_free_entry(rmidi->proc_entry);
> >       rmidi->proc_entry = NULL;
> > -     mutex_lock(&register_mutex);
> >       if (rmidi->ops && rmidi->ops->dev_unregister)
> >               rmidi->ops->dev_unregister(rmidi);
> > -     mutex_unlock(&register_mutex);
> >
> >       snd_rawmidi_free_substreams(&rmidi->streams[SNDRV_RAWMIDI_STREAM_INPUT]);
> >       snd_rawmidi_free_substreams(&rmidi->streams[SNDRV_RAWMIDI_STREAM_OUTPUT]);
> > -- 8< --
> >
> > This register_mutex there should be superfluous since the device has
> > been already processed and detached by snd_rawmidi_dev_disconnect()
> > beforehand.  But if the first one is confirmed to work, the second one
> > can be left untouched.
>
> Could you check whether one of two changes above fixes the bug?
> Once after confirmed, I'll cook a proper patch for the submission.
>
>
> thanks,
>
> Takashi