Date:   Thu, 19 Aug 2021 16:52:43 -0700
From:   Saravana Kannan <saravanak@...gle.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Alvin Šipraga <ALSI@...g-olufsen.dk>,
        Vladimir Oltean <olteanv@...il.com>,
        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>,
        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 Thu, Aug 19, 2021 at 6:35 AM Andrew Lunn <andrew@...n.ch> wrote:
>
> > (2) is what is happening in this case. fw_devlink=on sees that
> > "switch" implements the "switch_intc" and "switch" hasn't finished
> > probing yet. So it has no way of knowing that switch_intc is actually
> > ready. And even if switch_intc was registered as part of switch's
> > probe() by the time the PHYs are added, switch_intc could get
> > deregistered if the probe fails at a later point. So until probe()
> > returns 0, fw_devlink can't be fully sure the supplier (switch_intc)
> > is ready. Which is good in general because you won't have to
> > forcefully unbind (if that is even handled correctly in the first
> > place) the consumers of a device if it fails probe() half way through
> > registering a few services.

I had to read your email a couple of times before I understood it. I
think I do now, but apologies if I'm not making sense.

>
> There are actually a few different circular references with the way
> switches work. Take for example:
>
> &fec1 {
>         phy-mode = "rmii";
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_fec1>;
>         status = "okay";
>
>         fixed-link {
>                 speed = <100>;
>                 full-duplex;
>         };
>
>         mdio1: mdio {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>                 clock-frequency = <12500000>;
>                 suppress-preamble;
>                 status = "okay";
>
>                 switch0: switch0@0 {
>                         compatible = "marvell,mv88e6190";
>                         pinctrl-0 = <&pinctrl_gpio_switch0>;
>                         pinctrl-names = "default";
>                         reg = <0>;
>                         eeprom-length = <65536>;
>                         interrupt-parent = <&gpio3>;
>                         interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
>                         interrupt-controller;
>                         #interrupt-cells = <2>;
>
>                         ports {
>                                 #address-cells = <1>;
>                                 #size-cells = <0>;
>
>                                 port@0 {
>                                         reg = <0>;
>                                         label = "cpu";
>                                         ethernet = <&fec1>;
>
>                                         fixed-link {
>                                                 speed = <100>;
>                                                 full-duplex;
>                                         };
>                                 };
>
> FEC is an ethernet controller. It has an MDIO bus, and on the bus is
> an Ethernet switch. port 0 of the Ethernet switch is connected to the
> FEC ethernet controller.
>
> While the FEC probes, it will at some point register its MDIO bus. At
> that point, the MDIO bus is probed, the switch is found, and
> registered with the switch core. The switch core looks for the port
> with an ethernet property and goes looking for that ethernet
> interface. But that this point in time, the FEC probe has only got as
> far as registering the MDIO bus. The interface itself is not
> registered. So finding the interface fails, and we go into
> EPROBE_DEFER for probing the switch.

Ok, I understood up to here. Couple of questions:
Is this EPROBE_DEFER causing an issue? Wouldn't the switch then
probe successfully when it's reattempted? And then things work
normally? I don't see what the problem is.

> It is pretty hard to solve. An Ethernet interface can be used by the
> kernel itself, e.g. NFS root. At the point you call register_netdev()
> in the probe function, to register the interface with the core,

Are you using "ethernet interface" and "ethernet controller"
interchangeably? Looking at some other drivers, it looks like the
ethernet controlled (FEC) is what would call register_netdev(). So
what's wrong with that happening if switch0 has not probed
successfully?

> it
> needs to be fully ready to go.  The networking stack can start using
> the interface before register_netdev() even returns. So you cannot
> first register the interface and then register the MDIO bus.
>
> I once looked to see if it was possible to tell the driver core to not
> even bother probing a bus as soon as it is registered, go straight to
> defer probe handling. Because this is one case we know it cannot
> work. But it does not seem possible.

fw_devlink doesn't understand the "ethernet" property. If I add that,
then in the example you state above, switch0's probe won't even be
called until the FEC probe returns. The change is pretty trivial
(pasted below) -- can you try it out and tell me if it does what you
need/want?

-Saravana

+++ b/drivers/of/property.c
@@ -1292,6 +1292,7 @@ DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
 DEFINE_SIMPLE_PROP(leds, "leds", NULL)
 DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
 DEFINE_SIMPLE_PROP(phy_handle, "phy-handle", NULL)
+DEFINE_SIMPLE_PROP(ethernet, "ethernet", NULL)
 DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
 DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")

@@ -1381,6 +1382,7 @@ static const struct supplier_bindings
of_supplier_bindings[] = {
        { .parse_prop = parse_leds, },
        { .parse_prop = parse_backlight, },
        { .parse_prop = parse_phy_handle, },
+       { .parse_prop = parse_ethernet, },
        { .parse_prop = parse_gpio_compat, },
        { .parse_prop = parse_interrupts, },
        { .parse_prop = parse_regulators, },

Powered by blists - more mailing lists