Hi Oliver,
On Thu, May 06, 2021 at 10:27:21AM +0200, Oliver Neukum wrote:
> Am Mittwoch, den 05.05.2021, 22:57 +0100 schrieb Sean Young:
> > +static void uirt_disconnect(struct usb_interface *intf)
> > +{
> > + struct uirt *ir = usb_get_intfdata(intf);
> > +
> > + rc_unregister_device(ir->rc);
> > + usb_set_intfdata(intf, NULL);
> > + usb_kill_urb(ir->urb_out);
> > + usb_free_urb(ir->urb_out);
> > + usb_kill_urb(ir->urb_in);
> > + usb_free_urb(ir->urb_in);
> > + kfree(ir->in);
> > + kfree(ir->out);
> > + kfree(ir);
> > +}
>
> Hi,
>
> almost. Going through this again, it looks like you have a race
> condition here.
>
> CPU A CPU B
>
> usb_kill_urb(ir->urb_out);
> usb_free_urb(ir->urb_out);
>
> uirt_in_callback()
> uirt_response(struct uirt *uirt, u32 len)
> err = usb_submit_urb(uirt->urb_out, GFP_ATOMIC);
>
> BANG, you are using freeed memory
Nice catch, I had missed this. I'll send out a v3 with this fixed.
This needs fixing, for sure. Having said that, once we are in the disconnect
handler aren't all urbs/endpoints disabled in usb_disable_interface()? Does
this mean the usb_kill_urb() are superfluous?
Thanks,
Sean
