| Message ID | 20230611104019.33793-1-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: altera_tse: fix init sequence to avoid races with register_netdev | expand |
On Sun, Jun 11, 2023 at 12:40:19PM +0200, Maxime Chevallier wrote: > As reported here[1], the init sequence in altera_tse can be racy should > any operation on the registered netdev happen after netdev registration > but before phylink initialization. > > Fix the registering order to avoid such races by making register_netdev > the last step of the probing sequence. > > [1]: https://lore.kernel.org/netdev/ZH9XK5yEGyoDMIs%2F@shell.armlinux.org.uk/ > > Fixes: fef2998203e1 ("net: altera: tse: convert to phylink") > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > --- > This patch targets net-next as it fixes a commit that is in net-next > too. While it fixes the order in which stuff is registered, it introduces a new bug - register_netdev() is what atomically allocated a netdev name, and you use the netdev name when creating the PCS MII bus: snprintf(mrc.name, MII_BUS_ID_SIZE, "%s-pcs-mii", ndev->name); This needs to change, because this will end up being "eth%d-pcs-mii". I am at a loss why you didn't realise this (or in fact recognise that there are other issues) given that I sent you three patches fixing this mess.
Hi, On Sun, 11 Jun 2023 16:42:26 +0100 "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > On Sun, Jun 11, 2023 at 12:40:19PM +0200, Maxime Chevallier wrote: > > As reported here[1], the init sequence in altera_tse can be racy should > > any operation on the registered netdev happen after netdev registration > > but before phylink initialization. > > > > Fix the registering order to avoid such races by making register_netdev > > the last step of the probing sequence. > > > > [1]: https://lore.kernel.org/netdev/ZH9XK5yEGyoDMIs%2F@shell.armlinux.org.uk/ > > > > Fixes: fef2998203e1 ("net: altera: tse: convert to phylink") > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > > --- > > This patch targets net-next as it fixes a commit that is in net-next > > too. > > While it fixes the order in which stuff is registered, it introduces > a new bug - register_netdev() is what atomically allocated a netdev > name, and you use the netdev name when creating the PCS MII bus: > > snprintf(mrc.name, MII_BUS_ID_SIZE, "%s-pcs-mii", ndev->name); > > This needs to change, because this will end up being "eth%d-pcs-mii". > > I am at a loss why you didn't realise this (or in fact recognise that > there are other issues) given that I sent you three patches fixing > this mess. > Hmpf your patches went un-noticed, sorry... I'll base later work on that. I will also followup with a similar series for dwmac-socfpga Best regards, Maxime
diff --git a/drivers/net/ethernet/altera/altera_tse_main.c b/drivers/net/ethernet/altera/altera_tse_main.c index 2e15800e5310..54f1b5ad84ce 100644 --- a/drivers/net/ethernet/altera/altera_tse_main.c +++ b/drivers/net/ethernet/altera/altera_tse_main.c @@ -1395,12 +1395,6 @@ static int altera_tse_probe(struct platform_device *pdev) spin_lock_init(&priv->rxdma_irq_lock); netif_carrier_off(ndev); - ret = register_netdev(ndev); - if (ret) { - dev_err(&pdev->dev, "failed to register TSE net device\n"); - goto err_register_netdev; - } - platform_set_drvdata(pdev, ndev); priv->revision = ioread32(&priv->mac_dev->megacore_revision); @@ -1449,12 +1443,16 @@ static int altera_tse_probe(struct platform_device *pdev) goto err_init_phylink; } + ret = register_netdev(ndev); + if (ret) { + dev_err(&pdev->dev, "failed to register TSE net device\n"); + return ret; + } + return 0; err_init_phylink: lynx_pcs_destroy(priv->pcs); err_init_pcs: - unregister_netdev(ndev); -err_register_netdev: netif_napi_del(&priv->napi); altera_tse_mdio_destroy(ndev); err_free_netdev:
As reported here[1], the init sequence in altera_tse can be racy should any operation on the registered netdev happen after netdev registration but before phylink initialization. Fix the registering order to avoid such races by making register_netdev the last step of the probing sequence. [1]: https://lore.kernel.org/netdev/ZH9XK5yEGyoDMIs%2F@shell.armlinux.org.uk/ Fixes: fef2998203e1 ("net: altera: tse: convert to phylink") Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> --- This patch targets net-next as it fixes a commit that is in net-next too. drivers/net/ethernet/altera/altera_tse_main.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)