Message-ID: <CAGETcx8_vxxPxF8WrXqk=PZYfEggsozP+z9KyOu5C2bEW0VW8g@mail.gmail.com>
Date:   Wed, 25 Aug 2021 22:33:01 -0700
From:   Saravana Kannan <saravanak@...gle.com>
To:     Alvin Šipraga <ALSI@...g-olufsen.dk>
Cc:     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>,
        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 Wed, Aug 25, 2021 at 6:40 AM Alvin Šipraga <ALSI@...g-olufsen.dk> wrote:
>
> Hi Saravana,
>
> Sorry for the delayed response.
>
> On 8/23/21 8:50 PM, Saravana Kannan wrote:
> > On Sun, Aug 22, 2021 at 7:19 AM Alvin Šipraga <ALSI@...g-olufsen.dk> wrote:
> >>
> >> Hi Saravana,
> >>
> >> Thanks for the follow-up. I tested your change and it does the trick:
> >> there is no deferral and the PHY driver gets probed first-try during the
> >> mdiobus registration during the call to dsa_register_switch().
> >
> > I'm fairly certain the mdiobus registration happens before
> > dsa_register_switch(). It's in the probe call path of the DSA. The
> > connecting of the PHYs with the DSA is what happens when
> > dsa_register_switch() is called.
>
> Hm, are you sure about this? My understanding is that mdiobus
> registration happens as follows:
>
> dsa_register_switch()
>      -> ...
>          -> rtl836{6,5mb}_setup() # see [1] for RFC patch with rtl8365mb
>              -> realtek_smi_setup_mdio()
>                  -> of_mdiobus_register()

My bad, you are definitely right! Thanks for correcting my understanding.

> As it stands, dsa_register_switch() is currently called from
> realtek_smi_probe(). Your patch just moves this call to
> realtek_smi_sync_state(), but per the above, the mdiobus registration
> happens inside dsa_register_switch(), meaning it doesn't happen in the
> probe call path. Or am I missing something? I'm happy to be wrong :-)

Ok, my sync_state() hack is definitely not a solution anymore. It's
just a terrible hack.

>
> [1] https://lore.kernel.org/netdev/20210822193145.1312668-1-alvin@pqrs.dk/T/
>
> >
> >> I tested
> >> with the switch, PHY, and tagging drivers all builtin, or all modules,
> >> and it worked in both cases.
> >>
> >> On 8/20/21 6:52 PM, Saravana Kannan wrote:
> >>> Hi Alvin,
> >>>
> >>> Can you give this a shot to see if it fixes your issue? It basically
> >>> delays the registration of dsa_register_switch() until all the
> >>> consumers of this switch have probed. So it has a couple of caveats:
> >>
> >> Hm, weren't the only consumers the PHYs themselves? It seems like the
> >> main effect of your change is that - by doing the actual
> >> dsa_register_switch() call after the switch driver probe - the
> >> ethernet-switch (provider) is already probed, thereby allowing the PHY
> >> (consumer) to probe immediately.
> >
> > Correct-ish -- if you modify this to account for what I said above.
> >
> >>
> >>> 1. I'm hoping the PHYs are the only consumers of this switch.
> >>
> >> In my case that is true, if you count the mdio_bus as well:
> >>
> >> /sys/devices/platform/ethernet-switch# ls -l consumer\:*
> >> lrwxrwxrwx    1 root     root             0 Aug 22 16:00
> >> consumer:mdio_bus:SMI-0 ->
> >> ../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0
> >> lrwxrwxrwx    1 root     root             0 Aug 22 16:00
> >> consumer:mdio_bus:SMI-0:00 ->
> >> ../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0:00
> >> lrwxrwxrwx    1 root     root             0 Aug 22 16:00
> >> consumer:mdio_bus:SMI-0:01 ->
> >> ../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0:01
> >> lrwxrwxrwx    1 root     root             0 Aug 22 16:00
> >> consumer:mdio_bus:SMI-0:02 ->
> >> ../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0:02
> >> lrwxrwxrwx    1 root     root             0 Aug 22 16:00
> >> consumer:mdio_bus:SMI-0:03 ->
> >> ../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0:03
> >
> > Hmm... mdio_bus being a consumer should prevent the sync_state() from
> > being called on "ethernet-switch". What's the value of the "status"
> > and "sync_state_only" files inside that mdio_bus folder?
>
> Without your patch:
>
> /sys/devices/platform/ethernet-switch# ls -l consumer\:*
> lrwxrwxrwx    1 root     root             0 Aug 25 13:42
> consumer:mdio_bus:SMI-0 ->
> ../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0
> /sys/devices/platform/ethernet-switch# cat consumer\:*/status
> available
> /sys/devices/platform/ethernet-switch# cat consumer\:*/sync_state_only
> 1
>
>
> With your patch:
>
> 0.0.0.0@...4-:/sys/devices/platform/ethernet-switch# ls -l consumer\:*
> lrwxrwxrwx    1 root     root             0 Aug 25 15:03
> consumer:mdio_bus:SMI-0 ->
> ../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0
> lrwxrwxrwx    1 root     root             0 Aug 25 15:03
> consumer:mdio_bus:SMI-0:00 ->
> ../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0:00
> lrwxrwxrwx    1 root     root             0 Aug 25 15:03
> consumer:mdio_bus:SMI-0:01 ->
> ../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0:01
> lrwxrwxrwx    1 root     root             0 Aug 25 15:03
> consumer:mdio_bus:SMI-0:02 ->
> ../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0:02
> lrwxrwxrwx    1 root     root             0 Aug 25 15:03
> consumer:mdio_bus:SMI-0:03 ->
> ../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0:03
> 0.0.0.0@...4-:/sys/devices/platform/ethernet-switch#
> 0.0.0.0@...4-:/sys/devices/platform/ethernet-switch#
> 0.0.0.0@...4-:/sys/devices/platform/ethernet-switch# cat
> consumer\:*/status
> available
> active
> active
> active
> active
> 0.0.0.0@...4-:/sys/devices/platform/ethernet-switch# cat
> consumer\:*/sync_state_only
> 1
> 0
> 0
> 0
> 0
>
> Hope that helps you understand what's going on better.
>
> BTW, I noticed that when I build realtek-smi as a module with your
> patch, my kernel crashes if I unload the module. I haven't debugged this
> because it was just a test, nor did I get a stacktrace. LMK if you want
> more info.

Yeah, don't bother. It should never merge.

> >
> >>> 2. All of them have to probe successfully before the switch will
> >>> register itself.
> >>
> >> Yes.
> >
> > Right, it's a yes in your case. But will it be a yes for all instances
> > of "realtek,rtl8366rb"?
> >
> >>> 3. If dsa_register_switch() fails, we can't defer the probe (because
> >>> it already succeeded). But I'm not sure if it's a likely error code.
> >>
> >> It's of course possible that dsa_register_switch() fails. Assuming
> >> fw_devlink is doing its job properly, I think the reason is most likely
> >> going to be something specific to the driver, such as a communication
> >> timeout with the switch hardware itself.
> >
> > But what if someone sets fw_devlink=permissive? Is it okay to break
> > this? There are ways to make this work for fw_devlink=permissive and
> > =on -- you check for each and decide where to call
> > dsa_register_switch() based on that.
>
> I am new to DSA myself so I think I am unqualified to answer whether
> it's OK to break things or not.
>
> >
> >> I get the impression that you don't necessarily regard this change as a
> >> proper fix, so I'm happy to do further tests if you choose to
> >> investigate further.
> >
> > I thought about this in the background the past few days. I think
> > there are a couple of options:
> > 1. We (community/Andrew) agree that this driver would only work with
> > fw_devlink=on and we can confirm that the other upstream uses of
> > "realtek,rtl8366rb" won't have any unprobed consumers problem and
> > switch to using my patch. Benefit is that it's a trivial and quick
> > change that gets things working again.
> > 2. The "realtek,rtl8366rb" driver needs to be fixed to use a
> > "component device". A component device is a logical device that
> > represents a group of other devices. It's only initialized after all
> > these devices have probed successfully. The actual switch should be a
> > component device and it should call dsa_register_switch() in it's
> > "bind" (equivalent of probe). That way you can explicitly control what
> > devices need to be probed instead of depending on sync_state() that
> > have a bunch of caveats.
> >
> > Alvin, do you want to take up (2)?
>
> I can give it a shot, but first:
>
>    - It seems Andrew may also need some convincing that this is the
> right approach.

Agreed. Let's wait to see what Andrew thinks of my last response to him.

>    - Are you sure that this will solve the problem? See what I wrote
> upstairs in this email.

Yeah, it would solve the problem with a few changes:
1. The IRQ registration and mdio bus registration would get moved to
realtek_smi_probe() which probes "realtek,rtl8366rb".
2. The component device needs to be set up to be "made up of"
realtek,rtl8366rb and all the PHYs. So it'll wait for all of them to
finish probing before it's initialized. PHYs will initialize now
because realtek,rtl8366rb probe would finish without problems.
3. The component device init would call dsa_register_switch() which
kinda makes sense because the rtl8366rb and all the PHYs combined
together is what makes up the logical DSA switch.

If (2) and (3) could be make part of the framework itself, with (1)
like fix ups done to drivers with have these cyclic dependency issues,
then we could fix bad driver usage model (assuming device_add() will
probe the device before it returns).

>    - I have never written - nor can I recall reading - a component
> driver, so I would appreciate if you could point me to an upstream
> example that you think is illustrative.

The APIs you'll end up using are those in drivers/base/component.c.
You can also grep for component_master_ops and component_ops.
drivers/gpu/drm/msm/msm_drv.c might be one place to start.

Btw, the component API itself could afford some clean up and there was
a series [1], but it looks like Stephen has been busy and hasn't
gotten around to it?
[1] - https://lore.kernel.org/lkml/20210520002519.3538432-1-swboyd@chromium.org/

Having said all that, I might be able to give an API to tell
fw_devlink to ignore a specific child node as a supplier. So you'd
call it to ask fw_devlink to ignore the interrupt controller as a
supplier. I still maintain the current model of this driver is
definitely broken, but I'd rather just unblock this right now that
revert phy-handle support because of a few whacky corner cases like
this. I'll try to send those out this week.

> There's one more issue: I do not have an RTL8366 to test on - rather, I
> encountered this problem in realtek-smi while writing a new subdriver
> for it to support the RTL8365MB. So any proposed fix may be perceived as
> speculative if I cannot test on the '66 hardware.

If you know it fixes the issue on a very similar downstream
hardware/driver, I think it's okay to send fixes. And someone else
might be able to test it out for you.

> In that case this may
> have to wait until the '65MB subdriver is accepted. Many ifs and maybes,
> but don't take it to mean I'm not interested in helping. On the contrary
> - I would like to fix this bug since I am affected by it!

Thanks.

-Saravana

Powered by blists - more mailing lists