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

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

[Dan Carpenter]

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

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

[Takashi Iwai]

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

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

[Amadeusz Sławiński]

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.

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

[Amadeusz Sławiński]

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>

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

[Takashi Iwai]

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