>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);
>
