Re: [PATCH v2 2/3] media: rc: new driver for USB-UIRT device

[Sean Young <sean@mess.org>]

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