netdev - Re: [PATCH net-next v4 05/11] net: dsa: realtek: use phy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 17 Jan 2022 23:55:50 -0300
From:   Luiz Angelo Daros de Luca <luizluca@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     "open list:NETWORKING DRIVERS" <netdev@...r.kernel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        Arınç ÜNAL <arinc.unal@...nc9.com>,
        Frank Wunderlich <frank-w@...lic-files.de>
Subject: Re: [PATCH net-next v4 05/11] net: dsa: realtek: use phy_read in ds->ops

> On 1/4/2022 7:15 PM, Luiz Angelo Daros de Luca wrote:
> > The ds->ops->phy_read will only be used if the ds->slave_mii_bus
> > was not initialized. Calling realtek_smi_setup_mdio will create a
> > ds->slave_mii_bus, making ds->ops->phy_read dormant.
> >
> > Using ds->ops->phy_read will allow switches connected through non-SMI
> > interfaces (like mdio) to let ds allocate slave_mii_bus and reuse the
> > same code.
> >
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@...il.com>
> > Tested-by: Arınç ÜNAL <arinc.unal@...nc9.com>
> > Reviewed-by: Linus Walleij <linus.walleij@...aro.org>
>
> Humm assigning dsa_switch_ops::phy_read will force DSA into tearing down
> the MDIO bus in dsa_switch_teardown() instead of letting your driver do
> it and since realtek-smi-core.c uses devm_mdiobus_unregister(), it is
> not clear to me what is going to happen but it sounds like a double free
> might happen?

Thanks, Florian. You should be correct. It might call
mdiobus_unregister() and mdiobus_free() twice, once inside the dsa
code and another one by the devm (if I understood how devm functions
work).

The issue is that the dsa switch is assuming that if slave_mii is
allocated and ds->ops->phy_read is defined, it has allocated the
slave_mii by itself and it should clean up the slave_mii during
teardown.
That assumption came from commit
5135e96a3dd2f4555ae6981c3155a62bcf3227f6 "So I can only guess that no
driver that implements ds->ops->phy_read also allocates and registers
ds->slave_mii_bus itself.". If that is true, the condition during
dsa_switch_setup() is not correct.

During dsa_switch_setup(), if it does not fail, I know that
ds->slave_mii_bus will be allocated, either by ds->ops->setup() or by
itself.

dsa_switch_setup() {
        ....
        ds->ops->setup()
        ....
        if (!ds->slave_mii_bus && ds->ops->phy_read) {
              ...allocate and register ds->slave_mii_bus...
        }
}

During the teardown, ds->slave_mii_bus will always be true (if not
cleaning from an error before it was allocated). So, the test is
really about having ds->ops->phy_read.

dsa_switch_teardown() {
        ...
        if (ds->slave_mii_bus && ds->ops->phy_read) {
             ...unregister and free ds->slave_mii_bus...
        }
        ...
        ds->ops->teardown();
        ...
}

As ds->ops->teardown() is called after slave_mii_bus is gone, there is
no opportunity for ds->ops to clean the mii_slave_bus it might have
allocated.
It does not make sense for me to have those two "if" conditions
working together. It should be either:

dsa_switch_setup() {
        ....
        ds->ops->setup()
        ....
        if (ds->ops->phy_read) {
              if (ds->slave_mii_bus)
                    error("ds->ops->phy_read is set, I should be the
one allocating ds->slave_mii_bus!")
              ...allocate and register ds->slave_mii_bus...
        }
}

if "no driver that implements ds->ops->phy_read also allocates and
registers ds->slave_mii_bus itself" or:

dsa_switch_teardown() {
        ...
        if (ds->slave_mii_bus && "slave_mii_bus was allocated by myself") {
             ...unregister and free ds->slave_mii_bus...
        }
        ds->ops->teardown();
        ...
}

if ds->ops->phy_read value should not tell if ds->slave_mii_bus should
be cleaned by the DSA switch.

I would selfishly hope the correct one was the second option because
it would make my code much cleaner. If not, that's a complex issue to
solve without lots of duplications: realtek-smi drivers should not
have ds->ops->phy_read defined while realtek-mdio requires it. I'll
need to duplicate dsa_switch_ops for each subdriver only to unset
phy_read and also duplicate realtek_variant for each interface only to
reference that different dsa_switch_ops.

BTW, the realtek-smi calls
of_node_put(priv->slave_mii_bus->dev.of_node) during shutdown while
other dsa drivers do not seem to care. Wouldn't devm controls be
enough for cleaning that mii_bus?
Even if not, wouldn't the ds->ops->teardown be the correct place for
that cleanup and not realtek_smi_remove()?

> It seems more prudent to me to leave existing code.

As I mentioned, It would require a good amount of duplications. But
I'll do what needs to be done.

Regards,

Luiz