>On Tue, 7 Jun 2016, Chung-Geol Kim wrote: > >> ================================================= >> At *remove USB(3.0) Storage >> sequence <1> --> <5> ((Problem Case)) >> ================================================= >> VOLD >> ------------------------------------|------------ >> (uevent) >> ________|_________ >> |<1> | >> |dwc3_otg_sm_work | >> |usb_put_hcd | >> |shared_hcd(kref=2)| >> |__________________| >> ________|_________ >> |<2> | >> |New USB BUS #2 | >> | | >> |shared_hcd(kref=1)| >> | | >> --(Link)-bandXX_mutex| >> | |__________________| >> | >> ___________________ | >> |<3> | | >> |dwc3_otg_sm_work | | >> |usb_put_hcd | | >> |primary_hcd(kref=1)| | >> |___________________| | >> _________|_________ | >> |<4> | | >> |New USB BUS #1 | | >> |hcd_release | | >> |primary_hcd(kref=0)| | >> | | | >> |bandXX_mutex(free) |<- >> |___________________| >> (( VOLD )) >> ______|___________ >> |<5> | >> | SCSI | >> |usb_put_hcd | >> |shared_hcd(kref=0)| >> |*hcd_release | >> |bandXX_mutex(free*)|<- double free >> |__________________| >> >> ================================================= > >Okay, now I understand the problem you want to solve. What we need to >do is make sure the mutex is deallocated when the _last_ hcd is >released, which is not necessarily the same as when the _primary_ hcd >is released. > >Can you please test the patch below? > >By the way, a good change (if you want to do it) would be to rename the >"shared_hcd" field to "other_hcd" or "peer_hcd". This is because it >always points to the other hcd in the peer set: In the primary >structure it points to the secondary, and in the secondary structure it >points to the primary. > >Alan Stern > Thank you for clear understanding the problem that I faced. When I tested with your below patch, it also works well. The description has been modified as follows as you suggested. ================================================= At *remove USB(3.0) Storage sequence <1> --> <5> ((Problem Case)) ================================================= VOLD ------------------------------------|------------ (uevent) ________|_________ |<1> | |dwc3_otg_sm_work | |usb_put_hcd | |peer_hcd(kref=2)| |__________________| ________|_________ |<2> | |New USB BUS #2 | | | |peer_hcd(kref=1)| | | --(Link)-bandXX_mutex| | |__________________| | ___________________ | |<3> | | |dwc3_otg_sm_work | | |usb_put_hcd | | |primary_hcd(kref=1)| | |___________________| | _________|_________ | |<4> | | |New USB BUS #1 | | |hcd_release | | |primary_hcd(kref=0)| | | | | |bandXX_mutex(free) |<- |___________________| (( VOLD )) ______|___________ |<5> | | SCSI | |usb_put_hcd | |peer_hcd(kref=0)| |*hcd_release | |bandXX_mutex(free*)|<- double free |__________________| ================================================= > > >Index: usb-4.x/drivers/usb/core/hcd.c >=================================================================== >--- usb-4.x.orig/drivers/usb/core/hcd.c >+++ usb-4.x/drivers/usb/core/hcd.c >@@ -2588,24 +2588,22 @@ EXPORT_SYMBOL_GPL(usb_create_hcd); > * Don't deallocate the bandwidth_mutex until the last shared usb_hcd is > * deallocated. > * >- * Make sure to only deallocate the bandwidth_mutex when the primary HCD is >- * freed. When hcd_release() is called for either hcd in a peer set >- * invalidate the peer's ->shared_hcd and ->primary_hcd pointers to >- * block new peering attempts >+ * Make sure to deallocate the bandwidth_mutex only when the last HCD is >+ * freed. When hcd_release() is called for either hcd in a peer set, >+ * invalidate the peer's ->shared_hcd and ->primary_hcd pointers. > */ > static void hcd_release(struct kref *kref) > { > struct usb_hcd *hcd = container_of (kref, struct usb_hcd, kref); > > mutex_lock(&usb_port_peer_mutex); >- if (usb_hcd_is_primary_hcd(hcd)) >- kfree(hcd->bandwidth_mutex); > if (hcd->shared_hcd) { > struct usb_hcd *peer = hcd->shared_hcd; > > peer->shared_hcd = NULL; >- if (peer->primary_hcd == hcd) >- peer->primary_hcd = NULL; >+ peer->primary_hcd = NULL; >+ } else { >+ kfree(hcd->bandwidth_mutex); > } > mutex_unlock(&usb_port_peer_mutex); > kfree(hcd); >