> From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxx>
>
> 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
