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

On Thu, Jun 21, 2018 at 12:32 AM, Rafael J. Wysocki [off-list ref] wrote:

On Wed, Jun 20, 2018 at 5:46 PM, Johan Hovold [off-list ref] wrote:

quoted

On Wed, Jun 20, 2018 at 02:54:10PM +0200, Rafael J. Wysocki wrote:

quoted

On Wednesday, June 20, 2018 2:23:46 PM CEST Johan Hovold wrote:

quoted

On Wed, Jun 20, 2018 at 02:16:59AM -0700, Tony Lindgren wrote:

quoted

Hi,

Adding Rafael and linux-pm to Cc as well.

* Felipe Balbi [off-list ref] [180619 01:23]:

quoted

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.

quoted

quoted

(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).

quoted

quoted

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
        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().
---
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