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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 7/5/2023 4:47 PM, Amadeusz Sławiński wrote:
On 7/4/2023 10:07 AM, Takashi Iwai wrote:
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);


Looking at code it looks like it should also work. Will schedule test run tomorrow to see if it causes any problems.

I've run tests and see nothing worrying, so
Tested-by: Amadeusz Sławiński <amadeuszx.slawinski@xxxxxxxxxxxxxxx>



[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux