Date:   Tue, 17 Aug 2021 19:46:14 -0700
From:   Saravana Kannan <saravanak@...gle.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Alvin Šipraga <ALSI@...g-olufsen.dk>,
        Vladimir Oltean <vladimir.oltean@....com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Frank Rowand <frowand.list@...il.com>,
        Rob Herring <robh+dt@...nel.org>
Subject: Re: [PATCH net] net: dsa: sja1105: fix use-after-free after calling
 of_find_compatible_node, or worse

On Tue, Aug 17, 2021 at 3:31 PM Vladimir Oltean <olteanv@...il.com> wrote:
>
> Hi Alvin,
>
> On Tue, Aug 17, 2021 at 09:25:28PM +0000, Alvin Šipraga wrote:
> > I have an observation that's slightly out of the scope of your patch,
> > but I'll post here on the off chance that you find it relevant.
> > Apologies if it's out of place.
> >
> > Do these integrated NXP PHYs use a specific PHY driver, or do they just
> > use the Generic PHY driver?
>
> They refuse to probe at all with the Generic PHY driver. I have been
> caught off guard a few times now when I had a kernel built with
> CONFIG_NXP_C45_TJA11XX_PHY=n and their probing returns -22 in that case.
>
> > If the former is the case, do you experience that the PHY driver fails
> > to get probed during mdiobus registration if the kernel uses
> > fw_devlink=on?
>
> I don't test with "fw_devlink=on" in /proc/cmdline, this is the first
> time I do it. It behaves exactly as you say.
>
> >
> > In my case I am writing a new subdriver for realtek-smi, a DSA driver
> > which registers an internal MDIO bus analogously to sja1105, which is
> > why I'm asking. I noticed a deferred probe of the PHY driver because the
> > supplier (ethernet-switch) is not ready - presumably because all of this
> > is happening in the probe of the switch driver. See below:
> >
> > [   83.653213] device_add:3270: device: 'SMI-0': device_add
> > [   83.653905] device_pm_add:136: PM: Adding info for No Bus:SMI-0
> > [   83.654055] device_add:3270: device: 'platform:ethernet-switch--mdio_bus:SMI-0': device_add
> > [   83.654224] device_link_add:843: mdio_bus SMI-0: Linked as a sync state only consumer to ethernet-switch
> > [   83.654291] libphy: SMI slave MII: probed
> > ...
> > [   83.659809] device_add:3270: device: 'SMI-0:00': device_add
> > [   83.659883] bus_add_device:447: bus: 'mdio_bus': add device SMI-0:00
> > [   83.659970] device_pm_add:136: PM: Adding info for mdio_bus:SMI-0:00
> > [   83.660122] device_add:3270: device: 'platform:ethernet-switch--mdio_bus:SMI-0:00': device_add
> > [   83.660274] devices_kset_move_last:2701: devices_kset: Moving SMI-0:00 to end of list
> > [   83.660282] device_pm_move_last:203: PM: Moving mdio_bus:SMI-0:00 to end of list
> > [   83.660293] device_link_add:859: mdio_bus SMI-0:00: Linked as a consumer to ethernet-switch
> > [   83.660350] __driver_probe_device:736: bus: 'mdio_bus': __driver_probe_device: matched device SMI-0:00 with driver RTL8365MB-VC Gigabit Ethernet
> > [   83.660365] device_links_check_suppliers:1001: mdio_bus SMI-0:00: probe deferral - supplier ethernet-switch not ready
> > [   83.660376] driver_deferred_probe_add:138: mdio_bus SMI-0:00: Added to deferred list
>
> So it's a circular dependency? Switch cannot finish probing because it
> cannot connect to PHY, which cannot probe because switch has not
> finished probing, which....

Hi Vladimir/Alvin,

If there's a cyclic dependency between two devices, then fw_devlink=on
is smart enough to notice that. Once it notices a cycle, it knows that
it can't tell which one is the real dependency and which one is the
false dependency and so stops enforcing ordering between the devices
in the cycle.

But fw_devlink doesn't understand all the properties yet. Just most of
them and I'm always trying to add more. So when it only understands
the property that's causing the false dependency but not the property
that causes the real dependency, it can cause issues like this where
fw_devlink=on enforces the false dependency and the driver/code
enforces the real dependency. These are generally easy to fix -- you
just need to teach fw_devlink how to parse more properties.

This is just a preliminary analysis since I don't have all the info
yet -- so I could be wrong. With that said, I happened to be working
on adding fw_devlink support for phy-handle property and I think it
should fix your issue with fw_devlink=on. Can you give [1] a shot?

If it doesn't fix it, can one of you please point me to an upstream
dts (not dtsi) file for a platform in which you see this issue? And
ideally also the DT nodes and their drivers that are involved in this
cycle? With that info, I should be able to root cause this if the
patch above doesn't already fix it.

>
> So how is it supposed to be solved then? Intuitively the 'mdio_bus SMI-0:00'
> device should not be added to the deferred list, it should have everything
> it needs right now (after all, it works without fw_devlink). No?
>
> It might be the late hour over here too, but right now I just don't
> know. Let me add Saravana to the discussion too, he made an impressive
> analysis recently on a PHY probing issue with mdio-mux,

Lol, thanks for the kind words.

> so the PHY
> library probing dependencies should still be fresh in his mind, maybe he
> has an idea what's wrong.

[1] - https://lore.kernel.org/lkml/20210818021717.3268255-1-saravanak@google.com/T/#u

Thanks,
Saravana

Powered by blists - more mailing lists