netdev - Re: [PATCH] net: mdio: thunder: Do not unregister buses that have not been registered

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:   Thu, 13 May 2021 13:16:34 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        David Daney <david.daney@...ium.com>
Cc:     andrew@...n.ch, hkallweit1@...il.com, davem@...emloft.net,
        kuba@...nel.org, david.daney@...ium.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] net: mdio: thunder: Do not unregister buses that have
 not been registered

On Thu, May 13, 2021 at 01:51:40PM +0200, Christophe JAILLET wrote:
> In the probe, if 'of_mdiobus_register()' fails, 'nexus->buses[i]' will
> still have a non-NULL value.
> So in the remove function, we will try to unregister a bus that has not
> been registered.
> 
> In order to avoid that NULLify 'nexus->buses[i]'.
> 'oct_mdio_writeq(0,...)' must also be called here.
> 
> Suggested-by: Russell King - ARM Linux admin <linux@...linux.org.uk>
> Fixes: 379d7ac7ca31 ("phy: mdio-thunder: Add driver for Cavium Thunder SoC MDIO buses.")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
> ---
> Calling 'devm_mdiobus_free()' would also be cleaner, IMHO.
> I've not added it because:
>    - it should be fine, even without it
>    - I'm not sure how to use it

devm_mdiobus_free() is a static function not intended to be used by
drivers. There is no devm.*free() function available for this, so
this memory will only ever be freed when either probe fails or the
driver is unbound from its device.

That should be fine, but it would be nice to give that memory back
to the system. Without having a function for drivers to use though,
that's not possible. Such a function should take a struct device
pointer and the struct mii_bus pointer returned by the devm
allocation function.

So, unless Andrew things we really need to free that, what you're
doing below should be fine as far as setting the pointer to NULL.

I think I'd want comments from Cavium on setting the register to
zero - as we don't know how this hardware behaves, and whether that
would have implications we aren't aware of. So, I'm copying in
David Daney (the original driver author) for comment, if his email
address still works!

> ---
>  drivers/net/mdio/mdio-thunder.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mdio/mdio-thunder.c b/drivers/net/mdio/mdio-thunder.c
> index 822d2cdd2f35..140c405d4a41 100644
> --- a/drivers/net/mdio/mdio-thunder.c
> +++ b/drivers/net/mdio/mdio-thunder.c
> @@ -97,8 +97,14 @@ static int thunder_mdiobus_pci_probe(struct pci_dev *pdev,
>  		bus->mii_bus->write = cavium_mdiobus_write;
>  
>  		err = of_mdiobus_register(bus->mii_bus, node);
> -		if (err)
> +		if (err) {
>  			dev_err(&pdev->dev, "of_mdiobus_register failed\n");
> +			/* non-registered buses must not be unregistered in
> +			 * the .remove function
> +			 */
> +			oct_mdio_writeq(0, bus->register_base + SMI_EN);
> +			nexus->buses[i] = NULL;
> +		}
>  
>  		dev_info(&pdev->dev, "Added bus at %llx\n", r.start);
>  		if (i >= ARRAY_SIZE(nexus->buses))
> -- 
> 2.30.2
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!