Re: possible deadlock in snd_rawmidi_free

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@suse.de 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