Re: Re: Re: Re: Re: [PATCH] usb: core: fix a double free in the usb driver

2016-06-08 Thread Chung-Geol Kim 
>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

Re: Re: Re: Re: Re: [PATCH] usb: core: fix a double free in the usb driver

2016-06-08 Thread Chung-Geol Kim 
>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

RE: Re: Re: Re: [PATCH] usb: core: fix a double free in the usb driver

2016-06-07 Thread Alan Stern 
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



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(_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(_port_peer_mutex);
kfree(hcd);

RE: Re: Re: Re: [PATCH] usb: core: fix a double free in the usb driver

2016-06-07 Thread Alan Stern 
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



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(_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(_port_peer_mutex);
kfree(hcd);

RE: Re: Re: Re: [PATCH] usb: core: fix a double free in the usb driver

2016-06-06 Thread Chung-Geol Kim 
>On Fri, 3 Jun 2016, Chung-Geol Kim wrote:
>
>> Yes, you are right, The presentational errors in order to obtain an 
>> understanding of the process.
>> Therefore, I will be happy to explain again the diagrammatic representation 
>> as shown below.
>> If using usb 3.0 storage(OTG), you can see as below.
>> 
>> ==
>> At *Insert USB(3.0) Storage
>> sequence <1> --> <5>
>> ==
>> VOLD   
>> =|
>>  (uevent)
>>__|___ 
>>   |<5>   |
>>   |  SCSI|
>>   |usb_get_hcd   |
>>   |shared_hcd(kref=3)|
>>   |__|
>>___ |_ 
>>   |<2>|   |<4>   |
>>   |dwc3_otg_sm_work   |   |dwc3_otg_sm_work  |
>>   |usb_get_hcd|   |usb_get_hcd   |
>>   |primary_hcd(kref=2)|   |shared_hcd(kref=2)|
>>   |___|   |__|
>>_|_ |_ 
>>   |<1>|   |<3>   |
>>   |New USB BUS #1 |   |New USB BUS #2|
>>   |usb_create_hcd |   |usb_create_hcd|
>>   |primary_hcd(kref=1)|   |shared_hcd(kref=1)|
>>   |   |   |  |
>>   |bandXX_mutex(alloc)|<-(Link)-bandXX_mutex |
>>   |___|   |__|
>> 
>
>When people present diagrams like this, the universal convention is to 
>show earlier times at the top and later times at the bottom.  That way 
>the order in which you read the diagram is the same as the order in 
>which the events are supposed to occur.
>
>Also, the convention is to put events next to each other if they happen 
>at the same time.  In your diagram, <1> and <3> are next to each other 
>but they don't happen at the same time.

Thank you for your kind explanation. It has been modified as shown below.
=
 At *Insert USB(3.0) Storage
 sequence <1> --> <5>
=
   ___
  |<1>|
  |New USB BUS #1 |
  |usb_create_hcd |
  |primary_hcd(kref=1)|
  |   |
  |bandXX_mutex(alloc)|<--
  |___|  |
   _|_   |
  |<2>|  |
  |dwc3_otg_sm_work   |  |
  |usb_get_hcd|  |
  |primary_hcd(kref=2)|  |
  |___|  |
 |  __
 | |<3>   |
 | |New USB BUS #2|
 | |usb_create_hcd|
 | |shared_hcd(kref=1)|
 | |  |
 --(Link)-bandXX_mutex|
   |__|
|_ 
   |<4>   |
   |dwc3_otg_sm_work  |
   |usb_get_hcd   |
   |shared_hcd(kref=2)|
   |__|
___|__ 
   |<5>   |
   |  SCSI|
   |usb_get_hcd   |
   |shared_hcd(kref=3)|
   |__|
|
(uevent)
---|
 VOLD
=


=
 At *remove USB(3.0) Storage
 sequence <1> --> <5> ((Normal Case))
=
   VOLD
|
(uevent)
  __|___
 |<1>   |
 |  SCSI|
 |usb_put_hcd   |
 |shared_hcd(kref=2)|
 |__|
  |_ 
 |<2>   |
 |dwc3_otg_sm_work  |
 |usb_put_hcd   |
 |shared_hcd(kref=1)|
 |__|
  |_ 
 |<3>   |
 |New USB BUS #2|
 |hcd_release   |
 |shared_hcd(kref=0)|

RE: Re: Re: Re: [PATCH] usb: core: fix a double free in the usb driver

2016-06-06 Thread Chung-Geol Kim 
>On Fri, 3 Jun 2016, Chung-Geol Kim wrote:
>
>> Yes, you are right, The presentational errors in order to obtain an 
>> understanding of the process.
>> Therefore, I will be happy to explain again the diagrammatic representation 
>> as shown below.
>> If using usb 3.0 storage(OTG), you can see as below.
>> 
>> ==
>> At *Insert USB(3.0) Storage
>> sequence <1> --> <5>
>> ==
>> VOLD   
>> =|
>>  (uevent)
>>__|___ 
>>   |<5>   |
>>   |  SCSI|
>>   |usb_get_hcd   |
>>   |shared_hcd(kref=3)|
>>   |__|
>>___ |_ 
>>   |<2>|   |<4>   |
>>   |dwc3_otg_sm_work   |   |dwc3_otg_sm_work  |
>>   |usb_get_hcd|   |usb_get_hcd   |
>>   |primary_hcd(kref=2)|   |shared_hcd(kref=2)|
>>   |___|   |__|
>>_|_ |_ 
>>   |<1>|   |<3>   |
>>   |New USB BUS #1 |   |New USB BUS #2|
>>   |usb_create_hcd |   |usb_create_hcd|
>>   |primary_hcd(kref=1)|   |shared_hcd(kref=1)|
>>   |   |   |  |
>>   |bandXX_mutex(alloc)|<-(Link)-bandXX_mutex |
>>   |___|   |__|
>> 
>
>When people present diagrams like this, the universal convention is to 
>show earlier times at the top and later times at the bottom.  That way 
>the order in which you read the diagram is the same as the order in 
>which the events are supposed to occur.
>
>Also, the convention is to put events next to each other if they happen 
>at the same time.  In your diagram, <1> and <3> are next to each other 
>but they don't happen at the same time.

Thank you for your kind explanation. It has been modified as shown below.
=
 At *Insert USB(3.0) Storage
 sequence <1> --> <5>
=
   ___
  |<1>|
  |New USB BUS #1 |
  |usb_create_hcd |
  |primary_hcd(kref=1)|
  |   |
  |bandXX_mutex(alloc)|<--
  |___|  |
   _|_   |
  |<2>|  |
  |dwc3_otg_sm_work   |  |
  |usb_get_hcd|  |
  |primary_hcd(kref=2)|  |
  |___|  |
 |  __
 | |<3>   |
 | |New USB BUS #2|
 | |usb_create_hcd|
 | |shared_hcd(kref=1)|
 | |  |
 --(Link)-bandXX_mutex|
   |__|
|_ 
   |<4>   |
   |dwc3_otg_sm_work  |
   |usb_get_hcd   |
   |shared_hcd(kref=2)|
   |__|
___|__ 
   |<5>   |
   |  SCSI|
   |usb_get_hcd   |
   |shared_hcd(kref=3)|
   |__|
|
(uevent)
---|
 VOLD
=


=
 At *remove USB(3.0) Storage
 sequence <1> --> <5> ((Normal Case))
=
   VOLD
|
(uevent)
  __|___
 |<1>   |
 |  SCSI|
 |usb_put_hcd   |
 |shared_hcd(kref=2)|
 |__|
  |_ 
 |<2>   |
 |dwc3_otg_sm_work  |
 |usb_put_hcd   |
 |shared_hcd(kref=1)|
 |__|
  |_ 
 |<3>   |
 |New USB BUS #2|
 |hcd_release   |
 |shared_hcd(kref=0)|

Re: Re: Re: [PATCH] usb: core: fix a double free in the usb driver

2016-06-03 Thread Alan Stern 
On Fri, 3 Jun 2016, Chung-Geol Kim wrote:

> Yes, you are right, The presentational errors in order to obtain an 
> understanding of the process.
> Therefore, I will be happy to explain again the diagrammatic representation 
> as shown below.
> If using usb 3.0 storage(OTG), you can see as below.
> 
> ==
> At *Insert USB(3.0) Storage
> sequence <1> --> <5>
> ==
> VOLD   
> =|
>  (uevent)
>__|___ 
>   |<5>   |
>   |  SCSI|
>   |usb_get_hcd   |
>   |shared_hcd(kref=3)|
>   |__|
>___ |_ 
>   |<2>|   |<4>   |
>   |dwc3_otg_sm_work   |   |dwc3_otg_sm_work  |
>   |usb_get_hcd|   |usb_get_hcd   |
>   |primary_hcd(kref=2)|   |shared_hcd(kref=2)|
>   |___|   |__|
>_|_ |_ 
>   |<1>|   |<3>   |
>   |New USB BUS #1 |   |New USB BUS #2|
>   |usb_create_hcd |   |usb_create_hcd|
>   |primary_hcd(kref=1)|   |shared_hcd(kref=1)|
>   |   |   |  |
>   |bandXX_mutex(alloc)|<-(Link)-bandXX_mutex |
>   |___|   |__|
> 

When people present diagrams like this, the universal convention is to 
show earlier times at the top and later times at the bottom.  That way 
the order in which you read the diagram is the same as the order in 
which the events are supposed to occur.

Also, the convention is to put events next to each other if they happen 
at the same time.  In your diagram, <1> and <3> are next to each other 
but they don't happen at the same time.

> ==
> At *remove USB(3.0) Storage 
> sequence <1> --> <5> ((Normal Case))
> ==
> VOLD   
> =|
>  (uevent)
>__|___ 
>   |<1>   |
>   |  SCSI|
>   |usb_put_hcd   |
>   |shared_hcd(kref=2)|
>   |__|
>___ |_ 
>   |<4>|   |<2>   |
>   |dwc3_otg_sm_work   |   |dwc3_otg_sm_work  |
>   |usb_put_hcd|   |usb_put_hcd   |
>   |primary_hcd(kref=1)|   |shared_hcd(kref=1)|
>   |___|   |__|
>_|_ |_ 
>   |<5>|   |<3>   |
>   |New USB BUS #1 |   |New USB BUS #2|
>   |hcd_release|   |hcd_release   |
>   |primary_hcd(kref=0)|   |shared_hcd(kref=0)|
>   |   |   |  |
>   |bandXX_mutex(free) | -X-cut off)-bandXX_mutex|
>   |___|   |__|
> 
> --

> >The real bug here is that the shared_hcd is released after the 
> >primary_hcd.  That's what you need to fix.
> 
> NO, It 's only depend on vold(scsi) release time.
> If the vold later released and is being released first hcd,
> Double free happened at <5> as below.
> 
> ==
> At *remove USB(3.0) Storage 
> sequence <1> --> <5> ((Problem Case))
> ==
> VOLD   
> =|
>  (uevent)
>__|___ 
>   |<5>   |
>   |  SCSI|
>   |usb_put_hcd   |
>   |shared_hcd(kref=0)|
>   |*hcd_release  |
>   |bandXX_mutex(free*)|<- double free
>   |__|
>___ |_ 
>   |<3>|   |<1>   |
>   |dwc3_otg_sm_work   |   |dwc3_otg_sm_work  |
>   |usb_put_hcd|   |usb_put_hcd   |
>   |primary_hcd(kref=1)|   |shared_hcd(kref=2)|
>   |___|   |__|
>_|_ |_ 
>   |<4>|   |<2>   |
>   |New USB BUS #1 |   |New USB BUS #2|
>   |hcd_release|   |  |
>   |primary_hcd(kref=0)|   |shared_hcd(kref=1)|
>   |   |   |  |
>   |bandXX_mutex(free) |<-(Link)-bandXX_mutex |
>

Re: Re: Re: [PATCH] usb: core: fix a double free in the usb driver

2016-06-03 Thread Alan Stern 
On Fri, 3 Jun 2016, Chung-Geol Kim wrote:

> Yes, you are right, The presentational errors in order to obtain an 
> understanding of the process.
> Therefore, I will be happy to explain again the diagrammatic representation 
> as shown below.
> If using usb 3.0 storage(OTG), you can see as below.
> 
> ==
> At *Insert USB(3.0) Storage
> sequence <1> --> <5>
> ==
> VOLD   
> =|
>  (uevent)
>__|___ 
>   |<5>   |
>   |  SCSI|
>   |usb_get_hcd   |
>   |shared_hcd(kref=3)|
>   |__|
>___ |_ 
>   |<2>|   |<4>   |
>   |dwc3_otg_sm_work   |   |dwc3_otg_sm_work  |
>   |usb_get_hcd|   |usb_get_hcd   |
>   |primary_hcd(kref=2)|   |shared_hcd(kref=2)|
>   |___|   |__|
>_|_ |_ 
>   |<1>|   |<3>   |
>   |New USB BUS #1 |   |New USB BUS #2|
>   |usb_create_hcd |   |usb_create_hcd|
>   |primary_hcd(kref=1)|   |shared_hcd(kref=1)|
>   |   |   |  |
>   |bandXX_mutex(alloc)|<-(Link)-bandXX_mutex |
>   |___|   |__|
> 

When people present diagrams like this, the universal convention is to 
show earlier times at the top and later times at the bottom.  That way 
the order in which you read the diagram is the same as the order in 
which the events are supposed to occur.

Also, the convention is to put events next to each other if they happen 
at the same time.  In your diagram, <1> and <3> are next to each other 
but they don't happen at the same time.

> ==
> At *remove USB(3.0) Storage 
> sequence <1> --> <5> ((Normal Case))
> ==
> VOLD   
> =|
>  (uevent)
>__|___ 
>   |<1>   |
>   |  SCSI|
>   |usb_put_hcd   |
>   |shared_hcd(kref=2)|
>   |__|
>___ |_ 
>   |<4>|   |<2>   |
>   |dwc3_otg_sm_work   |   |dwc3_otg_sm_work  |
>   |usb_put_hcd|   |usb_put_hcd   |
>   |primary_hcd(kref=1)|   |shared_hcd(kref=1)|
>   |___|   |__|
>_|_ |_ 
>   |<5>|   |<3>   |
>   |New USB BUS #1 |   |New USB BUS #2|
>   |hcd_release|   |hcd_release   |
>   |primary_hcd(kref=0)|   |shared_hcd(kref=0)|
>   |   |   |  |
>   |bandXX_mutex(free) | -X-cut off)-bandXX_mutex|
>   |___|   |__|
> 
> --

> >The real bug here is that the shared_hcd is released after the 
> >primary_hcd.  That's what you need to fix.
> 
> NO, It 's only depend on vold(scsi) release time.
> If the vold later released and is being released first hcd,
> Double free happened at <5> as below.
> 
> ==
> At *remove USB(3.0) Storage 
> sequence <1> --> <5> ((Problem Case))
> ==
> VOLD   
> =|
>  (uevent)
>__|___ 
>   |<5>   |
>   |  SCSI|
>   |usb_put_hcd   |
>   |shared_hcd(kref=0)|
>   |*hcd_release  |
>   |bandXX_mutex(free*)|<- double free
>   |__|
>___ |_ 
>   |<3>|   |<1>   |
>   |dwc3_otg_sm_work   |   |dwc3_otg_sm_work  |
>   |usb_put_hcd|   |usb_put_hcd   |
>   |primary_hcd(kref=1)|   |shared_hcd(kref=2)|
>   |___|   |__|
>_|_ |_ 
>   |<4>|   |<2>   |
>   |New USB BUS #1 |   |New USB BUS #2|
>   |hcd_release|   |  |
>   |primary_hcd(kref=0)|   |shared_hcd(kref=1)|
>   |   |   |  |
>   |bandXX_mutex(free) |<-(Link)-bandXX_mutex |
>

Re: Re: Re: [PATCH] usb: core: fix a double free in the usb driver

2016-06-03 Thread Chung-Geol Kim 
>On Fri, 27 May 2016, Chung-Geol Kim wrote:
>
>> >On Fri, May 27, 2016 at 01:38:17AM +, Chung-Geol Kim wrote:
>> >> There is a double free problem in the usb driver.
>> >
>> >Which driver?
>> When I using the USB OTG Storage, this issue happened.
>> When remove the OTG Storage, it reproduced sometimes.
>
>>  cpu 0   cpu 1
>>  
>> 
>>  (*Insert USB Storage)
>>  usb_create_shared_hcd()
>>  kmalloc(primary_hcd)
>>  kmalloc(primary_hcd->bandwidth_mutex)
>>   ->(primary_hcd->kref==1)
>>  usb_get_hcd()
>>   ->(primary_hcd->kref==2)
>>  usb_create_shared_hcd()
>>  kmalloc(hcd->shared_hcd)
>>   
>> ->hcd->shared_hcd->bandwidth_mutex=primary->bandwidth_mutex
>>   
>> ->primary_hcd->primary_hcd = primary_hcd
>>   
>> ->hcd->shared_hcd->primary_hcd = primary_hcd
>>  
>> ->(hcd->shared_hcd->kref==1)
>>  usb_get_hcd()
>>  
>> ->(hcd->shared_hcd->kref==2)
>> 
>>  usb_get_hcd()
>>   
>> ->(hcd->shared_hcd->kref==3)
>
>I don't understand.  Why do these actions take place on two different
>CPUs?  Aren't the primary_hcd and the shared_hcd structures allocated
>by the same thread, on the same CPU?

Yes, you are right, The presentational errors in order to obtain an 
understanding of the process.
Therefore, I will be happy to explain again the diagrammatic representation as 
shown below.
If using usb 3.0 storage(OTG), you can see as below.

==
At *Insert USB(3.0) Storage
sequence <1> --> <5>
==
VOLD   
=|
 (uevent)
   __|___ 
  |<5>   |
  |  SCSI|
  |usb_get_hcd   |
  |shared_hcd(kref=3)|
  |__|
   ___ |_ 
  |<2>|   |<4>   |
  |dwc3_otg_sm_work   |   |dwc3_otg_sm_work  |
  |usb_get_hcd|   |usb_get_hcd   |
  |primary_hcd(kref=2)|   |shared_hcd(kref=2)|
  |___|   |__|
   _|_ |_ 
  |<1>|   |<3>   |
  |New USB BUS #1 |   |New USB BUS #2|
  |usb_create_hcd |   |usb_create_hcd|
  |primary_hcd(kref=1)|   |shared_hcd(kref=1)|
  |   |   |  |
  |bandXX_mutex(alloc)|<-(Link)-bandXX_mutex |
  |___|   |__|

==
At *remove USB(3.0) Storage 
sequence <1> --> <5> ((Normal Case))
==
VOLD   
=|
 (uevent)
   __|___ 
  |<1>   |
  |  SCSI|
  |usb_put_hcd   |
  |shared_hcd(kref=2)|
  |__|
   ___ |_ 
  |<4>|   |<2>   |
  |dwc3_otg_sm_work   |   |dwc3_otg_sm_work  |
  |usb_put_hcd|   |usb_put_hcd   |
  |primary_hcd(kref=1)|   |shared_hcd(kref=1)|
  |___|   |__|
   _|_ |_ 
  |<5>|   |<3>   |
  |New USB BUS #1 |   |New USB BUS #2|
  |hcd_release|   |hcd_release   |
  |primary_hcd(kref=0)|   |shared_hcd(kref=0)|
  |   |   |  |
  |bandXX_mutex(free) | -X-cut off)-bandXX_mutex|
  |___|   |__|

--
>
>> 
>> ---
>>  (*remove USB Storage)   
>>  usb_release_dev()
>>   
>> ->(hcd->shared_hcd-kref==2)
>>  usb_release_dev()
>>   
>>

Re: Re: Re: [PATCH] usb: core: fix a double free in the usb driver

2016-06-03 Thread Chung-Geol Kim 
>On Fri, 27 May 2016, Chung-Geol Kim wrote:
>
>> >On Fri, May 27, 2016 at 01:38:17AM +, Chung-Geol Kim wrote:
>> >> There is a double free problem in the usb driver.
>> >
>> >Which driver?
>> When I using the USB OTG Storage, this issue happened.
>> When remove the OTG Storage, it reproduced sometimes.
>
>>  cpu 0   cpu 1
>>  
>> 
>>  (*Insert USB Storage)
>>  usb_create_shared_hcd()
>>  kmalloc(primary_hcd)
>>  kmalloc(primary_hcd->bandwidth_mutex)
>>   ->(primary_hcd->kref==1)
>>  usb_get_hcd()
>>   ->(primary_hcd->kref==2)
>>  usb_create_shared_hcd()
>>  kmalloc(hcd->shared_hcd)
>>   
>> ->hcd->shared_hcd->bandwidth_mutex=primary->bandwidth_mutex
>>   
>> ->primary_hcd->primary_hcd = primary_hcd
>>   
>> ->hcd->shared_hcd->primary_hcd = primary_hcd
>>  
>> ->(hcd->shared_hcd->kref==1)
>>  usb_get_hcd()
>>  
>> ->(hcd->shared_hcd->kref==2)
>> 
>>  usb_get_hcd()
>>   
>> ->(hcd->shared_hcd->kref==3)
>
>I don't understand.  Why do these actions take place on two different
>CPUs?  Aren't the primary_hcd and the shared_hcd structures allocated
>by the same thread, on the same CPU?

Yes, you are right, The presentational errors in order to obtain an 
understanding of the process.
Therefore, I will be happy to explain again the diagrammatic representation as 
shown below.
If using usb 3.0 storage(OTG), you can see as below.

==
At *Insert USB(3.0) Storage
sequence <1> --> <5>
==
VOLD   
=|
 (uevent)
   __|___ 
  |<5>   |
  |  SCSI|
  |usb_get_hcd   |
  |shared_hcd(kref=3)|
  |__|
   ___ |_ 
  |<2>|   |<4>   |
  |dwc3_otg_sm_work   |   |dwc3_otg_sm_work  |
  |usb_get_hcd|   |usb_get_hcd   |
  |primary_hcd(kref=2)|   |shared_hcd(kref=2)|
  |___|   |__|
   _|_ |_ 
  |<1>|   |<3>   |
  |New USB BUS #1 |   |New USB BUS #2|
  |usb_create_hcd |   |usb_create_hcd|
  |primary_hcd(kref=1)|   |shared_hcd(kref=1)|
  |   |   |  |
  |bandXX_mutex(alloc)|<-(Link)-bandXX_mutex |
  |___|   |__|

==
At *remove USB(3.0) Storage 
sequence <1> --> <5> ((Normal Case))
==
VOLD   
=|
 (uevent)
   __|___ 
  |<1>   |
  |  SCSI|
  |usb_put_hcd   |
  |shared_hcd(kref=2)|
  |__|
   ___ |_ 
  |<4>|   |<2>   |
  |dwc3_otg_sm_work   |   |dwc3_otg_sm_work  |
  |usb_put_hcd|   |usb_put_hcd   |
  |primary_hcd(kref=1)|   |shared_hcd(kref=1)|
  |___|   |__|
   _|_ |_ 
  |<5>|   |<3>   |
  |New USB BUS #1 |   |New USB BUS #2|
  |hcd_release|   |hcd_release   |
  |primary_hcd(kref=0)|   |shared_hcd(kref=0)|
  |   |   |  |
  |bandXX_mutex(free) | -X-cut off)-bandXX_mutex|
  |___|   |__|

--
>
>> 
>> ---
>>  (*remove USB Storage)   
>>  usb_release_dev()
>>   
>> ->(hcd->shared_hcd-kref==2)
>>  usb_release_dev()
>>   
>>

Re: Re: [PATCH] usb: core: fix a double free in the usb driver

2016-05-27 Thread Alan Stern 
On Fri, 27 May 2016, Chung-Geol Kim wrote:

> >On Fri, May 27, 2016 at 01:38:17AM +, Chung-Geol Kim wrote:
> >> There is a double free problem in the usb driver.
> >
> >Which driver?
> When I using the USB OTG Storage, this issue happened.
> When remove the OTG Storage, it reproduced sometimes.

>  cpu 0   cpu 1
>  
> 
>  (*Insert USB Storage)
>  usb_create_shared_hcd()
>  kmalloc(primary_hcd)
>  kmalloc(primary_hcd->bandwidth_mutex)
>   ->(primary_hcd->kref==1)
>  usb_get_hcd()
>   ->(primary_hcd->kref==2)
>  usb_create_shared_hcd()
>  kmalloc(hcd->shared_hcd)
>   
> ->hcd->shared_hcd->bandwidth_mutex=primary->bandwidth_mutex
>   
> ->primary_hcd->primary_hcd = primary_hcd
>   
> ->hcd->shared_hcd->primary_hcd = primary_hcd
>   
> ->(hcd->shared_hcd->kref==1)
>  usb_get_hcd()
>   
> ->(hcd->shared_hcd->kref==2)
> 
>  usb_get_hcd()
>   
> ->(hcd->shared_hcd->kref==3)

I don't understand.  Why do these actions take place on two different
CPUs?  Aren't the primary_hcd and the shared_hcd structures allocated
by the same thread, on the same CPU?

> 
> ---
>  (*remove USB Storage)
>  usb_release_dev()
>   
> ->(hcd->shared_hcd-kref==2)
>  usb_release_dev()
>   
> ->(hcd->shared_hcd-kref==1)
>  usb_release_dev()
>   -> (primary_hcd-kref==1)
>  usb_release_dev()
>   -> (primary_hcd-kref==0)
>  hcd_release()
>   -> kfree(primary_hcd->bandwidth_mutex)
>   -> hcd->shared_hcd->primary_hcd = NULL
>   -> kfree(primary_hcd)
>  usb_release_dev()
>   -> 
> (hcd->shared_hcd-kref==0)
>  hcd_release()
>   -> 
> usb_hcd_is_primary_hcd(hcd->shared_hcd)
>   -> 
> hcd->shared_hcd->primary_hcd already NULL, return 1
>   -> try to double 
> kfree(primary_hcd->bandwidth_mutex)

The same question applies here.  Aren't the shared_hcd and primary_hcd 
structures released by the same thread, on the same CPU?

The real bug here is that the shared_hcd is released after the 
primary_hcd.  That's what you need to fix.

> Since hcd->shared_hcd->priary_hcd was Null it didn't reach (hcd == 
> hcd->primary_hcd) in usb_hcd_is_primary_hcd().
> It returned 1 at since condition !hcd->primary_hcd is met.

> >> --- a/drivers/usb/core/hcd.c
> >> +++ b/drivers/usb/core/hcd.c
> >> @@ -2608,7 +2608,7 @@ static void hcd_release(struct kref *kref)
> >> struct usb_hcd *hcd = container_of (kref, struct usb_hcd, kref);
> >> 
> >> mutex_lock(_port_peer_mutex);
> >> - if (usb_hcd_is_primary_hcd(hcd)) {
> >> + if (hcd == hcd->primary_hcd) {
> >
> >That doesn't make sense, usb_hcd_is_primary_hcd() is the same as this
> >check, what are you changing here?
> 
> Since hcd->priary_hcd was Null it didn't reach (hcd == hcd->primary_hcd).
> It returned 1 at since condition !hcd->primary_hcd is met.
> 
> int usb_hcd_is_primary_hcd(struct usb_hcd *hcd)
> {
>   if (!hcd->primary_hcd)
>   return 1;
>   return hcd == hcd->primary_hcd;
> }

That's just a symptom, not the real cause of the bug.  You need to fix 
the real cause: the shared_hcd has to be released _before_ the 
primary_hcd.

The right way to do this is to make the shared_hcd take a reference to
the primary_hcd.  This reference should be dropped when hcd_release()
is called for the shared_hcd.

Alan Stern

Re: Re: [PATCH] usb: core: fix a double free in the usb driver

2016-05-27 Thread Alan Stern 
On Fri, 27 May 2016, Chung-Geol Kim wrote:

> >On Fri, May 27, 2016 at 01:38:17AM +, Chung-Geol Kim wrote:
> >> There is a double free problem in the usb driver.
> >
> >Which driver?
> When I using the USB OTG Storage, this issue happened.
> When remove the OTG Storage, it reproduced sometimes.

>  cpu 0   cpu 1
>  
> 
>  (*Insert USB Storage)
>  usb_create_shared_hcd()
>  kmalloc(primary_hcd)
>  kmalloc(primary_hcd->bandwidth_mutex)
>   ->(primary_hcd->kref==1)
>  usb_get_hcd()
>   ->(primary_hcd->kref==2)
>  usb_create_shared_hcd()
>  kmalloc(hcd->shared_hcd)
>   
> ->hcd->shared_hcd->bandwidth_mutex=primary->bandwidth_mutex
>   
> ->primary_hcd->primary_hcd = primary_hcd
>   
> ->hcd->shared_hcd->primary_hcd = primary_hcd
>   
> ->(hcd->shared_hcd->kref==1)
>  usb_get_hcd()
>   
> ->(hcd->shared_hcd->kref==2)
> 
>  usb_get_hcd()
>   
> ->(hcd->shared_hcd->kref==3)

I don't understand.  Why do these actions take place on two different
CPUs?  Aren't the primary_hcd and the shared_hcd structures allocated
by the same thread, on the same CPU?

> 
> ---
>  (*remove USB Storage)
>  usb_release_dev()
>   
> ->(hcd->shared_hcd-kref==2)
>  usb_release_dev()
>   
> ->(hcd->shared_hcd-kref==1)
>  usb_release_dev()
>   -> (primary_hcd-kref==1)
>  usb_release_dev()
>   -> (primary_hcd-kref==0)
>  hcd_release()
>   -> kfree(primary_hcd->bandwidth_mutex)
>   -> hcd->shared_hcd->primary_hcd = NULL
>   -> kfree(primary_hcd)
>  usb_release_dev()
>   -> 
> (hcd->shared_hcd-kref==0)
>  hcd_release()
>   -> 
> usb_hcd_is_primary_hcd(hcd->shared_hcd)
>   -> 
> hcd->shared_hcd->primary_hcd already NULL, return 1
>   -> try to double 
> kfree(primary_hcd->bandwidth_mutex)

The same question applies here.  Aren't the shared_hcd and primary_hcd 
structures released by the same thread, on the same CPU?

The real bug here is that the shared_hcd is released after the 
primary_hcd.  That's what you need to fix.

> Since hcd->shared_hcd->priary_hcd was Null it didn't reach (hcd == 
> hcd->primary_hcd) in usb_hcd_is_primary_hcd().
> It returned 1 at since condition !hcd->primary_hcd is met.

> >> --- a/drivers/usb/core/hcd.c
> >> +++ b/drivers/usb/core/hcd.c
> >> @@ -2608,7 +2608,7 @@ static void hcd_release(struct kref *kref)
> >> struct usb_hcd *hcd = container_of (kref, struct usb_hcd, kref);
> >> 
> >> mutex_lock(_port_peer_mutex);
> >> - if (usb_hcd_is_primary_hcd(hcd)) {
> >> + if (hcd == hcd->primary_hcd) {
> >
> >That doesn't make sense, usb_hcd_is_primary_hcd() is the same as this
> >check, what are you changing here?
> 
> Since hcd->priary_hcd was Null it didn't reach (hcd == hcd->primary_hcd).
> It returned 1 at since condition !hcd->primary_hcd is met.
> 
> int usb_hcd_is_primary_hcd(struct usb_hcd *hcd)
> {
>   if (!hcd->primary_hcd)
>   return 1;
>   return hcd == hcd->primary_hcd;
> }

That's just a symptom, not the real cause of the bug.  You need to fix 
the real cause: the shared_hcd has to be released _before_ the 
primary_hcd.

The right way to do this is to make the shared_hcd take a reference to
the primary_hcd.  This reference should be dropped when hcd_release()
is called for the shared_hcd.

Alan Stern

Re: Re: [PATCH] usb: core: fix a double free in the usb driver

2016-05-27 Thread Chung-Geol Kim 
>On Fri, May 27, 2016 at 01:38:17AM +, Chung-Geol Kim wrote:
>> There is a double free problem in the usb driver.
>
>Which driver?
When I using the USB OTG Storage, this issue happened.
When remove the OTG Storage, it reproduced sometimes.

>
>> This is caused by delayed deregister for scsi device.
>> <*> at Insert USB Storage
>> -  USB bus #1 register
>>  usb_create_hcd (primary-kref==1)
>>  * primary-bandwidth_mutex(alloc))
>> usb_get_hcd(primary-kref==2)
>> -  USB bus #2 register
>> usb_create_hcd (second-kref==1)
>>  * second-bandwidth_mutex==primary-bandwidth_mutex
>>  usb_get_hcd(second-kref==2)
>> -  scsi_device_get
>> usb_get_hcd(second-kref==3)
>> 
>> <*> at remove USB Storage (Normal)
>> -  scsi_device_put
>> usb_put_hcd(second-kref==2)
>> -  USB bus #2 deregister
>>  usb_release_dev(second-kref==1)
>> usb_release_dev(second-kref==0)  -> hcd_release()
>> -  USB bus #1 deregister
>>  usb_release_dev(primary-kref==1)
>>  usb_release_dev(primary-kref==0) -> hcd_release()
>> *(primary-bandwidth_mutex free)
>> 
>> at remove USB Storage 
>> -  USB bus #2 deregister
>>  usb_release_dev(second-kref==2)
>> usb_release_dev(second-kref==1)
>> -  USB bus #1 deregister
>>  usb_release_dev(primary-kref==1)
>>  usb_release_dev(primary-kref==0) -> hcd_release()
>>  *(primary-bandwidth_mutex free)
>> -  scsi_device_put
>>  usb_put_hcd(second-kref==0)  -> hcd_release(*)
>>  * at this, second->primary==0 therefore try to
>> free the primary-bandwidth_mutex.(already freed)
>
>The formatting for this is all confused, can you fix it up?
Sorry to confuse you, Let me change as below.
 cpu 0   cpu 1
 

 (*Insert USB Storage)
 usb_create_shared_hcd()
 kmalloc(primary_hcd)
 kmalloc(primary_hcd->bandwidth_mutex)
  ->(primary_hcd->kref==1)
 usb_get_hcd()
  ->(primary_hcd->kref==2)
 usb_create_shared_hcd()
 kmalloc(hcd->shared_hcd)
  
->hcd->shared_hcd->bandwidth_mutex=primary->bandwidth_mutex
  
->primary_hcd->primary_hcd = primary_hcd
  
->hcd->shared_hcd->primary_hcd = primary_hcd

->(hcd->shared_hcd->kref==1)
 usb_get_hcd()

->(hcd->shared_hcd->kref==2)

 usb_get_hcd()
  
->(hcd->shared_hcd->kref==3)

---
 (*remove USB Storage)  
 usb_release_dev()
  
->(hcd->shared_hcd-kref==2)
 usb_release_dev()
  
->(hcd->shared_hcd-kref==1)
 usb_release_dev()
  -> (primary_hcd-kref==1)
 usb_release_dev()
  -> (primary_hcd-kref==0)
 hcd_release()
  -> kfree(primary_hcd->bandwidth_mutex)
  -> hcd->shared_hcd->primary_hcd = NULL
  -> kfree(primary_hcd)
 usb_release_dev()
  -> 
(hcd->shared_hcd-kref==0)
 hcd_release()
  -> 
usb_hcd_is_primary_hcd(hcd->shared_hcd)
  -> 
hcd->shared_hcd->primary_hcd already NULL, return 1
  -> try to double 
kfree(primary_hcd->bandwidth_mutex)

Since hcd->shared_hcd->priary_hcd was Null it didn't reach (hcd == 
hcd->primary_hcd) in usb_hcd_is_primary_hcd().
It returned 1 at since condition !hcd->primary_hcd is met.

>
>> 
>> To fix this problem kfree(hcd->bandwidth_mutex);
>> should be executed at only (hcd->primary_hcd==hcd).
>> 
>> Signed-off-by: Chunggeol Kim 
>
>We need an email address at the end of this line, look at how the
>commits in the kernel git history look like for examples.

sorry, maybe deleted during transfer the patch.
Signed-off-by: Chunggeol Kim 

>
>> ---
>> drivers/usb/core/hcd.c |2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index 34b837a..60077f3 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -2608,7

Re: Re: [PATCH] usb: core: fix a double free in the usb driver

2016-05-27 Thread Chung-Geol Kim 
>On Fri, May 27, 2016 at 01:38:17AM +, Chung-Geol Kim wrote:
>> There is a double free problem in the usb driver.
>
>Which driver?
When I using the USB OTG Storage, this issue happened.
When remove the OTG Storage, it reproduced sometimes.

>
>> This is caused by delayed deregister for scsi device.
>> <*> at Insert USB Storage
>> -  USB bus #1 register
>>  usb_create_hcd (primary-kref==1)
>>  * primary-bandwidth_mutex(alloc))
>> usb_get_hcd(primary-kref==2)
>> -  USB bus #2 register
>> usb_create_hcd (second-kref==1)
>>  * second-bandwidth_mutex==primary-bandwidth_mutex
>>  usb_get_hcd(second-kref==2)
>> -  scsi_device_get
>> usb_get_hcd(second-kref==3)
>> 
>> <*> at remove USB Storage (Normal)
>> -  scsi_device_put
>> usb_put_hcd(second-kref==2)
>> -  USB bus #2 deregister
>>  usb_release_dev(second-kref==1)
>> usb_release_dev(second-kref==0)  -> hcd_release()
>> -  USB bus #1 deregister
>>  usb_release_dev(primary-kref==1)
>>  usb_release_dev(primary-kref==0) -> hcd_release()
>> *(primary-bandwidth_mutex free)
>> 
>> at remove USB Storage 
>> -  USB bus #2 deregister
>>  usb_release_dev(second-kref==2)
>> usb_release_dev(second-kref==1)
>> -  USB bus #1 deregister
>>  usb_release_dev(primary-kref==1)
>>  usb_release_dev(primary-kref==0) -> hcd_release()
>>  *(primary-bandwidth_mutex free)
>> -  scsi_device_put
>>  usb_put_hcd(second-kref==0)  -> hcd_release(*)
>>  * at this, second->primary==0 therefore try to
>> free the primary-bandwidth_mutex.(already freed)
>
>The formatting for this is all confused, can you fix it up?
Sorry to confuse you, Let me change as below.
 cpu 0   cpu 1
 

 (*Insert USB Storage)
 usb_create_shared_hcd()
 kmalloc(primary_hcd)
 kmalloc(primary_hcd->bandwidth_mutex)
  ->(primary_hcd->kref==1)
 usb_get_hcd()
  ->(primary_hcd->kref==2)
 usb_create_shared_hcd()
 kmalloc(hcd->shared_hcd)
  
->hcd->shared_hcd->bandwidth_mutex=primary->bandwidth_mutex
  
->primary_hcd->primary_hcd = primary_hcd
  
->hcd->shared_hcd->primary_hcd = primary_hcd

->(hcd->shared_hcd->kref==1)
 usb_get_hcd()

->(hcd->shared_hcd->kref==2)

 usb_get_hcd()
  
->(hcd->shared_hcd->kref==3)

---
 (*remove USB Storage)  
 usb_release_dev()
  
->(hcd->shared_hcd-kref==2)
 usb_release_dev()
  
->(hcd->shared_hcd-kref==1)
 usb_release_dev()
  -> (primary_hcd-kref==1)
 usb_release_dev()
  -> (primary_hcd-kref==0)
 hcd_release()
  -> kfree(primary_hcd->bandwidth_mutex)
  -> hcd->shared_hcd->primary_hcd = NULL
  -> kfree(primary_hcd)
 usb_release_dev()
  -> 
(hcd->shared_hcd-kref==0)
 hcd_release()
  -> 
usb_hcd_is_primary_hcd(hcd->shared_hcd)
  -> 
hcd->shared_hcd->primary_hcd already NULL, return 1
  -> try to double 
kfree(primary_hcd->bandwidth_mutex)

Since hcd->shared_hcd->priary_hcd was Null it didn't reach (hcd == 
hcd->primary_hcd) in usb_hcd_is_primary_hcd().
It returned 1 at since condition !hcd->primary_hcd is met.

>
>> 
>> To fix this problem kfree(hcd->bandwidth_mutex);
>> should be executed at only (hcd->primary_hcd==hcd).
>> 
>> Signed-off-by: Chunggeol Kim 
>
>We need an email address at the end of this line, look at how the
>commits in the kernel git history look like for examples.

sorry, maybe deleted during transfer the patch.
Signed-off-by: Chunggeol Kim 

>
>> ---
>> drivers/usb/core/hcd.c |2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index 34b837a..60077f3 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -2608,7 +2608,7 @@ static void

Re: [PATCH] usb: core: fix a double free in the usb driver

2016-05-26 Thread gre...@linuxfoundation.org 
On Fri, May 27, 2016 at 01:38:17AM +, Chung-Geol Kim wrote:
> There is a double free problem in the usb driver.

Which driver?

> This is caused by delayed deregister for scsi device.
> <*> at Insert USB Storage
> -  USB bus #1 register
>  usb_create_hcd (primary-kref==1)
>  * primary-bandwidth_mutex(alloc))
> usb_get_hcd(primary-kref==2)
> -  USB bus #2 register
> usb_create_hcd (second-kref==1)
>  * second-bandwidth_mutex==primary-bandwidth_mutex
>  usb_get_hcd(second-kref==2)
> -  scsi_device_get
> usb_get_hcd(second-kref==3)
> 
> <*> at remove USB Storage (Normal)
> -  scsi_device_put
> usb_put_hcd(second-kref==2)
> -  USB bus #2 deregister
>  usb_release_dev(second-kref==1)
> usb_release_dev(second-kref==0)  -> hcd_release()
> -  USB bus #1 deregister
>  usb_release_dev(primary-kref==1)
>  usb_release_dev(primary-kref==0) -> hcd_release()
> *(primary-bandwidth_mutex free)
> 
> at remove USB Storage 
> -  USB bus #2 deregister
>  usb_release_dev(second-kref==2)
> usb_release_dev(second-kref==1)
> -  USB bus #1 deregister
>  usb_release_dev(primary-kref==1)
>  usb_release_dev(primary-kref==0) -> hcd_release()
>  *(primary-bandwidth_mutex free)
> -  scsi_device_put
>  usb_put_hcd(second-kref==0)  -> hcd_release(*)
>  * at this, second->primary==0 therefore try to
> free the primary-bandwidth_mutex.(already freed)

The formatting for this is all confused, can you fix it up?

> 
> To fix this problem kfree(hcd->bandwidth_mutex);
> should be executed at only (hcd->primary_hcd==hcd).
> 
> Signed-off-by: Chunggeol Kim 

We need an email address at the end of this line, look at how the
commits in the kernel git history look like for examples.

> ---
> drivers/usb/core/hcd.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 34b837a..60077f3 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2608,7 +2608,7 @@ static void hcd_release(struct kref *kref)
> struct usb_hcd *hcd = container_of (kref, struct usb_hcd, kref);
> 
> mutex_lock(_port_peer_mutex);
> - if (usb_hcd_is_primary_hcd(hcd)) {
> + if (hcd == hcd->primary_hcd) {

That doesn't make sense, usb_hcd_is_primary_hcd() is the same as this
check, what are you changing here?

> kfree(hcd->address0_mutex);
> kfree(hcd->bandwidth_mutex);
> }

Your patch itself is also corrupted, and can't be applied, can you also
resolve this and resend?

thanks,

greg k-h

Re: [PATCH] usb: core: fix a double free in the usb driver

2016-05-26 Thread gre...@linuxfoundation.org 
On Fri, May 27, 2016 at 01:38:17AM +, Chung-Geol Kim wrote:
> There is a double free problem in the usb driver.

Which driver?

> This is caused by delayed deregister for scsi device.
> <*> at Insert USB Storage
> -  USB bus #1 register
>  usb_create_hcd (primary-kref==1)
>  * primary-bandwidth_mutex(alloc))
> usb_get_hcd(primary-kref==2)
> -  USB bus #2 register
> usb_create_hcd (second-kref==1)
>  * second-bandwidth_mutex==primary-bandwidth_mutex
>  usb_get_hcd(second-kref==2)
> -  scsi_device_get
> usb_get_hcd(second-kref==3)
> 
> <*> at remove USB Storage (Normal)
> -  scsi_device_put
> usb_put_hcd(second-kref==2)
> -  USB bus #2 deregister
>  usb_release_dev(second-kref==1)
> usb_release_dev(second-kref==0)  -> hcd_release()
> -  USB bus #1 deregister
>  usb_release_dev(primary-kref==1)
>  usb_release_dev(primary-kref==0) -> hcd_release()
> *(primary-bandwidth_mutex free)
> 
> at remove USB Storage 
> -  USB bus #2 deregister
>  usb_release_dev(second-kref==2)
> usb_release_dev(second-kref==1)
> -  USB bus #1 deregister
>  usb_release_dev(primary-kref==1)
>  usb_release_dev(primary-kref==0) -> hcd_release()
>  *(primary-bandwidth_mutex free)
> -  scsi_device_put
>  usb_put_hcd(second-kref==0)  -> hcd_release(*)
>  * at this, second->primary==0 therefore try to
> free the primary-bandwidth_mutex.(already freed)

The formatting for this is all confused, can you fix it up?

> 
> To fix this problem kfree(hcd->bandwidth_mutex);
> should be executed at only (hcd->primary_hcd==hcd).
> 
> Signed-off-by: Chunggeol Kim 

We need an email address at the end of this line, look at how the
commits in the kernel git history look like for examples.

> ---
> drivers/usb/core/hcd.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 34b837a..60077f3 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2608,7 +2608,7 @@ static void hcd_release(struct kref *kref)
> struct usb_hcd *hcd = container_of (kref, struct usb_hcd, kref);
> 
> mutex_lock(_port_peer_mutex);
> - if (usb_hcd_is_primary_hcd(hcd)) {
> + if (hcd == hcd->primary_hcd) {

That doesn't make sense, usb_hcd_is_primary_hcd() is the same as this
check, what are you changing here?

> kfree(hcd->address0_mutex);
> kfree(hcd->bandwidth_mutex);
> }

Your patch itself is also corrupted, and can't be applied, can you also
resolve this and resend?

thanks,

greg k-h

[PATCH] usb: core: fix a double free in the usb driver

2016-05-26 Thread Chung-Geol Kim 
There is a double free problem in the usb driver.
This is caused by delayed deregister for scsi device.
<*> at Insert USB Storage
-  USB bus #1 register
 usb_create_hcd (primary-kref==1)
 * primary-bandwidth_mutex(alloc))
usb_get_hcd(primary-kref==2)
-  USB bus #2 register
usb_create_hcd (second-kref==1)
 * second-bandwidth_mutex==primary-bandwidth_mutex
 usb_get_hcd(second-kref==2)
-  scsi_device_get
usb_get_hcd(second-kref==3)

<*> at remove USB Storage (Normal)
-  scsi_device_put
usb_put_hcd(second-kref==2)
-  USB bus #2 deregister
 usb_release_dev(second-kref==1)
usb_release_dev(second-kref==0)  -> hcd_release()
-  USB bus #1 deregister
 usb_release_dev(primary-kref==1)
 usb_release_dev(primary-kref==0) -> hcd_release()
*(primary-bandwidth_mutex free)

at remove USB Storage 
-  USB bus #2 deregister
 usb_release_dev(second-kref==2)
usb_release_dev(second-kref==1)
-  USB bus #1 deregister
 usb_release_dev(primary-kref==1)
 usb_release_dev(primary-kref==0) -> hcd_release()
 *(primary-bandwidth_mutex free)
-  scsi_device_put
 usb_put_hcd(second-kref==0)  -> hcd_release(*)
 * at this, second->primary==0 therefore try to
free the primary-bandwidth_mutex.(already freed)

To fix this problem kfree(hcd->bandwidth_mutex);
should be executed at only (hcd->primary_hcd==hcd).

Signed-off-by: Chunggeol Kim 
---
drivers/usb/core/hcd.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 34b837a..60077f3 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2608,7 +2608,7 @@ static void hcd_release(struct kref *kref)
struct usb_hcd *hcd = container_of (kref, struct usb_hcd, kref);

mutex_lock(_port_peer_mutex);
- if (usb_hcd_is_primary_hcd(hcd)) {
+ if (hcd == hcd->primary_hcd) {
kfree(hcd->address0_mutex);
kfree(hcd->bandwidth_mutex);
}
-- 
1.7.9.5

[PATCH] usb: core: fix a double free in the usb driver

2016-05-26 Thread Chung-Geol Kim 
There is a double free problem in the usb driver.
This is caused by delayed deregister for scsi device.
<*> at Insert USB Storage
-  USB bus #1 register
 usb_create_hcd (primary-kref==1)
 * primary-bandwidth_mutex(alloc))
usb_get_hcd(primary-kref==2)
-  USB bus #2 register
usb_create_hcd (second-kref==1)
 * second-bandwidth_mutex==primary-bandwidth_mutex
 usb_get_hcd(second-kref==2)
-  scsi_device_get
usb_get_hcd(second-kref==3)

<*> at remove USB Storage (Normal)
-  scsi_device_put
usb_put_hcd(second-kref==2)
-  USB bus #2 deregister
 usb_release_dev(second-kref==1)
usb_release_dev(second-kref==0)  -> hcd_release()
-  USB bus #1 deregister
 usb_release_dev(primary-kref==1)
 usb_release_dev(primary-kref==0) -> hcd_release()
*(primary-bandwidth_mutex free)

at remove USB Storage 
-  USB bus #2 deregister
 usb_release_dev(second-kref==2)
usb_release_dev(second-kref==1)
-  USB bus #1 deregister
 usb_release_dev(primary-kref==1)
 usb_release_dev(primary-kref==0) -> hcd_release()
 *(primary-bandwidth_mutex free)
-  scsi_device_put
 usb_put_hcd(second-kref==0)  -> hcd_release(*)
 * at this, second->primary==0 therefore try to
free the primary-bandwidth_mutex.(already freed)

To fix this problem kfree(hcd->bandwidth_mutex);
should be executed at only (hcd->primary_hcd==hcd).

Signed-off-by: Chunggeol Kim 
---
drivers/usb/core/hcd.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 34b837a..60077f3 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2608,7 +2608,7 @@ static void hcd_release(struct kref *kref)
struct usb_hcd *hcd = container_of (kref, struct usb_hcd, kref);

mutex_lock(_port_peer_mutex);
- if (usb_hcd_is_primary_hcd(hcd)) {
+ if (hcd == hcd->primary_hcd) {
kfree(hcd->address0_mutex);
kfree(hcd->bandwidth_mutex);
}
-- 
1.7.9.5