Re: [bug report] ALSA: jack: Access input_dev under mutex

[Takashi Iwai <tiwai@xxxxxxx>]

On Mon, 03 Jul 2023 16:18:27 +0200,
Dan Carpenter wrote:
> 
> Hello Amadeusz Sławiński,
> 
> The patch 1b6a6fc5280e: "ALSA: jack: Access input_dev under mutex"
> from Apr 12, 2022, leads to the following Smatch static checker
> warning:
> 
> 	sound/core/jack.c:673 snd_jack_report()
> 	warn: sleeping in atomic context
> 
> sound/core/jack.c
>     663         jack->hw_status_cache = status;
>     664 
>     665         list_for_each_entry(jack_kctl, &jack->kctl_list, list)
>     666                 if (jack_kctl->sw_inject_enable)
>     667                         mask_bits |= jack_kctl->mask_bits;
>     668                 else
>     669                         snd_kctl_jack_report(jack->card, jack_kctl->kctl,
>     670                                              status & jack_kctl->mask_bits);
>     671 
>     672 #ifdef CONFIG_SND_JACK_INPUT_DEV
> --> 673         mutex_lock(&jack->input_dev_lock);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> That patch adds this mutex but we can't take mutex because we're already
> holding a spinlock.  The problematic call trees are:
> 
> virtsnd_event_notify_cb() <- disables preempt
> virtsnd_disable_event_vq() <- disables preempt
> -> virtsnd_event_dispatch()
>    -> virtsnd_jack_event()
>       -> snd_jack_report()
> 
> The virtsnd_event_notify_cb() and virtsnd_disable_event_vq() functions
> take the spin_lock_irqsave(&queue->lock, flags);

Indeed it was no good choice to use the mutex there inside the report
function.  It's supposed to be callable from an irq-disabled context,
too.

How about simply using the device refcount like below?

Although we may drop the mutex from snd_jack, it can can be left, as
it's still useful for protecting a potential race between creation and
deletion.


thanks,

Takashi

-- 8< --
--- a/sound/core/jack.c
+++ b/sound/core/jack.c
@@ -654,6 +654,7 @@ void snd_jack_report(struct snd_jack *jack, int status)
 	struct snd_jack_kctl *jack_kctl;
 	unsigned int mask_bits = 0;
 #ifdef CONFIG_SND_JACK_INPUT_DEV
+	struct input_dev *idev;
 	int i;
 #endif
 
@@ -670,17 +671,15 @@ void snd_jack_report(struct snd_jack *jack, int status)
 					     status & jack_kctl->mask_bits);
 
 #ifdef CONFIG_SND_JACK_INPUT_DEV
-	mutex_lock(&jack->input_dev_lock);
-	if (!jack->input_dev) {
-		mutex_unlock(&jack->input_dev_lock);
+	idev = input_get_device(jack->input_dev);
+	if (!idev)
 		return;
-	}
 
 	for (i = 0; i < ARRAY_SIZE(jack->key); i++) {
 		int testbit = ((SND_JACK_BTN_0 >> i) & ~mask_bits);
 
 		if (jack->type & testbit)
-			input_report_key(jack->input_dev, jack->key[i],
+			input_report_key(idev, jack->key[i],
 					 status & testbit);
 	}
 
@@ -688,13 +687,13 @@ void snd_jack_report(struct snd_jack *jack, int status)
 		int testbit = ((1 << i) & ~mask_bits);
 
 		if (jack->type & testbit)
-			input_report_switch(jack->input_dev,
+			input_report_switch(idev,
 					    jack_switch_types[i],
 					    status & testbit);
 	}
 
-	input_sync(jack->input_dev);
-	mutex_unlock(&jack->input_dev_lock);
+	input_sync(idev);
+	input_put_device(idev);
 #endif /* CONFIG_SND_JACK_INPUT_DEV */
 }
 EXPORT_SYMBOL(snd_jack_report);