| Message ID | 20230717152709.574773-11-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introduce ndo_hwtstamp_get() and ndo_hwtstamp_set() | expand |
On Mon, Jul 17, 2023 at 06:27:07PM +0300, Vladimir Oltean wrote: > phy_init() and phy_exit() will have to do more stuff under rtnl_lock() > in a future change. Since rtnl_unlock() -> netdev_run_todo() does a lot > of stuff under the hood, it's a pity to lock and unlock the rtnetlink > mutex twice in a row. > > Change the calling convention such that the only caller of > ethtool_set_ethtool_phy_ops(), phy_device.c, provides a context where > the rtnl_mutex is already acquired. > > Note that phy_exit() wasn't performing the opposite teardown of > phy_init(). Reverse mdio_bus_init() with ethtool_set_ethtool_phy_ops(), > so that this is now the case. To me, this looks buggy. > @@ -3451,11 +3452,14 @@ static int __init phy_init(void) > { > int rc; > > + rtnl_lock(); > + ethtool_set_ethtool_phy_ops(&phy_ethtool_phy_ops); > + rtnl_unlock(); > + > rc = mdio_bus_init(); > if (rc) > return rc; If mdio_bus_init() fails, and phylib is built as a module, then we leave ethtool_phy_ops pointing into module space that has potentially been freed or re-used for another module. This error path needs to properly clean up. The same is also true for the other failure paths in phy_init() which already do not cater for their failures leaving a dangling pointer in ethtool_phy_ops. This should probably be fixed first in a separate patch for the net tree. Thanks.
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 0c2014accba7..ab53d10f1844 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -30,6 +30,7 @@ #include <linux/phy_led_triggers.h> #include <linux/pse-pd/pse.h> #include <linux/property.h> +#include <linux/rtnetlink.h> #include <linux/sfp.h> #include <linux/skbuff.h> #include <linux/slab.h> @@ -3451,11 +3452,14 @@ static int __init phy_init(void) { int rc; + rtnl_lock(); + ethtool_set_ethtool_phy_ops(&phy_ethtool_phy_ops); + rtnl_unlock(); + rc = mdio_bus_init(); if (rc) return rc; - ethtool_set_ethtool_phy_ops(&phy_ethtool_phy_ops); features_init(); rc = phy_driver_register(&genphy_c45_driver, THIS_MODULE); @@ -3477,7 +3481,9 @@ static void __exit phy_exit(void) phy_driver_unregister(&genphy_c45_driver); phy_driver_unregister(&genphy_driver); mdio_bus_exit(); + rtnl_lock(); ethtool_set_ethtool_phy_ops(NULL); + rtnl_unlock(); } subsys_initcall(phy_init); diff --git a/net/ethtool/common.c b/net/ethtool/common.c index 5fb19050991e..f5598c5f50de 100644 --- a/net/ethtool/common.c +++ b/net/ethtool/common.c @@ -665,9 +665,8 @@ const struct ethtool_phy_ops *ethtool_phy_ops; void ethtool_set_ethtool_phy_ops(const struct ethtool_phy_ops *ops) { - rtnl_lock(); + ASSERT_RTNL(); ethtool_phy_ops = ops; - rtnl_unlock(); } EXPORT_SYMBOL_GPL(ethtool_set_ethtool_phy_ops);
phy_init() and phy_exit() will have to do more stuff under rtnl_lock() in a future change. Since rtnl_unlock() -> netdev_run_todo() does a lot of stuff under the hood, it's a pity to lock and unlock the rtnetlink mutex twice in a row. Change the calling convention such that the only caller of ethtool_set_ethtool_phy_ops(), phy_device.c, provides a context where the rtnl_mutex is already acquired. Note that phy_exit() wasn't performing the opposite teardown of phy_init(). Reverse mdio_bus_init() with ethtool_set_ethtool_phy_ops(), so that this is now the case. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- Changes in v8: - Patch is new drivers/net/phy/phy_device.c | 8 +++++++- net/ethtool/common.c | 3 +-- 2 files changed, 8 insertions(+), 3 deletions(-)