On Tue, 24 Jul 2012, Bjørn Mork wrote: > Bjørn Mork <bjorn@xxxxxxx> writes: > > Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes: > > > >> I suspect that answer is to make usb_wwan_disconnect a .port_remove > >> callback instead of a .disconnect callback. By the time the disconnect > >> method runs, the ports have been unregistered and are basically gone. > > > > I have a hard time trying to figure out the logic in usb_wwan. > > > > There is the usb_wwan_release which is called from destroy_serial, > > attempting to access and free driver specific serial_port_data. > > > > And there is the usb_wwan_disconnect which is called from the USB driver > > disconnect, and will attempt to kill a number of urbs stored in the > > serial_port_data. > > > > This seems a bit backwards, and AFAICS both of these must expect to be > > called after the port is gone. Should both be merged and converted to a > > .port_remove callback? That does seem sanest, given that they operate > > on port data. In general it's not that simple. Even though the port may be gone (i.e., the logical abstraction of a physical port has been destroyed), the port's data structure may still exist. The data structure won't be deallocated until all the references to it are dropped. Therefore it's reasonable for a driver to access a device's data structure after the device has been deregistered, provided the driver is very careful about not doing anything too extensive. For example, the data structure may now be under the control of a second driver, so the first driver should avoid doing anything that would interfere with the second driver's operation. On the other hand, if the necessary actions can be carried out in the port_remove callback, it will be better and simpler to do so. Since the usb-serial core does not invoke any callbacks when the port structures are deallocated in usb-serial.c:port_release(), in this case you don't have a choice anyway. In fact, I would say that the three loops in stop_read_write_urbs() and usb_wwan_release() could be combined into a single loop that should run in the port_remove callback. > One might have guessed this, but the commit which make these Oopses > reliable is > > 0998d0631 device-core: Ensure drvdata = NULL when no driver is bound Yes, that's what I expected. > I did a bisect between 3.5 and next-20120723, and it pointed out this > one without doubt. > > But I believe it only makes the underlying problem in usb_wwan obvious. > The use after free has been there forever, just using a pointer which > would happen to be valid most of the time. > > So I guess that commit really is a very nice fix, flushing out things > like this. usb_wwan certainly needs fixing. Agreed. And its users. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
