On Fri, 27 May 2016, Chung-Geol Kim wrote:
> >On Fri, May 27, 2016 at 01:38:17AM +0000, 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(&usb_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
