* [bug report] ALSA: jack: Access input_dev under mutex
@ 2023-07-03 14:18 Dan Carpenter
2023-07-04 8:07 ` Takashi Iwai
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2023-07-03 14:18 UTC (permalink / raw)
To: amadeuszx.slawinski; +Cc: alsa-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] ALSA: jack: Access input_dev under mutex
2023-07-03 14:18 [bug report] ALSA: jack: Access input_dev under mutex Dan Carpenter
@ 2023-07-04 8:07 ` Takashi Iwai
2023-07-05 14:47 ` Amadeusz Sławiński
0 siblings, 1 reply; 5+ messages in thread
From: Takashi Iwai @ 2023-07-04 8:07 UTC (permalink / raw)
To: Dan Carpenter; +Cc: amadeuszx.slawinski, alsa-devel
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);
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] ALSA: jack: Access input_dev under mutex
2023-07-04 8:07 ` Takashi Iwai
@ 2023-07-05 14:47 ` Amadeusz Sławiński
2023-07-06 15:16 ` Amadeusz Sławiński
0 siblings, 1 reply; 5+ messages in thread
From: Amadeusz Sławiński @ 2023-07-05 14:47 UTC (permalink / raw)
To: Takashi Iwai, Dan Carpenter; +Cc: alsa-devel
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] ALSA: jack: Access input_dev under mutex
2023-07-05 14:47 ` Amadeusz Sławiński
@ 2023-07-06 15:16 ` Amadeusz Sławiński
2023-07-06 15:53 ` Takashi Iwai
0 siblings, 1 reply; 5+ messages in thread
From: Amadeusz Sławiński @ 2023-07-06 15:16 UTC (permalink / raw)
To: Takashi Iwai, Dan Carpenter; +Cc: alsa-devel
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] ALSA: jack: Access input_dev under mutex
2023-07-06 15:16 ` Amadeusz Sławiński
@ 2023-07-06 15:53 ` Takashi Iwai
0 siblings, 0 replies; 5+ messages in thread
From: Takashi Iwai @ 2023-07-06 15:53 UTC (permalink / raw)
To: Amadeusz Sławiński; +Cc: Dan Carpenter, alsa-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-07-06 15:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-03 14:18 [bug report] ALSA: jack: Access input_dev under mutex Dan Carpenter
2023-07-04 8:07 ` Takashi Iwai
2023-07-05 14:47 ` Amadeusz Sławiński
2023-07-06 15:16 ` Amadeusz Sławiński
2023-07-06 15:53 ` Takashi Iwai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox