Re: [PATCH net-next v20 06/15] net: phylink: make phylink_validate state locked

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] net: phylink: lock order inversion between state_mutex and
  phy->lock in phylink_validate_mask()
--

commit 15371468f04d0bb04d8068bf4c41584df2589e40
Author: Christian Marangi <ansuelsmth@gmail.com>

net: phylink: make phylink_validate state locked

This commit acquires pl->state_mutex in phylink_validate() and
phylink_validate_mask(), adds unlocked __phylink_validate() and
__phylink_validate_mask() helpers, and asserts that pl->state_mutex is held
in the inner validation functions.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 8a0e87a9062f8..d1bd7109c5668 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> @@ -667,14 +674,30 @@ static int phylink_validate_mask(struct phylink *pl, struct phy_device *phy,
>  	return phylink_is_empty_linkmode(supported) ? -EINVAL : 0;
>  }
>  
> -static int phylink_validate(struct phylink *pl, unsigned long *supported,
> -			    struct phylink_link_state *state)
> +static int phylink_validate_mask(struct phylink *pl, struct phy_device *phy,
> +				 unsigned long *supported,
> +				 struct phylink_link_state *state,
> +				 const unsigned long *interfaces)
> +{
> +	int ret;
> +
> +	mutex_lock(&pl->state_mutex);
> +	ret = __phylink_validate_mask(pl, phy, supported, state, interfaces);
> +	mutex_unlock(&pl->state_mutex);
> +
> +	return ret;
> +}

[Severity: High]
Does taking pl->state_mutex in phylink_validate_mask() invert the lock order
against phy->lock?

When phylink_bringup_phy() -> phylink_validate_phy() validates a PHY with
non-empty phy->possible_interfaces, __phylink_validate_mask() ->
phylink_validate_one() calls phy_get_rate_matching() under pl->state_mutex,
which takes phy->lock if the PHY driver implements .get_rate_matching:

drivers/net/phy/phy.c:phy_get_rate_matching() {
	...
	if (phydev->drv->get_rate_matching) {
		mutex_lock(&phydev->lock);
		ret = phydev->drv->get_rate_matching(phydev, iface);
		mutex_unlock(&phydev->lock);
	}
	...
}

Elsewhere in phylink (including later in phylink_bringup_phy(),
phylink_resolve(), and phylink_phy_change()), phy->lock is acquired before
pl->state_mutex, creating an ABBA deadlock and lockdep splat:

CPU0                              CPU1
----                              ----
lock(&pl->state_mutex)
                                  lock(&phy->lock)
lock(&phy->lock)
                                  lock(&pl->state_mutex)

 *** DEADLOCK ***

CPU0: phylink_bringup_phy() -> phylink_validate_phy() ->
      phylink_validate_mask() -> __phylink_validate_mask() ->
      phylink_validate_one() -> phy_get_rate_matching()
CPU1: phy_state_machine() -> phylink_phy_change()

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114104426.235620-1-ansuelsmth@gmail.com?part=6