| Message ID | 20220401122931.4181700-1-amadeuszx.slawinski@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ALSA: jack: Access input_dev under mutex | expand |
On 2022-04-01 2:29 PM, Amadeusz Sławiński wrote: > It is possible when using ASoC that input_dev is unregistered while > calling snd_jack_report, which causes NULL pointer dereference. > In order to prevent this serialize access to input_dev using mutex lock. > > Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> Nitpick: "when using ASoC" is quite a generic statement. By that I guess you relate to concept of splitting audio functionality into smaller logical blocks - components (struct snd_soc_components) - and the possible synchronization issues that are part of that division. That alone is probably not a reason for re-send so: Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> > --- > include/sound/jack.h | 1 + > sound/core/jack.c | 37 ++++++++++++++++++++++++++++++------- > 2 files changed, 31 insertions(+), 7 deletions(-) > > diff --git a/include/sound/jack.h b/include/sound/jack.h > index 1181f536557e..1ed90e2109e9 100644 > --- a/include/sound/jack.h > +++ b/include/sound/jack.h > @@ -62,6 +62,7 @@ struct snd_jack { > const char *id; > #ifdef CONFIG_SND_JACK_INPUT_DEV > struct input_dev *input_dev; > + struct mutex input_dev_lock; > int registered; > int type; > char name[100]; > diff --git a/sound/core/jack.c b/sound/core/jack.c > index d1e3055f2b6a..58b9ebf5e7e1 100644 > --- a/sound/core/jack.c > +++ b/sound/core/jack.c > @@ -42,8 +42,11 @@ static int snd_jack_dev_disconnect(struct snd_device *device) > #ifdef CONFIG_SND_JACK_INPUT_DEV > struct snd_jack *jack = device->device_data; > > - if (!jack->input_dev) > + mutex_lock(&jack->input_dev_lock); > + if (!jack->input_dev) { > + mutex_unlock(&jack->input_dev_lock); > return 0; > + } > > /* If the input device is registered with the input subsystem > * then we need to use a different deallocator. */ > @@ -52,6 +55,7 @@ static int snd_jack_dev_disconnect(struct snd_device *device) > else > input_free_device(jack->input_dev); > jack->input_dev = NULL; > + mutex_unlock(&jack->input_dev_lock); > #endif /* CONFIG_SND_JACK_INPUT_DEV */ > return 0; > } > @@ -90,8 +94,11 @@ static int snd_jack_dev_register(struct snd_device *device) > snprintf(jack->name, sizeof(jack->name), "%s %s", > card->shortname, jack->id); > > - if (!jack->input_dev) > + mutex_lock(&jack->input_dev_lock); > + if (!jack->input_dev) { > + mutex_unlock(&jack->input_dev_lock); > return 0; > + } > > jack->input_dev->name = jack->name; > > @@ -116,6 +123,7 @@ static int snd_jack_dev_register(struct snd_device *device) > if (err == 0) > jack->registered = 1; > > + mutex_unlock(&jack->input_dev_lock); > return err; > } > #endif /* CONFIG_SND_JACK_INPUT_DEV */ > @@ -517,11 +525,15 @@ int snd_jack_new(struct snd_card *card, const char *id, int type, > return -ENOMEM; > } > > - /* don't creat input device for phantom jack */ > - if (!phantom_jack) { > #ifdef CONFIG_SND_JACK_INPUT_DEV > + mutex_init(&jack->input_dev_lock); > + > + /* don't create input device for phantom jack */ > + if (!phantom_jack) { > int i; > > + mutex_lock(&jack->input_dev_lock); > + > jack->input_dev = input_allocate_device(); > if (jack->input_dev == NULL) { > err = -ENOMEM; > @@ -537,8 +549,9 @@ int snd_jack_new(struct snd_card *card, const char *id, int type, > input_set_capability(jack->input_dev, EV_SW, > jack_switch_types[i]); > > -#endif /* CONFIG_SND_JACK_INPUT_DEV */ > + mutex_unlock(&jack->input_dev_lock); > } > +#endif /* CONFIG_SND_JACK_INPUT_DEV */ > > err = snd_device_new(card, SNDRV_DEV_JACK, jack, &ops); > if (err < 0) > @@ -556,7 +569,9 @@ int snd_jack_new(struct snd_card *card, const char *id, int type, > > fail_input: > #ifdef CONFIG_SND_JACK_INPUT_DEV > + mutex_lock(&jack->input_dev_lock); > input_free_device(jack->input_dev); > + mutex_unlock(&jack->input_dev_lock); > #endif > kfree(jack->id); > kfree(jack); > @@ -578,10 +593,14 @@ EXPORT_SYMBOL(snd_jack_new); > void snd_jack_set_parent(struct snd_jack *jack, struct device *parent) > { > WARN_ON(jack->registered); > - if (!jack->input_dev) > + mutex_lock(&jack->input_dev_lock); > + if (!jack->input_dev) { > + mutex_unlock(&jack->input_dev_lock); > return; > + } > > jack->input_dev->dev.parent = parent; > + mutex_unlock(&jack->input_dev_lock); > } > EXPORT_SYMBOL(snd_jack_set_parent); > > @@ -654,8 +673,11 @@ void snd_jack_report(struct snd_jack *jack, int status) > status & jack_kctl->mask_bits); > > #ifdef CONFIG_SND_JACK_INPUT_DEV > - if (!jack->input_dev) > + mutex_lock(&jack->input_dev_lock); > + if (!jack->input_dev) { > + mutex_unlock(&jack->input_dev_lock); > return; > + } > > for (i = 0; i < ARRAY_SIZE(jack->key); i++) { > int testbit = ((SND_JACK_BTN_0 >> i) & ~mask_bits); > @@ -675,6 +697,7 @@ void snd_jack_report(struct snd_jack *jack, int status) > } > > input_sync(jack->input_dev); > + mutex_unlock(&jack->input_dev_lock); > #endif /* CONFIG_SND_JACK_INPUT_DEV */ > } > EXPORT_SYMBOL(snd_jack_report);
On Fri, 01 Apr 2022 14:29:31 +0200, Amadeusz Sławiński wrote: > > @@ -517,11 +525,15 @@ int snd_jack_new(struct snd_card *card, const char *id, int type, > return -ENOMEM; > } > > - /* don't creat input device for phantom jack */ > - if (!phantom_jack) { > #ifdef CONFIG_SND_JACK_INPUT_DEV > + mutex_init(&jack->input_dev_lock); > + > + /* don't create input device for phantom jack */ > + if (!phantom_jack) { > int i; > > + mutex_lock(&jack->input_dev_lock); > + This lock is superfluous. The object is being created here and isn't registered anywhere, so no one else can use it yet. And moreover .... > jack->input_dev = input_allocate_device(); > if (jack->input_dev == NULL) { > err = -ENOMEM; ... you forgot unlock here, and this error path will lead to a deadlock. > @@ -537,8 +549,9 @@ int snd_jack_new(struct snd_card *card, const char *id, int type, > input_set_capability(jack->input_dev, EV_SW, > jack_switch_types[i]); > > -#endif /* CONFIG_SND_JACK_INPUT_DEV */ > + mutex_unlock(&jack->input_dev_lock); > } > +#endif /* CONFIG_SND_JACK_INPUT_DEV */ > > err = snd_device_new(card, SNDRV_DEV_JACK, jack, &ops); > if (err < 0) > @@ -556,7 +569,9 @@ int snd_jack_new(struct snd_card *card, const char *id, int type, > > fail_input: > #ifdef CONFIG_SND_JACK_INPUT_DEV > + mutex_lock(&jack->input_dev_lock); > input_free_device(jack->input_dev); > + mutex_unlock(&jack->input_dev_lock); Ditto, no need for mutex lock yet. One thing I'm not sure is whether it's good to use mutex. Basically snd_jack_report() is considered to be callable from non-atomic context, too, so far, and we don't need to prohibit it. OTOH, all current calls are in the sleepable context, so it's likely no big problem. But if we use mutex, it should be documented in snd_jack_report() function, too. thanks, Takashi
diff --git a/include/sound/jack.h b/include/sound/jack.h index 1181f536557e..1ed90e2109e9 100644 --- a/include/sound/jack.h +++ b/include/sound/jack.h @@ -62,6 +62,7 @@ struct snd_jack { const char *id; #ifdef CONFIG_SND_JACK_INPUT_DEV struct input_dev *input_dev; + struct mutex input_dev_lock; int registered; int type; char name[100]; diff --git a/sound/core/jack.c b/sound/core/jack.c index d1e3055f2b6a..58b9ebf5e7e1 100644 --- a/sound/core/jack.c +++ b/sound/core/jack.c @@ -42,8 +42,11 @@ static int snd_jack_dev_disconnect(struct snd_device *device) #ifdef CONFIG_SND_JACK_INPUT_DEV struct snd_jack *jack = device->device_data; - if (!jack->input_dev) + mutex_lock(&jack->input_dev_lock); + if (!jack->input_dev) { + mutex_unlock(&jack->input_dev_lock); return 0; + } /* If the input device is registered with the input subsystem * then we need to use a different deallocator. */ @@ -52,6 +55,7 @@ static int snd_jack_dev_disconnect(struct snd_device *device) else input_free_device(jack->input_dev); jack->input_dev = NULL; + mutex_unlock(&jack->input_dev_lock); #endif /* CONFIG_SND_JACK_INPUT_DEV */ return 0; } @@ -90,8 +94,11 @@ static int snd_jack_dev_register(struct snd_device *device) snprintf(jack->name, sizeof(jack->name), "%s %s", card->shortname, jack->id); - if (!jack->input_dev) + mutex_lock(&jack->input_dev_lock); + if (!jack->input_dev) { + mutex_unlock(&jack->input_dev_lock); return 0; + } jack->input_dev->name = jack->name; @@ -116,6 +123,7 @@ static int snd_jack_dev_register(struct snd_device *device) if (err == 0) jack->registered = 1; + mutex_unlock(&jack->input_dev_lock); return err; } #endif /* CONFIG_SND_JACK_INPUT_DEV */ @@ -517,11 +525,15 @@ int snd_jack_new(struct snd_card *card, const char *id, int type, return -ENOMEM; } - /* don't creat input device for phantom jack */ - if (!phantom_jack) { #ifdef CONFIG_SND_JACK_INPUT_DEV + mutex_init(&jack->input_dev_lock); + + /* don't create input device for phantom jack */ + if (!phantom_jack) { int i; + mutex_lock(&jack->input_dev_lock); + jack->input_dev = input_allocate_device(); if (jack->input_dev == NULL) { err = -ENOMEM; @@ -537,8 +549,9 @@ int snd_jack_new(struct snd_card *card, const char *id, int type, input_set_capability(jack->input_dev, EV_SW, jack_switch_types[i]); -#endif /* CONFIG_SND_JACK_INPUT_DEV */ + mutex_unlock(&jack->input_dev_lock); } +#endif /* CONFIG_SND_JACK_INPUT_DEV */ err = snd_device_new(card, SNDRV_DEV_JACK, jack, &ops); if (err < 0) @@ -556,7 +569,9 @@ int snd_jack_new(struct snd_card *card, const char *id, int type, fail_input: #ifdef CONFIG_SND_JACK_INPUT_DEV + mutex_lock(&jack->input_dev_lock); input_free_device(jack->input_dev); + mutex_unlock(&jack->input_dev_lock); #endif kfree(jack->id); kfree(jack); @@ -578,10 +593,14 @@ EXPORT_SYMBOL(snd_jack_new); void snd_jack_set_parent(struct snd_jack *jack, struct device *parent) { WARN_ON(jack->registered); - if (!jack->input_dev) + mutex_lock(&jack->input_dev_lock); + if (!jack->input_dev) { + mutex_unlock(&jack->input_dev_lock); return; + } jack->input_dev->dev.parent = parent; + mutex_unlock(&jack->input_dev_lock); } EXPORT_SYMBOL(snd_jack_set_parent); @@ -654,8 +673,11 @@ void snd_jack_report(struct snd_jack *jack, int status) status & jack_kctl->mask_bits); #ifdef CONFIG_SND_JACK_INPUT_DEV - if (!jack->input_dev) + mutex_lock(&jack->input_dev_lock); + if (!jack->input_dev) { + mutex_unlock(&jack->input_dev_lock); return; + } for (i = 0; i < ARRAY_SIZE(jack->key); i++) { int testbit = ((SND_JACK_BTN_0 >> i) & ~mask_bits); @@ -675,6 +697,7 @@ void snd_jack_report(struct snd_jack *jack, int status) } input_sync(jack->input_dev); + mutex_unlock(&jack->input_dev_lock); #endif /* CONFIG_SND_JACK_INPUT_DEV */ } EXPORT_SYMBOL(snd_jack_report);
It is possible when using ASoC that input_dev is unregistered while calling snd_jack_report, which causes NULL pointer dereference. In order to prevent this serialize access to input_dev using mutex lock. Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> --- include/sound/jack.h | 1 + sound/core/jack.c | 37 ++++++++++++++++++++++++++++++------- 2 files changed, 31 insertions(+), 7 deletions(-)