Message-ID: <4c552594-5923-ba55-af2f-8a0f86936fca@gmail.com>
Date:   Mon, 20 Sep 2021 19:16:15 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Wolfram Sang <wsa@...nel.org>, linux-i2c@...r.kernel.org,
        Mark Brown <broonie@...nel.org>, linux-spi@...r.kernel.org,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        Lino Sanfilippo <LinoSanfilippo@....de>
Subject: Re: [PATCH net 1/2] net: dsa: don't allocate the slave_mii_bus using
 devres



On 9/20/2021 2:42 PM, Vladimir Oltean wrote:
> The Linux device model permits both the ->shutdown and ->remove driver
> methods to get called during a shutdown procedure. Example: a DSA switch
> which sits on an SPI bus, and the SPI bus driver calls this on its
> ->shutdown method:
> 
> spi_unregister_controller
> -> device_for_each_child(&ctlr->dev, NULL, __unregister);
>     -> spi_unregister_device(to_spi_device(dev));
>        -> device_del(&spi->dev);
> 
> So this is a simple pattern which can theoretically appear on any bus,
> although the only other buses on which I've been able to find it are
> I2C:
> 
> i2c_del_adapter
> -> device_for_each_child(&adap->dev, NULL, __unregister_client);
>     -> i2c_unregister_device(client);
>        -> device_unregister(&client->dev);
> 
> The implication of this pattern is that devices on these buses can be
> unregistered after having been shut down. The drivers for these devices
> might choose to return early either from ->remove or ->shutdown if the
> other callback has already run once, and they might choose that the
> ->shutdown method should only perform a subset of the teardown done by
> ->remove (to avoid unnecessary delays when rebooting).
> 
> So in other words, the device driver may choose on ->remove to not
> do anything (therefore to not unregister an MDIO bus it has registered
> on ->probe), because this ->remove is actually triggered by the
> device_shutdown path, and its ->shutdown method has already run and done
> the minimally required cleanup.
> 
> This used to be fine until the blamed commit, but now, the following
> BUG_ON triggers:
> 
> void mdiobus_free(struct mii_bus *bus)
> {
> 	/* For compatibility with error handling in drivers. */
> 	if (bus->state == MDIOBUS_ALLOCATED) {
> 		kfree(bus);
> 		return;
> 	}
> 
> 	BUG_ON(bus->state != MDIOBUS_UNREGISTERED);
> 	bus->state = MDIOBUS_RELEASED;
> 
> 	put_device(&bus->dev);
> }
> 
> In other words, there is an attempt to free an MDIO bus which was not
> unregistered. The attempt to free it comes from the devres release
> callbacks of the SPI device, which are executed after the device is
> unregistered.
> 
> I'm not saying that the fact that MDIO buses allocated using devres
> would automatically get unregistered wasn't strange. I'm just saying
> that the commit didn't care about auditing existing call paths in the
> kernel, and now, the following code sequences are potentially buggy:
> 
> (a) devm_mdiobus_alloc followed by plain mdiobus_register, for a device
>      located on a bus that unregisters its children on shutdown. After
>      the blamed patch, either both the alloc and the register should use
>      devres, or none should.
> 
> (b) devm_mdiobus_alloc followed by plain mdiobus_register, and then no
>      mdiobus_unregister at all in the remove path. After the blamed
>      patch, nobody unregisters the MDIO bus anymore, so this is even more
>      buggy than the previous case which needs a specific bus
>      configuration to be seen, this one is an unconditional bug.
> 
> In this case, DSA falls into category (a), it tries to be helpful and
> registers an MDIO bus on behalf of the switch, which might be on such a
> bus. I've no idea why it does it under devres.
> 
> It does this on probe:
> 
> 	if (!ds->slave_mii_bus && ds->ops->phy_read)
> 		alloc and register mdio bus
> 
> and this on remove:
> 
> 	if (ds->slave_mii_bus && ds->ops->phy_read)
> 		unregister mdio bus
> 
> I _could_ imagine using devres because the condition used on remove is
> different than the condition used on probe. So strictly speaking, DSA
> cannot determine whether the ds->slave_mii_bus it sees on remove is the
> ds->slave_mii_bus that _it_ has allocated on probe. Using devres would
> have solved that problem. But nonetheless, the existing code already
> proceeds to unregister the MDIO bus, even though it might be
> unregistering an MDIO bus it has never registered. So I can only guess
> that no driver that implements ds->ops->phy_read also allocates and
> registers ds->slave_mii_bus itself.
> 
> So in that case, if unregistering is fine, freeing must be fine too.
> 
> Stop using devres and free the MDIO bus manually. This will make devres
> stop attempting to free a still registered MDIO bus on ->shutdown.
> 
> Fixes: ac3a68d56651 ("net: phy: don't abuse devres in devm_mdiobus_register()")
> Reported-by: Lino Sanfilippo <LinoSanfilippo@....de>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>

Reviewed-by: Florian Fainelli <f.fainelli@...il.com>
-- 
Florian

Powered by blists - more mailing lists