| Message ID | CO1PR17MB54190B898057616EEB3F9E51E10E2@CO1PR17MB5419.namprd17.prod.outlook.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: gadget: u_audio: Fix race condition use of controls after free during gadget unbind. | expand |
On Thu, Apr 18, 2024 at 04:35:07PM +0000, Chris Wulff wrote: > Hang on to the control IDs instead of pointers since those are correctly handled with locks. > Prevent use of the uac data structure after it has been freed. > Mark the endpoint as disabled sooner so that freed requests aren't used. Nit, please wrap your changelog text at 72 columns. running scripts/checkpatch.pl should show this. > > Signed-off-by: Chris Wulff <chris.wulff@biamp.com> What commit id does this fix? > --- > drivers/usb/gadget/function/u_audio.c | 31 ++++++++++++++++----------- > 1 file changed, 19 insertions(+), 12 deletions(-) > > diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c > index 4a42574b4a7f..bcae95472455 100644 > --- a/drivers/usb/gadget/function/u_audio.c > +++ b/drivers/usb/gadget/function/u_audio.c > @@ -57,13 +57,13 @@ struct uac_rtd_params { > > /* Volume/Mute controls and their state */ > int fu_id; /* Feature Unit ID */ > - struct snd_kcontrol *snd_kctl_volume; > - struct snd_kcontrol *snd_kctl_mute; > + struct snd_ctl_elem_id snd_kctl_volume_id; > + struct snd_ctl_elem_id snd_kctl_mute_id; > s16 volume_min, volume_max, volume_res; > s16 volume; > int mute; No tabs? Odd. Not your fault, just a meta-comment. > > - struct snd_kcontrol *snd_kctl_rate; /* read-only current rate */ > + struct snd_ctl_elem_id snd_kctl_rate_id; /* read-only current rate */ > int srate; /* selected samplerate */ > int active; /* playback/capture running */ > > @@ -447,6 +447,8 @@ static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep) > if (!prm->ep_enabled) > return; > > + prm->ep_enabled = false; > + > audio_dev = uac->audio_dev; > params = &audio_dev->params; > > @@ -464,8 +466,6 @@ static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep) > } > } > > - prm->ep_enabled = false; > - > if (usb_ep_disable(ep)) > dev_err(uac->card->dev, "%s:%d Error!\n", __func__, __LINE__); > } > @@ -494,14 +494,13 @@ static inline void free_ep_fback(struct uac_rtd_params *prm, struct usb_ep *ep) > static void set_active(struct uac_rtd_params *prm, bool active) > { > // notifying through the Rate ctrl > - struct snd_kcontrol *kctl = prm->snd_kctl_rate; > unsigned long flags; > > spin_lock_irqsave(&prm->lock, flags); > if (prm->active != active) { > prm->active = active; > snd_ctl_notify(prm->uac->card, SNDRV_CTL_EVENT_MASK_VALUE, > - &kctl->id); > + &prm->snd_kctl_rate_id); > } > spin_unlock_irqrestore(&prm->lock, flags); > } > @@ -792,6 +791,9 @@ int u_audio_set_volume(struct g_audio *audio_dev, int playback, s16 val) > unsigned long flags; > int change = 0; > > + if (!uac) > + return 0; > + > if (playback) > prm = &uac->p_prm; > else > @@ -807,7 +809,7 @@ int u_audio_set_volume(struct g_audio *audio_dev, int playback, s16 val) > > if (change) > snd_ctl_notify(uac->card, SNDRV_CTL_EVENT_MASK_VALUE, > - &prm->snd_kctl_volume->id); > + &prm->snd_kctl_volume_id); > > return 0; > } > @@ -840,6 +842,9 @@ int u_audio_set_mute(struct g_audio *audio_dev, int playback, int val) > int change = 0; > int mute; > > + if (!uac) > + return 0; How can this happen? Is this a separate fix? Or the same issue? thanks, greg k-h
> From: Greg KH <gregkh@linuxfoundation.org> > Sent: Tuesday, April 23, 2024 7:18 PM > > On Thu, Apr 18, 2024 at 04:35:07PM +0000, Chris Wulff wrote: > > Hang on to the control IDs instead of pointers since those are correctly handled with locks. > > Prevent use of the uac data structure after it has been freed. > > Mark the endpoint as disabled sooner so that freed requests aren't used. > > Nit, please wrap your changelog text at 72 columns. running > scripts/checkpatch.pl should show this. Next version will be wrapped correctly. > > > > Signed-off-by: Chris Wulff <chris.wulff@biamp.com> > > What commit id does this fix? Several (next version will have Fixes: and see comments below about separating fixes) Modification to stored controls: 8fe9a03f4331 ("usb: gadget: u_audio: Rate ctl notifies about current srate (0=stopped)") c565ad07ef35 ("usb: gadget: u_audio: Support multiple sampling rates") 02de698ca812 ("usb: gadget: u_audio: add bi-directional volume and mute support") ep_enabled: 068fdad20454f81 ("usb: gadget: u_audio: fix race condition on endpoint stop") > > @@ -447,6 +447,8 @@ static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep) > > if (!prm->ep_enabled) > > return; > > > > + prm->ep_enabled = false; > > + > > audio_dev = uac->audio_dev; > > params = &audio_dev->params; > > > > @@ -464,8 +466,6 @@ static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep) > > } > > } > > > > - prm->ep_enabled = false; > > - > > if (usb_ep_disable(ep)) > > dev_err(uac->card->dev, "%s:%d Error!\n", __func__, __LINE__); > > } Looks like this actually is reverting part of 068fdad20454f81, which was put in to fix a different double-free problem but introduces a new race condition that I am running into. In my case, u_audio_iso_complete is called during unbind and _doesn't_ exit early and goes forward to access various things in the sound subsystem. I will split this off and see if I can better isolate what is really going wrong. > > @@ -792,6 +791,9 @@ int u_audio_set_volume(struct g_audio *audio_dev, int playback, s16 val) > > unsigned long flags; > > int change = 0; > > > > + if (!uac) > > + return 0; > > + > > if (playback) > > prm = &uac->p_prm; > > else > > @@ -840,6 +842,9 @@ int u_audio_set_mute(struct g_audio *audio_dev, int playback, int val) > > int change = 0; > > int mute; > > > > + if (!uac) > > + return 0; > > How can this happen? Is this a separate fix? Or the same issue? This happens if we receive a URB callback after/during g_audio_cleanup (via out_rq_cur_complete in f_uac1/2.c) for a UAC mute or volume control. If that happens, these can access freed memory. I think the higher-level race is that the dequeue for the request happens in composite_dev_cleanup after the function unbind (which in f_uac1/2 calls g_audio_cleanup.) I will make this a separate patch if you want as it is fixing a similar symptom as the others, but has it's own discrete cause. Presumably this can also happen for get of volume or mute controls though I didn't run into that. Here's a little timeline to better illustrate the race: Unbind URB composite_unbind __composite_unbind remove_config usb_remove_function f_audio_unbind g_audio_cleanup <-- uac is freed <-- URB received from host out_rq_cur_complete <-- set ctrl from host u_audio_set_mute <-- uses freed uac usb_remove_function... <-- other function in config may increase the window composite_dev_cleanup usb_ep_dequeue <-- EP0 req removed It is possible there is a better way to avoid this by making sure to dequeue the req prior to calling g_audio_cleanup in f_uac1/2.c. I will investigate that a bit and see what I can come up with. -- Chris Wulff
diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c index 4a42574b4a7f..bcae95472455 100644 --- a/drivers/usb/gadget/function/u_audio.c +++ b/drivers/usb/gadget/function/u_audio.c @@ -57,13 +57,13 @@ struct uac_rtd_params { /* Volume/Mute controls and their state */ int fu_id; /* Feature Unit ID */ - struct snd_kcontrol *snd_kctl_volume; - struct snd_kcontrol *snd_kctl_mute; + struct snd_ctl_elem_id snd_kctl_volume_id; + struct snd_ctl_elem_id snd_kctl_mute_id; s16 volume_min, volume_max, volume_res; s16 volume; int mute; - struct snd_kcontrol *snd_kctl_rate; /* read-only current rate */ + struct snd_ctl_elem_id snd_kctl_rate_id; /* read-only current rate */ int srate; /* selected samplerate */ int active; /* playback/capture running */ @@ -447,6 +447,8 @@ static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep) if (!prm->ep_enabled) return; + prm->ep_enabled = false; + audio_dev = uac->audio_dev; params = &audio_dev->params; @@ -464,8 +466,6 @@ static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep) } } - prm->ep_enabled = false; - if (usb_ep_disable(ep)) dev_err(uac->card->dev, "%s:%d Error!\n", __func__, __LINE__); } @@ -494,14 +494,13 @@ static inline void free_ep_fback(struct uac_rtd_params *prm, struct usb_ep *ep) static void set_active(struct uac_rtd_params *prm, bool active) { // notifying through the Rate ctrl - struct snd_kcontrol *kctl = prm->snd_kctl_rate; unsigned long flags; spin_lock_irqsave(&prm->lock, flags); if (prm->active != active) { prm->active = active; snd_ctl_notify(prm->uac->card, SNDRV_CTL_EVENT_MASK_VALUE, - &kctl->id); + &prm->snd_kctl_rate_id); } spin_unlock_irqrestore(&prm->lock, flags); } @@ -792,6 +791,9 @@ int u_audio_set_volume(struct g_audio *audio_dev, int playback, s16 val) unsigned long flags; int change = 0; + if (!uac) + return 0; + if (playback) prm = &uac->p_prm; else @@ -807,7 +809,7 @@ int u_audio_set_volume(struct g_audio *audio_dev, int playback, s16 val) if (change) snd_ctl_notify(uac->card, SNDRV_CTL_EVENT_MASK_VALUE, - &prm->snd_kctl_volume->id); + &prm->snd_kctl_volume_id); return 0; } @@ -840,6 +842,9 @@ int u_audio_set_mute(struct g_audio *audio_dev, int playback, int val) int change = 0; int mute; + if (!uac) + return 0; + if (playback) prm = &uac->p_prm; else @@ -856,7 +861,7 @@ int u_audio_set_mute(struct g_audio *audio_dev, int playback, int val) if (change) snd_ctl_notify(uac->card, SNDRV_CTL_EVENT_MASK_VALUE, - &prm->snd_kctl_mute->id); + &prm->snd_kctl_mute_id); return 0; } @@ -1331,7 +1336,7 @@ int g_audio_setup(struct g_audio *g_audio, const char *pcm_name, err = snd_ctl_add(card, kctl); if (err < 0) goto snd_fail; - prm->snd_kctl_mute = kctl; + prm->snd_kctl_mute_id = kctl->id; prm->mute = 0; } @@ -1359,7 +1364,7 @@ int g_audio_setup(struct g_audio *g_audio, const char *pcm_name, err = snd_ctl_add(card, kctl); if (err < 0) goto snd_fail; - prm->snd_kctl_volume = kctl; + prm->snd_kctl_volume_id = kctl->id; prm->volume = fu->volume_max; prm->volume_max = fu->volume_max; prm->volume_min = fu->volume_min; @@ -1383,7 +1388,7 @@ int g_audio_setup(struct g_audio *g_audio, const char *pcm_name, err = snd_ctl_add(card, kctl); if (err < 0) goto snd_fail; - prm->snd_kctl_rate = kctl; + prm->snd_kctl_rate_id = kctl->id; } strscpy(card->driver, card_name, sizeof(card->driver)); @@ -1420,6 +1425,8 @@ void g_audio_cleanup(struct g_audio *g_audio) return; uac = g_audio->uac; + g_audio->uac = NULL; + card = uac->card; if (card) snd_card_free_when_closed(card);
Hang on to the control IDs instead of pointers since those are correctly handled with locks. Prevent use of the uac data structure after it has been freed. Mark the endpoint as disabled sooner so that freed requests aren't used. Signed-off-by: Chris Wulff <chris.wulff@biamp.com> --- drivers/usb/gadget/function/u_audio.c | 31 ++++++++++++++++----------- 1 file changed, 19 insertions(+), 12 deletions(-)