In the Linux kernel, the following vulnerability has been resolved:

net: phy: transfer phyconfiginband() locking responsibility to phylink

Problem description

Lockdep reports a possible circular locking dependency (AB/BA) between &pl->state_mutex and &phy->lock, as follows.

phylinkresolve() // acquires &pl->statemutex -> phylinkmajorconfig() -> phyconfiginband() // acquires &pl->phydev->lock

whereas all the other call sites where &pl->statemutex and &pl->phydev->lock have the locking scheme reversed. Everywhere else, &pl->phydev->lock is acquired at the top level, and &pl->statemutex at the lower level. A clear example is phylinkbringupphy().

The outlier is the newly introduced phyconfiginband() and the existing lock order is the correct one. To understand why it cannot be the other way around, it is sufficient to consider phylinkphychange(), phylink's callback from the PHY device's phy->phylinkchange() virtual method, invoked by the PHY state machine.

phylinkup() and phylinkdown(), the (indirect) callers of phylinkphychange(), are called with &phydev->lock acquired. Then phylinkphychange() acquires its own &pl->statemutex, to serialize changes made to its pl->phystate and pl->linkconfig. So all other instances of &pl->statemutex and &phydev->lock must be consistent with this order.

Problem impact

I think the kernel runs a serious deadlock risk if an existing phylinkresolve() thread, which results in a phyconfiginband() call, is concurrent with a phylinkup() or phylinkdown() call, which will deadlock on &pl->statemutex in phylinkphychange(). Practically speaking, the impact may be limited by the slow speed of the medium auto-negotiation protocol, which makes it unlikely for the current state to still be unresolved when a new one is detected, but I think the problem is there. Nonetheless, the problem was discovered using lockdep.

Proposed solution

Practically speaking, the phyconfiginband() requirement of having phydev->lock acquired must transfer to the caller (phylink is the only caller). There, it must bubble up until immediately before &pl->state_mutex is acquired, for the cases where that takes place.

Solution details, considerations, notes

This is the phyconfiginband() call graph:

                      sfp_upstream_ops :: connect_phy()
                      |
                      v
                      phylink_sfp_connect_phy()
                      |
                      v
                      phylink_sfp_config_phy()
                      |
                      |   sfp_upstream_ops :: module_insert()
                      |   |
                      |   v
                      |   phylink_sfp_module_insert()
                      |   |
                      |   |   sfp_upstream_ops :: module_start()
                      |   |   |
                      |   |   v
                      |   |   phylink_sfp_module_start()
                      |   |   |
                      |   v   v
                      |   phylink_sfp_config_optical()

phylinkstart() | | | phylinkresume() v v | | phylinksfpsetconfig() | | | v v v phylinkmacinitialconfig() | phylinkresolve() | | phylinkethtoolksettingsset() v v v phylinkmajorconfig() | v phyconfiginband()

phylinkmajorconfig() caller #1, phylinkmacinitialconfig(), does not acquire &pl->statemutex nor do its callers. It must acquire &pl->phydev->lock prior to calling phylinkmajorconfig().

phylinkmajorconfig() caller #2, phylinkresolve() acquires &pl->statemutex, thus also needs to acquire &pl->phydev->lock.

phylinkmajorconfig() caller #3, phylinkethtoolksettings_set(), is completely uninteresting, because it only call ---truncated---