Re: possible deadlock in snd_rawmidi_free

[Takashi Iwai <tiwai@xxxxxxx>]

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