[bug report] ALSA: jack: Access input_dev under mutex
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);
regards, dan carpenter
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);
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.
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@linux.intel.com
On Thu, 06 Jul 2023 17:16:38 +0200, Amadeusz Sławiński wrote:
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@linux.intel.com
Good to hear, I'll submit the proper patch.
thanks,
Takashi
participants (3)
-
Amadeusz Sławiński -
Dan Carpenter -
Takashi Iwai