FAILED: patch "[PATCH] ALSA: timer: Fix mutex deadlock at releasing card" failed to apply to 4.19-stable tree

FAILED: patch "[PATCH] ALSA: timer: Fix mutex deadlock at releasing card" failed to apply to 4.19-stable tree

[gregkh]

The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm
tree, then please email the backport, including the original git commit
id to <stable@vger.kernel.org>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From a39331867335d4a94b6165e306265c9e24aca073 Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Wed, 30 Oct 2019 22:42:57 +0100
Subject: [PATCH] ALSA: timer: Fix mutex deadlock at releasing card

When a card is disconnected while in use, the system waits until all
opened files are closed then releases the card.  This is done via
put_device() of the card device in each device release code.

The recently reported mutex deadlock bug happens in this code path;
snd_timer_close() for the timer device deals with the global
register_mutex and it calls put_device() there.  When this timer
device is the last one, the card gets freed and it eventually calls
snd_timer_free(), which has again the protection with the global
register_mutex -- boom.

Basically put_device() call itself is race-free, so a relative simple
workaround is to move this put_device() call out of the mutex.  For
achieving that, in this patch, snd_timer_close_locked() got a new
argument to store the card device pointer in return, and each caller
invokes put_device() with the returned object after the mutex unlock.

Reported-and-tested-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>

diff --git a/sound/core/timer.c b/sound/core/timer.c
index 5c9fbf3f4340..6b724d2ee2de 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -226,7 +226,8 @@ static int snd_timer_check_master(struct snd_timer_instance *master)
 	return 0;
 }
 
-static int snd_timer_close_locked(struct snd_timer_instance *timeri);
+static int snd_timer_close_locked(struct snd_timer_instance *timeri,
+				  struct device **card_devp_to_put);
 
 /*
  * open a timer instance
@@ -238,6 +239,7 @@ int snd_timer_open(struct snd_timer_instance **ti,
 {
 	struct snd_timer *timer;
 	struct snd_timer_instance *timeri = NULL;
+	struct device *card_dev_to_put = NULL;
 	int err;
 
 	mutex_lock(&register_mutex);
@@ -261,7 +263,7 @@ int snd_timer_open(struct snd_timer_instance **ti,
 		list_add_tail(&timeri->open_list, &snd_timer_slave_list);
 		err = snd_timer_check_slave(timeri);
 		if (err < 0) {
-			snd_timer_close_locked(timeri);
+			snd_timer_close_locked(timeri, &card_dev_to_put);
 			timeri = NULL;
 		}
 		goto unlock;
@@ -313,7 +315,7 @@ int snd_timer_open(struct snd_timer_instance **ti,
 			timeri = NULL;
 
 			if (timer->card)
-				put_device(&timer->card->card_dev);
+				card_dev_to_put = &timer->card->card_dev;
 			module_put(timer->module);
 			goto unlock;
 		}
@@ -323,12 +325,15 @@ int snd_timer_open(struct snd_timer_instance **ti,
 	timer->num_instances++;
 	err = snd_timer_check_master(timeri);
 	if (err < 0) {
-		snd_timer_close_locked(timeri);
+		snd_timer_close_locked(timeri, &card_dev_to_put);
 		timeri = NULL;
 	}
 
  unlock:
 	mutex_unlock(&register_mutex);
+	/* put_device() is called after unlock for avoiding deadlock */
+	if (card_dev_to_put)
+		put_device(card_dev_to_put);
 	*ti = timeri;
 	return err;
 }
@@ -338,7 +343,8 @@ EXPORT_SYMBOL(snd_timer_open);
  * close a timer instance
  * call this with register_mutex down.
  */
-static int snd_timer_close_locked(struct snd_timer_instance *timeri)
+static int snd_timer_close_locked(struct snd_timer_instance *timeri,
+				  struct device **card_devp_to_put)
 {
 	struct snd_timer *timer = timeri->timer;
 	struct snd_timer_instance *slave, *tmp;
@@ -395,7 +401,7 @@ static int snd_timer_close_locked(struct snd_timer_instance *timeri)
 			timer->hw.close(timer);
 		/* release a card refcount for safe disconnection */
 		if (timer->card)
-			put_device(&timer->card->card_dev);
+			*card_devp_to_put = &timer->card->card_dev;
 		module_put(timer->module);
 	}
 
@@ -407,14 +413,18 @@ static int snd_timer_close_locked(struct snd_timer_instance *timeri)
  */
 int snd_timer_close(struct snd_timer_instance *timeri)
 {
+	struct device *card_dev_to_put = NULL;
 	int err;
 
 	if (snd_BUG_ON(!timeri))
 		return -ENXIO;
 
 	mutex_lock(&register_mutex);
-	err = snd_timer_close_locked(timeri);
+	err = snd_timer_close_locked(timeri, &card_dev_to_put);
 	mutex_unlock(&register_mutex);
+	/* put_device() is called after unlock for avoiding deadlock */
+	if (card_dev_to_put)
+		put_device(card_dev_to_put);
 	return err;
 }
 EXPORT_SYMBOL(snd_timer_close);

Re: FAILED: patch "[PATCH] ALSA: timer: Fix mutex deadlock at releasing card" failed to apply to 4.19-stable tree

[Sasha Levin]

On Sun, Nov 03, 2019 at 06:40:59PM +0100, gregkh@linuxfoundation.org wrote:
>
>The patch below does not apply to the 4.19-stable tree.
>If someone wants it applied there, or to any other stable or longterm
>tree, then please email the backport, including the original git commit
>id to <stable@vger.kernel.org>.
>
>thanks,
>
>greg k-h
>
>------------------ original commit in Linus's tree ------------------
>
>From a39331867335d4a94b6165e306265c9e24aca073 Mon Sep 17 00:00:00 2001
>From: Takashi Iwai <tiwai@suse.de>
>Date: Wed, 30 Oct 2019 22:42:57 +0100
>Subject: [PATCH] ALSA: timer: Fix mutex deadlock at releasing card
>
>When a card is disconnected while in use, the system waits until all
>opened files are closed then releases the card.  This is done via
>put_device() of the card device in each device release code.
>
>The recently reported mutex deadlock bug happens in this code path;
>snd_timer_close() for the timer device deals with the global
>register_mutex and it calls put_device() there.  When this timer
>device is the last one, the card gets freed and it eventually calls
>snd_timer_free(), which has again the protection with the global
>register_mutex -- boom.
>
>Basically put_device() call itself is race-free, so a relative simple
>workaround is to move this put_device() call out of the mutex.  For
>achieving that, in this patch, snd_timer_close_locked() got a new
>argument to store the card device pointer in return, and each caller
>invokes put_device() with the returned object after the mutex unlock.
>
>Reported-and-tested-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>Cc: <stable@vger.kernel.org>
>Signed-off-by: Takashi Iwai <tiwai@suse.de>

Looks like this was introduced by 41672c0c24a6 ("ALSA: timer: Simplify
error path in snd_timer_open()"), which means it's not needed on 4.19 or
older.

-- 
Thanks,
Sasha

Re: FAILED: patch "[PATCH] ALSA: timer: Fix mutex deadlock at releasing card" failed to apply to 4.19-stable tree

[Takashi Iwai]

On Mon, 04 Nov 2019 11:30:20 +0100,
Sasha Levin wrote:
> 
> On Sun, Nov 03, 2019 at 06:40:59PM +0100, gregkh@linuxfoundation.org wrote:
> >
> >The patch below does not apply to the 4.19-stable tree.
> >If someone wants it applied there, or to any other stable or longterm
> >tree, then please email the backport, including the original git commit
> >id to <stable@vger.kernel.org>.
> >
> >thanks,
> >
> >greg k-h
> >
> >------------------ original commit in Linus's tree ------------------
> >
> >From a39331867335d4a94b6165e306265c9e24aca073 Mon Sep 17 00:00:00 2001
> >From: Takashi Iwai <tiwai@suse.de>
> >Date: Wed, 30 Oct 2019 22:42:57 +0100
> >Subject: [PATCH] ALSA: timer: Fix mutex deadlock at releasing card
> >
> >When a card is disconnected while in use, the system waits until all
> >opened files are closed then releases the card.  This is done via
> >put_device() of the card device in each device release code.
> >
> >The recently reported mutex deadlock bug happens in this code path;
> >snd_timer_close() for the timer device deals with the global
> >register_mutex and it calls put_device() there.  When this timer
> >device is the last one, the card gets freed and it eventually calls
> >snd_timer_free(), which has again the protection with the global
> >register_mutex -- boom.
> >
> >Basically put_device() call itself is race-free, so a relative simple
> >workaround is to move this put_device() call out of the mutex.  For
> >achieving that, in this patch, snd_timer_close_locked() got a new
> >argument to store the card device pointer in return, and each caller
> >invokes put_device() with the returned object after the mutex unlock.
> >
> >Reported-and-tested-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> >Cc: <stable@vger.kernel.org>
> >Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> Looks like this was introduced by 41672c0c24a6 ("ALSA: timer: Simplify
> error path in snd_timer_open()"), which means it's not needed on 4.19 or
> older.

We'd still need a similar fix, as the code path in question is about
closing, not opening the device.  If backporting the commit
41672c0c24a6 makes the fix cleanly applicable, it'd be worth to
backport both.

If not, I can submit a modified 4.19.y patch, too.


thanks,

Takashi

Re: FAILED: patch "[PATCH] ALSA: timer: Fix mutex deadlock at releasing card" failed to apply to 4.19-stable tree

[Sasha Levin]

On Mon, Nov 04, 2019 at 11:42:14AM +0100, Takashi Iwai wrote:
>On Mon, 04 Nov 2019 11:30:20 +0100,
>Sasha Levin wrote:
>>
>> On Sun, Nov 03, 2019 at 06:40:59PM +0100, gregkh@linuxfoundation.org wrote:
>> >
>> >The patch below does not apply to the 4.19-stable tree.
>> >If someone wants it applied there, or to any other stable or longterm
>> >tree, then please email the backport, including the original git commit
>> >id to <stable@vger.kernel.org>.
>> >
>> >thanks,
>> >
>> >greg k-h
>> >
>> >------------------ original commit in Linus's tree ------------------
>> >
>> >From a39331867335d4a94b6165e306265c9e24aca073 Mon Sep 17 00:00:00 2001
>> >From: Takashi Iwai <tiwai@suse.de>
>> >Date: Wed, 30 Oct 2019 22:42:57 +0100
>> >Subject: [PATCH] ALSA: timer: Fix mutex deadlock at releasing card
>> >
>> >When a card is disconnected while in use, the system waits until all
>> >opened files are closed then releases the card.  This is done via
>> >put_device() of the card device in each device release code.
>> >
>> >The recently reported mutex deadlock bug happens in this code path;
>> >snd_timer_close() for the timer device deals with the global
>> >register_mutex and it calls put_device() there.  When this timer
>> >device is the last one, the card gets freed and it eventually calls
>> >snd_timer_free(), which has again the protection with the global
>> >register_mutex -- boom.
>> >
>> >Basically put_device() call itself is race-free, so a relative simple
>> >workaround is to move this put_device() call out of the mutex.  For
>> >achieving that, in this patch, snd_timer_close_locked() got a new
>> >argument to store the card device pointer in return, and each caller
>> >invokes put_device() with the returned object after the mutex unlock.
>> >
>> >Reported-and-tested-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> >Cc: <stable@vger.kernel.org>
>> >Signed-off-by: Takashi Iwai <tiwai@suse.de>
>>
>> Looks like this was introduced by 41672c0c24a6 ("ALSA: timer: Simplify
>> error path in snd_timer_open()"), which means it's not needed on 4.19 or
>> older.
>
>We'd still need a similar fix, as the code path in question is about
>closing, not opening the device.  If backporting the commit
>41672c0c24a6 makes the fix cleanly applicable, it'd be worth to
>backport both.
>
>If not, I can submit a modified 4.19.y patch, too.

Yeah, it works for 4.19 and 4.14, I've queued it up.

The 4.9 backport requires two more commits:

9b7d869ee5a7 ("ALSA: timer: Limit max instances per timer")
988563929d5b ("ALSA: timer: Follow standard EXPORT_SYMBOL() declarations")

Does it makes sense to take them?

-- 
Thanks,
Sasha

Re: FAILED: patch "[PATCH] ALSA: timer: Fix mutex deadlock at releasing card" failed to apply to 4.19-stable tree

[Takashi Iwai]

On Mon, 04 Nov 2019 13:25:38 +0100,
Sasha Levin wrote:
> 
> On Mon, Nov 04, 2019 at 11:42:14AM +0100, Takashi Iwai wrote:
> >On Mon, 04 Nov 2019 11:30:20 +0100,
> >Sasha Levin wrote:
> >>
> >> On Sun, Nov 03, 2019 at 06:40:59PM +0100, gregkh@linuxfoundation.org wrote:
> >> >
> >> >The patch below does not apply to the 4.19-stable tree.
> >> >If someone wants it applied there, or to any other stable or longterm
> >> >tree, then please email the backport, including the original git commit
> >> >id to <stable@vger.kernel.org>.
> >> >
> >> >thanks,
> >> >
> >> >greg k-h
> >> >
> >> >------------------ original commit in Linus's tree ------------------
> >> >
> >> >From a39331867335d4a94b6165e306265c9e24aca073 Mon Sep 17 00:00:00 2001
> >> >From: Takashi Iwai <tiwai@suse.de>
> >> >Date: Wed, 30 Oct 2019 22:42:57 +0100
> >> >Subject: [PATCH] ALSA: timer: Fix mutex deadlock at releasing card
> >> >
> >> >When a card is disconnected while in use, the system waits until all
> >> >opened files are closed then releases the card.  This is done via
> >> >put_device() of the card device in each device release code.
> >> >
> >> >The recently reported mutex deadlock bug happens in this code path;
> >> >snd_timer_close() for the timer device deals with the global
> >> >register_mutex and it calls put_device() there.  When this timer
> >> >device is the last one, the card gets freed and it eventually calls
> >> >snd_timer_free(), which has again the protection with the global
> >> >register_mutex -- boom.
> >> >
> >> >Basically put_device() call itself is race-free, so a relative simple
> >> >workaround is to move this put_device() call out of the mutex.  For
> >> >achieving that, in this patch, snd_timer_close_locked() got a new
> >> >argument to store the card device pointer in return, and each caller
> >> >invokes put_device() with the returned object after the mutex unlock.
> >> >
> >> >Reported-and-tested-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> >> >Cc: <stable@vger.kernel.org>
> >> >Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >>
> >> Looks like this was introduced by 41672c0c24a6 ("ALSA: timer: Simplify
> >> error path in snd_timer_open()"), which means it's not needed on 4.19 or
> >> older.
> >
> >We'd still need a similar fix, as the code path in question is about
> >closing, not opening the device.  If backporting the commit
> >41672c0c24a6 makes the fix cleanly applicable, it'd be worth to
> >backport both.
> >
> >If not, I can submit a modified 4.19.y patch, too.
> 
> Yeah, it works for 4.19 and 4.14, I've queued it up.
> 
> The 4.9 backport requires two more commits:
> 
> 9b7d869ee5a7 ("ALSA: timer: Limit max instances per timer")
> 988563929d5b ("ALSA: timer: Follow standard EXPORT_SYMBOL() declarations")
> 
> Does it makes sense to take them?

Yes, they are fine.  Especially the former should have been merged to
stable trees as much as possible.


thanks,

Takashi