q: Memory modified after free in usb2

[Hans Petter Selasky]

On Thursday 26 March 2009, Weongyo Jeong wrote:
> On Wed, Mar 25, 2009 at 10:46:54AM +0100, Hans Petter Selasky wrote:

> > > To solve this problem I modified codes slightly like below:
> > >
> > >   usb2_transfer_unsetup(sc->sc_xfer, UATH_N_XFERS);
> > >   usb2_pause_mtx(NULL, 5 * hz);
> > >   ...
> > >   uath_free_rx_data_list(sc);
> > >   uath_free_tx_data_list(sc);
> > >   uath_free_cmd_list(sc, sc->sc_cmd, UATH_CMD_LIST_COUNT);
> > >
> > > After adding it I couldn't see `Memory modified after free' messages
> > > anymore.  My question is that I can't understand why adding
> > > usb2_pause_mtx() helps this symptom?
> >
> > Did you drain all the taskqueues before unsetup ?
>
> Yes.  All I used was two callouts that the driver currently doesn't use
> usb2_proc_create() and tried to drain its before calling
> usb2_transfer_unsetup() but it still encounters `Memory modified after
> free'.

Hi,

Is this the link to the complete code?

http://perforce.freebsd.org/fileViewer.cgi?FSPC=//depot/user/weongyo/wireless/src/sys/dev/usb/wlan/if_uath.c&REV=24

1) The manpage states: "This function MUST NOT be called while holding any 
locks ..."
              |        UATH_LOCK(sc);
              |        callout_drain(&sc->stat_ch);
              |        callout_drain(&sc->watchdog_ch);
505:     159831 21 |        UATH_UNLOCK(sc);


callout_stop() is not callout_drain() !

Same sematics with USB2. transfer_stop()/transfer_drain()

2) Instead of copying the data twice, use the .ext_buffer=1 flag to instruct 
USB to not allocate its own buffer in "struct usb2_config", see "umass.c" for 
example, and the  and 
                        usb2_set_frame_data(xfer,
                            urb->transfer_buffer, 0);

Before you start the hardware!

Actually you can save alot of copying if you can exploit the multi-frame 
feature of the USB-stack for BULK transfers! Then you have to set "frames > 
1" in usb2_config structure. For example you would then copy in the header to 
wMaxPacketSize bytes, and use the data pointer for the rest, given that the 
IP-packet is not that defragged.

3) There is a chicken egg problem at detach.

I suspect that "uath_bulk_tx_callback()" is called with the USB_ERR_CANCELLED 
error. And here you seem to access freed memory. I think you need to re-think 
how you get that last node freed. Maybe it should be done by the if_stop() 
and not at cancelled, because according to detach, you will do if_stop() 
first and then cancel USB and unless you drain, you are not certain that the 
callback is complete!

              |        default:
               |                data = STAILQ_FIRST(&sc->sc_datatx_active);
               |                if (data == NULL)
               |                        goto setup;
2605:     159735  3 |                if (data->ni != NULL) {
               |                        ieee80211_free_node(data->ni);
               |                        data->ni = NULL;
               |                        ifp->if_oerrors++;
               |                }
2610:     159733  1 |                if (xfer->error != USB_ERR_CANCELLED) {
               |                        xfer->flags.stall_pipe = 1;
               |                        goto setup;
               |                }

I recommend that you do the usb2_transfer_unsetup() first at detach like in 
if_rum.c and the other WLAN drivers. That will solve your problem, and maybe 
you have to fix the datatx_active queue, so that the last "data" is not stuck 
there for ever ???

--HPS