On Tue, 30 Nov 2021 01:05:08 +0100 Andrew Lunn wrote:

What i'm missing is some sort of state machine to keep track of the
SFP. Since RTNL is not held other operations could be performed in
parallel. Does CMIS allow this? Can you intermix firmware writes with
reading the temperature sensor for hwmon? Poll the LOS indicator to
see if the link has been lost?

Ah, rtnl_lock is not held throughout? I just looked at this code:

+	rtnl_lock();
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		goto out_rtnl;
+
+	ret = module_flash_fw(dev, tb, info->extack);
+
+	ethnl_ops_complete(dev);
+
+out_rtnl:
+	rtnl_unlock();

and assumed module_flash_fw() flashes the module's FW, not starts 
an async process...

And it appears the user is racy:

+	dev_put(ns->netdev);
+	rtnl_lock();
+	ns->ethtool.module_fw.in_progress = false;
+	rtnl_unlock();
+	release_firmware(ns->ethtool.module_fw.fw);

The dev_put() should be last, otherwise references to ns could be UAF?

With cable testing, phylib already has a state machine, and i added a
new state for cable test running. If any other operation happened
which would cause a change out of this state, like ifdown, or a
request to restart autoneg, the cable test is aborted first.