Re: [PATCH] ALSA: core: Replace mutex_lock with mutex_trylock

[Takashi Iwai <tiwai@xxxxxxx>]

On Tue, 07 Nov 2023 09:17:59 +0100,
강신형 wrote:
> 
> Task1 waits for mutex_lock, and task2 waits for pde to be unused.(deadlock)
> /*call trace*/
>     task1 Call trace:
>             __switch_to+0x174/0x338
>             schedule+0x7c/0xe8
>             schedule_preempt_disabled+0x24/0x40
>             mutex_lock+0x40/0xec
>             snd_info_text_entry_open+0x28/0x120
>             proc_reg_open+0xe4/0x248
>             do_dentry_open+0x2a4/0x4e0
> 
>     task2 Call trace:
>             schedule_timeout+0x44/0x1c8
>             wait_for_completion+0x18/0x24
>             proc_entry_rundown+0x60/0xf0
>             remove_proc_subtree+0x180/0x218
>             proc_remove+0x20/0x30
>             snd_info_disconnect+0x4c/0x68
>             snd_info_card_disconnect+0x3c/0x58
>             snd_card_disconnect+0x130/0x264
>             usb_audio_disconnect+0xc0/0x24c
> 
> /*the sequence*/
>     task1:
>             - proc_reg_open: set the use_pde
>     task2:
>             - usb_audio_disconnect: usb device disconnection occurs
>             - snd_info_card_disconnect: acquire the mutex_lock(&info_mutex)
>             - proc_entry_rundown: wait_for_completion(unuse_pde)
>     task1:
>             - wait for mutex_lock in snd_info_text_entry_open
> 
> To avoid it, a mutex without wating(mutex_trylock) shoud be used in
> snd_info_text_entry_open(task1).
> Then, when mutex_lock acquisition fails, an error is returned, and the pde
> becomes unused, and the mutex_lock held by task2 is released.
> 
> 
> Signed-off-by: Shinhyung Kang <s47.kang@xxxxxxxxxxx>

Thanks for the patch.  But this change may break the current working
behavior; e.g. when two proc reads are running concurrently, one would
be aborted unexpectedly.

IIUC, the problem is the call of proc_remove(), and this call itself
can be outside the global mutex.

Could you check whether the patch below works instead?  (Note that
it's only compile-tested.)  It makes the proc_remove() called at
first, then clearing the internal entries.  The function was renamed
accordingly for avoiding confusion, too.


Takashi

--- 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);
 	}