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

mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed

* RE: Re: Re: Re: [PATCH] usb: core: fix a double free in the usb driver
       [not found] <CGME20160603142804epcas3p43ccf34c3557e95062ef54131f50d4014@epcas3p4.samsung.com>
@ 2016-06-07  4:39 ` Chung-Geol Kim
  2016-06-07 14:54   ` Alan Stern
  0 siblings, 1 reply; 2+ messages in thread
From: Chung-Geol Kim @ 2016-06-07  4:39 UTC (permalink / raw)
  To: Alan Stern
  Cc: gregkh, mathias.nyman, stefan.koch10, hkallweit1,
	sergei.shtylyov, dan.j.williams, sarah.a.sharp, chris.bainbridge,
	linux-usb, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 11244 bytes --]

>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)|
                             |                  |
                          -X-(cut off)-bandXX_mutex|
                           | |__________________|
                           |
      ___________________  |
     |<4>                | |
     |dwc3_otg_sm_work   | |
     |usb_put_hcd        | |
     |primary_hcd(kref=1)| |
     |___________________| |
      _________|_________  |
     |<5>                | |
     |New USB BUS #1     | |
     |hcd_release        | |
     |primary_hcd(kref=0)| |
     |                   | |
     |bandXX_mutex(free) |<-
     |___________________| 

=================================================

>
>> ==============================================
>>     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 |
>>   |___________________|   |__________________|
>> 
>> ----------------------------------------------
>
>That's the same as what I said.  In your diagram, vold releases the 
>shared_hcd (in <5>) after dwc3_otg_sm_work releases the primary_hcd 
>(in <4>).
>


=================================================
     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
                           |__________________|

=================================================

>If you change the code so that the shared_hcd takes a reference to the 
>primary_hcd and drops that reference when the shared_hcd is released, 
>this will never occur.
>
>Alan Stern

^ permalink raw reply	[flat|nested] 2+ messages in thread

* RE: Re: Re: Re: [PATCH] usb: core: fix a double free in the usb driver
  2016-06-07  4:39 ` Re: Re: Re: [PATCH] usb: core: fix a double free in the usb driver Chung-Geol Kim
@ 2016-06-07 14:54   ` Alan Stern
  0 siblings, 0 replies; 2+ messages in thread
From: Alan Stern @ 2016-06-07 14:54 UTC (permalink / raw)
  To: Chung-Geol Kim
  Cc: gregkh, mathias.nyman, stefan.koch10, hkallweit1,
	sergei.shtylyov, dan.j.williams, chris.bainbridge, linux-usb,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2016-06-07 14:54 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20160603142804epcas3p43ccf34c3557e95062ef54131f50d4014@epcas3p4.samsung.com>
2016-06-07  4:39 ` Re: Re: Re: [PATCH] usb: core: fix a double free in the usb driver Chung-Geol Kim
2016-06-07 14:54   ` Alan Stern

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®