| Message ID | 20201012083942.12722-1-ceggers@arri.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v4] net: dsa: microchip: fix race condition | expand |
On Mon, 12 Oct 2020 10:39:42 +0200 Christian Eggers wrote: > Between queuing the delayed work and finishing the setup of the dsa > ports, the process may sleep in request_module() (via > phy_device_create()) and the queued work may be executed prior to the > switch net devices being registered. 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() makes 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. > > Fixes: 7c6ff470aa86 ("net: dsa: microchip: add MIB counter reading support") > Signed-off-by: Christian Eggers <ceggers@arri.de> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > Reviewed-by: Vladimir Oltean <olteanv@gmail.com> > Reviewed-by: Jakub Kicinski <kuba@kernel.org> Now you went too far in the opposite direction, I never gave you my explicit tag :) So I'll drop it. Applied and queued for stable, thanks!
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index 01f5784f69cb..0ef854911f21 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); @@ -451,6 +447,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);
Between queuing the delayed work and finishing the setup of the dsa ports, the process may sleep in request_module() (via phy_device_create()) and the queued work may be executed prior to the switch net devices being registered. 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() makes 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(). 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 */ 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. Fixes: 7c6ff470aa86 ("net: dsa: microchip: add MIB counter reading support") Signed-off-by: Christian Eggers <ceggers@arri.de> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> Reviewed-by: Vladimir Oltean <olteanv@gmail.com> Reviewed-by: Jakub Kicinski <kuba@kernel.org> --- On Friday, 9 October 2020, 22:04:42 CEST, Jakub Kicinski wrote: > Your patch seems fine, but I wonder what was the original author trying > to achieve with this schedule_delayed_work(..., 0) call? > > The work is supposed to be scheduled at this point, right? > In that case another call to schedule_delayed_work() is > simply ignored. I came to the same conclusion... > Judging by the comment it seems like someone was under the impression > this will reschedule the work to be run immediately, which is not the > case. > > In fact looks like a separate bug introduced in: > > 469b390e1ba3 ("net: dsa: microchip: use delayed_work instead of timer + work") Seems so. Before that change, the "work" was only queued after timer expiry, so it could be scheduled for "soon" execution when the link is going down. After that change, the work is always queued as "delayed work", so scheduling when the the link goes down has no effect. @George: Can you write a fix for this? v4: --------- - Added Reviewed-by: tags from Florian, Vladimir and Jakub v3: --------- - Use 12 digts for commit id in "Fixes:" tag v2: --------- - no changes in the patch itself - use correct subject-prefix - changed wording of commit description - added call tree to commit description - added "Fixes:" tag drivers/net/dsa/microchip/ksz_common.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)