| Message ID | 20231109141954.4283-1-tiwai@suse.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c7a60651953359f98dbf24b43e1bf561e1573ed4 |
| Headers | show |
| Series | ALSA: info: Fix potential deadlock at disconnection | expand |
On 09. 11. 23 15:19, Takashi Iwai wrote: > As reported recently, ALSA core info helper may cause a deadlock at > the forced device disconnection during the procfs operation. > > The proc_remove() (that is called from the snd_card_disconnect() > helper) has a synchronization of the pending procfs accesses via > wait_for_completion(). Meanwhile, ALSA procfs helper takes the global > mutex_lock(&info_mutex) at both the proc_open callback and > snd_card_info_disconnect() helper. Since the proc_open can't finish > due to the mutex lock, wait_for_completion() never returns, either, > hence it deadlocks. Really nice fix. Thanks Takashi. Reviewed-by: Jaroslav Kysela <perex@perex.cz> (maybe Cc: to stable?) Jaroslav
On Thu, 09 Nov 2023 16:04:21 +0100, Jaroslav Kysela wrote: > > On 09. 11. 23 15:19, Takashi Iwai wrote: > > As reported recently, ALSA core info helper may cause a deadlock at > > the forced device disconnection during the procfs operation. > > > > The proc_remove() (that is called from the snd_card_disconnect() > > helper) has a synchronization of the pending procfs accesses via > > wait_for_completion(). Meanwhile, ALSA procfs helper takes the global > > mutex_lock(&info_mutex) at both the proc_open callback and > > snd_card_info_disconnect() helper. Since the proc_open can't finish > > due to the mutex lock, wait_for_completion() never returns, either, > > hence it deadlocks. > > Really nice fix. Thanks Takashi. > > Reviewed-by: Jaroslav Kysela <perex@perex.cz> > > (maybe Cc: to stable?) Yes, I'll add it. thanks, Takashi
> As reported recently, ALSA core info helper may cause a deadlock at the forced device disconnection during the procfs operation. > > The proc_remove() (that is called from the snd_card_disconnect() > helper) has a synchronization of the pending procfs accesses via wait_for_completion(). Meanwhile, ALSA procfs helper takes the global > mutex_lock(&info_mutex) at both the proc_open callback and > snd_card_info_disconnect() helper. Since the proc_open can't finish due to the mutex lock, wait_for_completion() never returns, either, hence it deadlocks. As you said, the issue is easily reproduced after adding extra delay. And the problem is solved completely with your patch. Thanks for your help! Thanks. Shinhyung Kang.
diff --git a/sound/core/info.c b/sound/core/info.c index 0b2f04dcb589..e2f302e55bbb 100644 --- a/sound/core/info.c +++ b/sound/core/info.c @@ -56,7 +56,7 @@ struct snd_info_private_data { }; static int snd_info_version_init(void); -static void snd_info_disconnect(struct snd_info_entry *entry); +static void snd_info_clear_entries(struct snd_info_entry *entry); /* @@ -569,11 +569,16 @@ void snd_info_card_disconnect(struct snd_card *card) { if (!card) return; - mutex_lock(&info_mutex); + proc_remove(card->proc_root_link); - card->proc_root_link = NULL; if (card->proc_root) - snd_info_disconnect(card->proc_root); + proc_remove(card->proc_root->p); + + mutex_lock(&info_mutex); + if (card->proc_root) + snd_info_clear_entries(card->proc_root); + card->proc_root_link = NULL; + card->proc_root = NULL; mutex_unlock(&info_mutex); } @@ -745,15 +750,14 @@ struct snd_info_entry *snd_info_create_card_entry(struct snd_card *card, } EXPORT_SYMBOL(snd_info_create_card_entry); -static void snd_info_disconnect(struct snd_info_entry *entry) +static void snd_info_clear_entries(struct snd_info_entry *entry) { struct snd_info_entry *p; if (!entry->p) return; list_for_each_entry(p, &entry->children, list) - snd_info_disconnect(p); - proc_remove(entry->p); + snd_info_clear_entries(p); entry->p = NULL; } @@ -770,8 +774,9 @@ void snd_info_free_entry(struct snd_info_entry * entry) if (!entry) return; if (entry->p) { + proc_remove(entry->p); mutex_lock(&info_mutex); - snd_info_disconnect(entry); + snd_info_clear_entries(entry); mutex_unlock(&info_mutex); }
As reported recently, ALSA core info helper may cause a deadlock at the forced device disconnection during the procfs operation. The proc_remove() (that is called from the snd_card_disconnect() helper) has a synchronization of the pending procfs accesses via wait_for_completion(). Meanwhile, ALSA procfs helper takes the global mutex_lock(&info_mutex) at both the proc_open callback and snd_card_info_disconnect() helper. Since the proc_open can't finish due to the mutex lock, wait_for_completion() never returns, either, hence it deadlocks. TASK#1 TASK#2 proc_reg_open() takes use_pde() snd_info_text_entry_open() snd_card_disconnect() snd_info_card_disconnect() takes mutex_lock(&info_mutex) proc_remove() wait_for_completion(unused_pde) ... waiting task#1 closes mutex_lock(&info_mutex) => DEADLOCK This patch is a workaround for avoiding the deadlock scenario above. The basic strategy is to move proc_remove() call outside the mutex lock. proc_remove() can work gracefully without extra locking, and it can delete the tree recursively alone. So, we call proc_remove() at snd_info_card_disconnection() at first, then delete the rest resources recursively within the info_mutex lock. After the change, the function snd_info_disconnect() doesn't do disconnection by itself any longer, but it merely clears the procfs pointer. So rename the function to snd_info_clear_entries() for avoiding confusion. The similar change is applied to snd_info_free_entry(), too. Since the proc_remove() is called only conditionally with the non-NULL entry->p, it's skipped after the snd_info_clear_entries() call. Reported-by: Shinhyung Kang <s47.kang@samsung.com> Closes: https://lore.kernel.org/r/664457955.21699345385931.JavaMail.epsvc@epcpadp4 Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/core/info.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)