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

[Jakub Kicinski]

On Wed, 7 Oct 2020 10:55:23 +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().
> 
> 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 */

Thanks for this graph, very informative!

> 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@xxxxxxx>

You should add Florian's and Vladimir's review tags here, under your
sign-off.

> @@ -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);

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. 

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")

>  }
>  EXPORT_SYMBOL_GPL(ksz_mac_link_down);
>