Date: Fri, 27 Mar 2009 10:59:45 +0900 From: Weongyo Jeong <weongyo.jeong@gmail.com> To: Hans Petter Selasky <hselasky@c2i.net> Cc: Sam Leffler <sam@freebsd.org>, freebsd-usb@freebsd.org, Andrew Thompson <thompsa@freebsd.org> Subject: Re: q: Memory modified after free in usb2 Message-ID: <20090327015945.GB19512@weongyo.cdnetworks.kr> In-Reply-To: <200903260902.04274.hselasky@c2i.net> References: <20090325091756.GA14916@weongyo.cdnetworks.kr> <200903251046.55586.hselasky@c2i.net> <20090326014938.GB14916@weongyo.cdnetworks.kr> <200903260902.04274.hselasky@c2i.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Mar 26, 2009 at 09:02:02AM +0100, Hans Petter Selasky wrote: > 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 Yes. > 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() It looks it's my mistake to lock before going into callout_drain(9). > 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. This is what I want! I'll apply it immediately. Thanks! :-) > 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 ??? Thank you for advice again. I'll try to do some more tests. It looks ordering is important when detaching and with looking other WLAN drivers it seems calling ieee80211_ifdetach(ic) should be moved before usb2_transfer_unsetup(9) including if_zyd.c, if_ural.c and if_ral.c regards, Weongyo Jeong
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090327015945.GB19512>