usb: dwc3: of-simple: fix use-after-free on remove

[Johan Hovold <johan@kernel.org>]

On Thu, Jun 21, 2018 at 11:17:36AM +0300, Roger Quadros wrote:
> On 21/06/18 01:55, Rafael J. Wysocki wrote:
> > On Thu, Jun 21, 2018 at 12:32 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >> On Wed, Jun 20, 2018 at 5:46 PM, Johan Hovold <johan@kernel.org> wrote:
> >>> On Wed, Jun 20, 2018 at 02:54:10PM +0200, Rafael J. Wysocki wrote:
> >>>> On Wednesday, June 20, 2018 2:23:46 PM CEST Johan Hovold wrote:
> >>>>> On Wed, Jun 20, 2018 at 02:16:59AM -0700, Tony Lindgren wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> Adding Rafael and linux-pm to Cc as well.
> >>>>>>
> >>>>>> * Felipe Balbi <balbi@kernel.org> [180619 01:23]:
> >>>>>>> This is a direct consequence of not paying attention to the order of
> >>>>>>> things. If driver were to assume that pm_domain->activate() would do the
> >>>>>>> right thing for the device -- meaning that probe would run with an
> >>>>>>> active device --, then we wouldn't need that pm_runtime_get() call on
> >>>>>>> probe at all. Rather we would follow the sequence:
> >>>>>>>
> >>>>>>>         pm_runtime_forbid()
> >>>>>>>         pm_runtime_set_active()
> >>>>>>>         pm_runtime_enable()
> >>>>>>>
> >>>>>>>         /* do your probe routine */
> >>>>>>>
> >>>>>>>         pm_runtime_put_noidle()
> >>>>>>>
> >>>>>>> Then you remove you would need to call pm_runtime_get_noresume() to
> >>>>>>> balance out the pm_runtime_put_noidle() there.
> >>>>>
> >>>>>>> (If you need to know why the pm_runtime_put_noidle(), remember that
> >>>>>>> pm_runtime_set_active() increments the usage counter, so
> >>>>>>> pm_runtime_put_noidle is basically allowing pm_runtime to happen as soon
> >>>>>>> as userspace writes "auto" to /sys/..../power/control)
> >>>>>
> >>>>> That's not correct; pm_runtime_set_active() only increments the usage
> >>>>> counter of a parent (under some circumstances), so unless you have bus
> >>>>> code incrementing the usage counter before probe, the above
> >>>>> pm_runtime_put_noidle() would actually introduce an imbalance.
> >>>>
> >>>> No, it wouldn't.  It balances the incrementation in pm_runtime_forbid().
> >>>
> >>> Right, but even if you take the whole sequence, which included
> >>> pm_runtime_forbid(), consider what happens when pm_runtime_allow() is
> >>> later called through sysfs (see below).
> >>>
> >>>>> And note that that's also the case even if you meant to say that
> >>>>> *pm_runtime_forbid()* increments the usage counter (which it does).
> >>>>
> >>>> Why is it?
> >>>>
> >>>> Surely, after
> >>>>
> >>>> pm_runtime_forbid(dev);
> >>>> pm_runtime_put_noidle(dev);
> >>>>
> >>>> the runtime PM usage counter of dev will be the same as before, won't it?
> >>>
> >>> Sure, but the imbalance, or rather inconsistent state, has already been
> >>> introduced.
> >>>
> >>> Consider the following sequence of events:
> >>>
> >>>                                         usage count
> >>>                                         0
> >>> probe()
> >>>         pm_runtime_forbid()             1
> 
> Can you call pm_runtime_forbid() before pm_runtime_enable()?
> Wouldn't it fail with -EACCES as dev->power.disable_depth > 0?

No, pm_runtime_forbid() manipulates the usage count directly and doesn't
check for errors from rpm_resume(), so this actually "works".

That doesn't mean anyone should be doing it, though (e.g. for the below
reasons).

> >>>         pm_runtime_set_active()
> >>>         pm_runtime_enable()
> >>>         pm_runtime_put_noidle()         0
> >>>
> >>> Here nothing is preventing the device from runtime suspending, despite
> >>> runtime PM being forbidden. In fact, it will typically be suspended due
> >>> to the pm_request_idle() in driver_probe_device(). If later we have:
> >>>
> >>> echo auto > power/control
> >>>         pm_runtime_allow()              -1
> >>
> >> OK, you have a point.
> >>
> >> After calling pm_runtime_forbid() the driver should allow user space
> >> to unblock runtime PM for the device - or call pm_runtime_allow()
> >> itself.
> > 
> > The confusion regarding the pm_runtime_put_noidle() at the end may
> > come from the special requirement of the PCI bus type as per the
> > comment in local_pci_probe().
> 
> OK. So it is the PCI bus which is behaving odd here and
> pm_runtime_put_noidle() needs to be done only if its a PCI device, correct?

Yeah, the pm_runtime_put_noidle() would be used to balance the
unconditional runtime resume by the bus code in case a PCI driver
implements runtime pm.

Johan
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html