Re: Infinite recursion in device_reorder_to_tail() due to circular device links

[Stephan Gerhold]

On Tue, Jan 12, 2021 at 07:24:24PM +0100, Rafael J. Wysocki wrote:
> On Tue, Jan 12, 2021 at 3:32 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> >
> > On Mon, Jan 11, 2021 at 7:46 PM Stephan Gerhold <stephan@xxxxxxxxxxx> wrote:
> > >
> > > Hi,
> > >
> > > since 5.11-rc1 I get kernel crashes with infinite recursion in
> > > device_reorder_to_tail() in some situations... It's a bit complicated to
> > > explain so I want to apologize in advance for the long mail. :)
> > >
> > >   Kernel panic - not syncing: kernel stack overflow
> > >   CPU: 1 PID: 33 Comm: kworker/1:1 Not tainted 5.11.0-rc3 #1
> > >   Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
> > >   Call trace:
> > >    ...
> > >    device_reorder_to_tail+0x4c/0xf0
> > >    device_reorder_to_tail+0x98/0xf0
> > >    device_reorder_to_tail+0x60/0xf0
> > >    device_reorder_to_tail+0x60/0xf0
> > >    device_reorder_to_tail+0x60/0xf0
> > >    ...
> > >
> > > The crash happens only in 5.11 with commit 5b6164d3465f ("driver core:
> > > Reorder devices on successful probe"). It stops happening when I revert
> > > this commit.
> >
> > Thanks for the report!
> >
> > Greg, please revert commit 5b6164d3465f, it clearly is not an
> > improvement, at least at this point.
> >

Thanks a lot for the quick reply and for reverting the patch!

> > > But I don't think this commit is the actual problem...
> >
> > Well, it may not be the root cause, but it is a change in behavior
> > that exposes the breakage and this is not the only problem introduced
> > by it.
> >
> > > It's easy to reproduce on any device based on the Qualcomm MSM8916 SoC
> > > by adding a random regulator to the USB node, e.g.:
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> > > index 3a9538e1ec97..9f43fce9e6e3 100644
> > > --- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> > > @@ -372,6 +372,7 @@ codec {
> > >
> > >  &usb {
> > >         status = "okay";
> > > +       vbus-supply = <&pm8916_l5>;
> > >         extcon = <&usb_id>, <&usb_id>;
> > >
> > >         pinctrl-names = "default", "device";
> > >
> > > I searched for problems in the regulator core but the problem actually
> > > has nothing to do with regulators: The additional regulator supply just
> > > delays probing of the USB driver long enough to trigger the issue.
> > >
> > > Adding some debug output to device_reorder_to_tail() reveals that it
> > > keeps recursing over the same 4 devices:
> > >
> > >   msm_hsusb 78d9000.usb: device_reorder_to_tail()
> > >   ci_hdrc ci_hdrc.0: device_reorder_to_tail()
> > >   qcom_usb_hs_phy ci_hdrc.0.ulpi: device_reorder_to_tail()
> > >   phy phy-ci_hdrc.0.ulpi.0: device_reorder_to_tail()
> > >   msm_hsusb 78d9000.usb: device_reorder_to_tail()
> > >   ...
> > >
> > > The device hierarchy of these is (children devices):
> > >
> > >   78d9000.usb -> ci_hdrc.0 -> ci_hdrc.0.ulpi -> phy-ci_hdrc.0.ulpi.0
> > >
> > > ci_hdrc.0 calls phy_get(dev->parent, "usb-phy"). In phy_get(),
> > > the phy-core then attempts to add the following device link:
> > >
> > >   phy-ci_hdrc.0.ulpi.0 -> 78d9000.usb
> > >
> > > The device link creation in phy-core is optional (see commit 1d7cb11e1090
> > > ("phy: core: Fix phy_get() to not return error on link creation failure"))
> > > because this device link is circular in case of ULPI PHYs (like here).
> > >
> > > And indeed, creating this device link usually fails (as it should).
> > > However, adding the "vbus-supply" changes probe order in some way that
> > > this circular device link ends up being created:
> > >   /sys/class/devlink/phy-ci_hdrc.0.ulpi.0--78d9000.usb/ exists only when
> > > I add the "vbus-supply" as in the diff above.
> > >
> > > Apparently, there is a special situation where device_is_dependent()
> > > does not work properly, and therefore the driver core allows creating
> > > the circular device link.
> > >
> > > To show the problem, I enabled some debug messages and added the
> > > following log message:
> > >
> > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > index 25e08e5f40bd..ff1344eabb31 100644
> > > --- a/drivers/base/core.c
> > > +++ b/drivers/base/core.c
> > > @@ -3089,9 +3089,11 @@ int device_add(struct device *dev)
> > >         }
> > >
> > >         bus_probe_device(dev);
> > > -       if (parent)
> > > +       if (parent) {
> > > +               dev_info(dev, "add to parent %s\n", dev_name(parent));
> > >                 klist_add_tail(&dev->p->knode_parent,
> > >                                &parent->p->klist_children);
> > > +       }
> > >
> > >         if (dev->class) {
> > >                 mutex_lock(&dev->class->p->mutex);
> > >
> > > Running this with "vbus-supply" (where it crashes) produces:
> > >
> > >      bus: 'platform': probing driver msm_hsusb with device 78d9000.usb
> > >        <some probe deferrals while waiting for the regulator>
> > >      bus: 'platform': probing driver msm_hsusb with device 78d9000.usb
> > >      bus: 'platform': probing driver ci_hdrc with device ci_hdrc.0
> > >      bus: 'ulpi': probing driver qcom_usb_hs_phy with device ci_hdrc.0.ulpi
> > >      phy phy-ci_hdrc.0.ulpi.0: add to parent ci_hdrc.0.ulpi
> > >      qcom_usb_hs_phy ci_hdrc.0.ulpi: add to parent ci_hdrc.0
> > >  (1) msm_hsusb 78d9000.usb: Linked as a consumer to phy-ci_hdrc.0.ulpi.0
> > >  (2) ci_hdrc ci_hdrc.0: add to parent 78d9000.usb
> > >      Kernel panic - not syncing: kernel stack overflow
> > >       ...
> > >
> > > Note how ci_hdrc is added to the children list of 78d9000.usb (2) after
> > > the device link was already created in (1). This is why device_is_dependent()
> > > does not realize the devices will eventually be dependent on each other.
> >
> > Well, it cannot know beforehand that the consumer is going to be
> > registered as a child of the supplier.
> 
> I've got it the other way around, so it should have been:
> 
> "it cannot know beforehand that the supplier is going to be registered
> as a child of the consumer."
> 
> Sorry.
> 
> That said, device_is_dependend() doesn't really check for "family
> connections", so to speak, which it should do in principle.
> 
> That is, it should return "true" if "target" or any direct ancestor of
> it is any of the devices that depend on "dev", not just when "target"
> itself is any of those devices.
> 
> Let me cut a patch for that.

Thanks!

While initially debugging the crash I naively(!) tried the following
two diffs:

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 14f165816742..3864af018834 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3088,10 +3088,10 @@ int device_add(struct device *dev)
 		fw_devlink_link_device(dev);
 	}
 
-	bus_probe_device(dev);
 	if (parent)
 		klist_add_tail(&dev->p->knode_parent,
 			       &parent->p->klist_children);
+	bus_probe_device(dev);
 
 	if (dev->class) {
 		mutex_lock(&dev->class->p->mutex);

or alternatively

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 14f165816742..268b88ce3df7 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -218,12 +218,16 @@ int device_links_read_lock_held(void)
  */
 int device_is_dependent(struct device *dev, void *target)
 {
+	struct device *target_dev = target;
 	struct device_link *link;
 	int ret;
 
 	if (dev == target)
 		return 1;
 
+	if (target_dev->parent && device_is_dependent(dev, target_dev->parent))
+		return 1;
+
 	ret = device_for_each_child(dev, target, device_is_dependent);
 	if (ret)
 		return ret;

Both prevent the circular device link and therefore also the crash.
I didn't mention it because it was really just a naive thought and
I'm sure they might cause other problems. :-)

Also, on a completely different note I looked again at the chipidea USB
driver that produces this situation. To request the PHY (which ends up
in the circular device link) it does:

	/* Look for a generic PHY first */
	ci->phy = devm_phy_get(dev->parent, "usb-phy");

To me it doesn't really seem great to use the devm_* helpers on the
parent device either, so I will check if I can refactor this somehow.
Perhaps this situation can be prevented entirely.

Thanks!
Stephan