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

Quoted 60 lines · Rafael J. Wysocki

Quoted 59 lines · Rafael J. Wysocki

Quoted 58 lines · Johan Hovold

Quoted 29 lines · Rafael J. Wysocki

Quoted 28 lines · CEST Johan Hovold

Quoted 26 lines · Tony Lindgren

Hi,

Adding Rafael and linux-pm to Cc as well.

Quoted 21 lines · Felipe Balbi

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)

Quoted 6 lines

Quoted 4 lines

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 11 lines

Quoted 2 lines

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:

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

                                        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?

Quoted 17 lines

Quoted 16 lines

Quoted 10 lines

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.

Quoted 3 lines

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?