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

["Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>]

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.