Re: [PATCH] net: dsa: microchip: fix race condition

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 10/5/2020 9:08 AM, Christian Eggers wrote:
Between queuing the delayed work and finishing the setup of the dsa
ports, the process may sleep in request_module() and the queued work may
be executed prior the initialization of the DSA ports is finished. In
ksz_mib_read_work(), a NULL dereference will happen within
netof_carrier_ok(dp->slave).

Not queuing the delayed work in ksz_init_mib_timer() make things even
worse because the work will now be queued for immediate execution
(instead of 2000 ms) in ksz_mac_link_down() via
dsa_port_link_register_of().

Solution:
1. Do not queue (only initialize) delayed work in ksz_init_mib_timer().
2. Only queue delayed work in ksz_mac_link_down() if init is completed.
3. Queue work once in ksz_switch_register(), after dsa_register_switch()
has completed.

Signed-off-by: Christian Eggers <ceggers@xxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx

This looks fine to me and the analysis appears to be correct, don't you need to pair the test for dev->mib_read_internal being non zero with setting it to zero in ksz_switch_unregister()?

You would also most likely want to add a Fixes: tag such that this can be back ported to stable trees?

---
Call tree:
ksz9477_i2c_probe()
\--ksz9477_switch_register()
    \--ksz_switch_register()
       +--dsa_register_switch()
       |  \--dsa_switch_probe()
       |     \--dsa_tree_setup()
       |        \--dsa_tree_setup_switches()
       |           +--dsa_switch_setup()
       |           |  +--ksz9477_setup()
       |           |  |  \--ksz_init_mib_timer()
       |           |  |     |--/* Start the timer 2 seconds later. */
       |           |  |     \--schedule_delayed_work(&dev->mib_read, msecs_to_jiffies(2000));
       |           |  \--__mdiobus_register()
       |           |     \--mdiobus_scan()
       |           |        \--get_phy_device()
       |           |           +--get_phy_id()
       |           |           \--phy_device_create()
       |           |              |--/* sleeping, ksz_mib_read_work() can be called meanwhile */
       |           |              \--request_module()
       |           |
       |           \--dsa_port_setup()
       |              +--/* Called for non-CPU ports */
       |              +--dsa_slave_create()
       |              |  +--/* Too late, ksz_mib_read_work() may be called beforehand */
       |              |  \--port->slave = ...
       |             ...
       |              +--Called for CPU port */
       |              \--dsa_port_link_register_of()
       |                 \--ksz_mac_link_down()
       |                    +--/* mib_read must be initialized here */
       |                    +--/* work is already scheduled, so it will be executed after 2000 ms */
       |                    \--schedule_delayed_work(&dev->mib_read, 0);
       \-- /* here port->slave is setup properly, scheduling the delayed work should be safe */

static void ksz_mib_read_work()
\--netif_carrier_ok(dp->slave);  dp->slave has not been initialized yet


  drivers/net/dsa/microchip/ksz_common.c | 16 +++++++++-------
  1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 8e755b50c9c1..a94d2278b95c 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -103,14 +103,8 @@ void ksz_init_mib_timer(struct ksz_device *dev)
INIT_DELAYED_WORK(&dev->mib_read, ksz_mib_read_work); - /* Read MIB counters every 30 seconds to avoid overflow. */
-	dev->mib_read_interval = msecs_to_jiffies(30000);
-
  	for (i = 0; i < dev->mib_port_cnt; i++)
  		dev->dev_ops->port_init_cnt(dev, i);
-
-	/* Start the timer 2 seconds later. */
-	schedule_delayed_work(&dev->mib_read, msecs_to_jiffies(2000));
  }
  EXPORT_SYMBOL_GPL(ksz_init_mib_timer);
@@ -143,7 +137,9 @@ void ksz_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode, /* Read all MIB counters when the link is going down. */
  	p->read = true;
-	schedule_delayed_work(&dev->mib_read, 0);
+	/* timer started */
+	if (dev->mib_read_interval)
+		schedule_delayed_work(&dev->mib_read, 0);
  }
  EXPORT_SYMBOL_GPL(ksz_mac_link_down);
@@ -446,6 +442,12 @@ int ksz_switch_register(struct ksz_device *dev,
  		return ret;
  	}
+ /* Read MIB counters every 30 seconds to avoid overflow. */
+	dev->mib_read_interval = msecs_to_jiffies(30000);
+
+	/* Start the MIB timer. */
+	schedule_delayed_work(&dev->mib_read, 0);
+
  	return 0;
  }
  EXPORT_SYMBOL(ksz_switch_register);


--
Florian



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux