FAILED: patch "[PATCH] ALSA: timer: Fix mutex deadlock at releasing card" failed to apply to 4.19-stable tree
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(®ister_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(®ister_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(®ister_mutex); - err = snd_timer_close_locked(timeri); + err = snd_timer_close_locked(timeri, &card_dev_to_put); mutex_unlock(®ister_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);
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.
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
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?
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
linux-stable-mirror@lists.linaro.org
-
gregkh＠linuxfoundation.org -
Sasha Levin -
Takashi Iwai