[PATCH net] net: phy: Fix deadlocking in phy_error() invocation

[PATCH net] net: phy: Fix deadlocking in phy_error() invocation

[Serge Semin]

Since commit 91a7cda1f4b8 ("net: phy: Fix race condition on link status
change") all the phy_error() method invocations have been causing the
nested-mutex-lock deadlock because it's normally done in the PHY-driver
threaded IRQ handlers which since that change have been called with the
phydev->lock mutex held. Here is the calls thread:

IRQ: phy_interrupt()
     +-> mutex_lock(&phydev->lock); <--------------------+
         drv->handle_interrupt()                         | Deadlock due
         +-> ERROR: phy_error()                          + to the nested
                    +-> phy_process_error()              | mutex lock
                        +-> mutex_lock(&phydev->lock); <-+
                            phydev->state = PHY_ERROR;
                            mutex_unlock(&phydev->lock);
         mutex_unlock(&phydev->lock);

The problem can be easily reproduced just by calling phy_error() from any
PHY-device threaded interrupt handler. Fix it by dropping the phydev->lock
mutex lock from the phy_process_error() method and printing a nasty error
message to the system log if the mutex isn't held in the caller execution
context.

Note for the fix to work correctly in the PHY-subsystem itself the
phydev->lock mutex locking must be added to the phy_error_precise()
function.

Link: https://lore.kernel.org/netdev/20230816180944.19262-1-fancer.lancer@gmail.com
Fixes: 91a7cda1f4b8 ("net: phy: Fix race condition on link status change")
Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/net/phy/phy.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index bdf00b2b2c1d..a9ecfdd19624 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1184,9 +1184,11 @@ void phy_stop_machine(struct phy_device *phydev)
 
 static void phy_process_error(struct phy_device *phydev)
 {
-	mutex_lock(&phydev->lock);
+	/* phydev->lock must be held for the state change to be safe */
+	if (!mutex_is_locked(&phydev->lock))
+		phydev_err(phydev, "PHY-device data unsafe context\n");
+
 	phydev->state = PHY_ERROR;
-	mutex_unlock(&phydev->lock);
 
 	phy_trigger_machine(phydev);
 }
@@ -1195,7 +1197,9 @@ static void phy_error_precise(struct phy_device *phydev,
 			      const void *func, int err)
 {
 	WARN(1, "%pS: returned: %d\n", func, err);
+	mutex_lock(&phydev->lock);
 	phy_process_error(phydev);
+	mutex_unlock(&phydev->lock);
 }
 
 /**
@@ -1204,8 +1208,7 @@ static void phy_error_precise(struct phy_device *phydev,
  *
  * Moves the PHY to the ERROR state in response to a read
  * or write error, and tells the controller the link is down.
- * Must not be called from interrupt context, or while the
- * phydev->lock is held.
+ * Must be called with phydev->lock held.
  */
 void phy_error(struct phy_device *phydev)
 {
-- 
2.41.0

Re: [PATCH net] net: phy: Fix deadlocking in phy_error() invocation

[Andrew Lunn]

On Fri, Aug 18, 2023 at 03:54:45PM +0300, Serge Semin wrote:
>  static void phy_process_error(struct phy_device *phydev)
>  {
> -	mutex_lock(&phydev->lock);
> +	/* phydev->lock must be held for the state change to be safe */
> +	if (!mutex_is_locked(&phydev->lock))
> +		phydev_err(phydev, "PHY-device data unsafe context\n");
> +
>  	phydev->state = PHY_ERROR;
> -	mutex_unlock(&phydev->lock);
>  
>  	phy_trigger_machine(phydev);
>  }

Thanks for the patch Serge. It looks like a good implementation of
what i suggested. But thinking about it further, if the error ever
appears in somebodies kernel log, there is probably not enough
information to actually fix it. There is no call path. So i think it
should actually use WARN_ON_ONCE() so we get a stack trace.

Sorry for changing my mind.

    Andrew

---
pw-bot: cr

Re: [PATCH net] net: phy: Fix deadlocking in phy_error() invocation

[Serge Semin]

On Fri, Aug 18, 2023 at 03:07:49PM +0200, Andrew Lunn wrote:
> On Fri, Aug 18, 2023 at 03:54:45PM +0300, Serge Semin wrote:
> >  static void phy_process_error(struct phy_device *phydev)
> >  {
> > -	mutex_lock(&phydev->lock);
> > +	/* phydev->lock must be held for the state change to be safe */
> > +	if (!mutex_is_locked(&phydev->lock))
> > +		phydev_err(phydev, "PHY-device data unsafe context\n");
> > +
> >  	phydev->state = PHY_ERROR;
> > -	mutex_unlock(&phydev->lock);
> >  
> >  	phy_trigger_machine(phydev);
> >  }
> 
> Thanks for the patch Serge. It looks like a good implementation of
> what i suggested. But thinking about it further, if the error ever
> appears in somebodies kernel log, there is probably not enough
> information to actually fix it. There is no call path. So i think it
> should actually use WARN_ON_ONCE() so we get a stack trace.

A trace is already printed by means of WARN()/WARN_ON()
in the phy_process_error() method callers:
phy_error_precise()
and
phy_error()
Wouldn't it be too much to print it twice in a row?

We can redefine phy_error_precise() and phy_process_error() functions
to something like this:

static void phy_process_error(struct phy_device *phydev,
			      const void *func, int err)
{
	if (__ONCE_LITE_IF(!mutex_is_locked(&phydev->lock)))
		WARN(1, "PHY-device data unsafe context\n");
	else if (func)
		WARN(1, "%pS: returned: %d\n", func, err);
	else
		WARN_ON(1);

	phydev->state = PHY_ERROR;

	phy_trigger_machine(phydev);
}

static void phy_error_precise(struct phy_device *phydev,
			      const void *func, int err)
{
        mutex_lock(&phydev->lock);
        phy_process_error(phydev, func, err);
        mutex_unlock(&phydev->lock);
}

void phy_error(struct phy_device *phydev)
{
	phy_process_error(phydev, NULL, 0);
}
EXPORT_SYMBOL(phy_error);

Though in such implementation phy_error_precise() looks redundant. We
can freely move its body to the single user - phy_state_machine()
function.

Note a positive side effect of this implementation is that potentially
phy_error() can be converted to accepting a function pointer caused
the error (phy_read(), phy_write(), etc). Alternatively if the
conversion would look too bulky, phy_error_preciseI() could be just
EXPORT_SYMBOL()-ed with the PHY-device mutex locking being moved to
phy_state_machine().

> 
> Sorry for changing my mind.

No worries.

-Serge(y)

> 
>     Andrew
> 
> ---
> pw-bot: cr

Re: [PATCH net] net: phy: Fix deadlocking in phy_error() invocation

[Andrew Lunn]

> A trace is already printed by means of WARN()/WARN_ON()
> in the phy_process_error() method callers:
> phy_error_precise()
> and
> phy_error()
> Wouldn't it be too much to print it twice in a row?

Ah, good point. I missed that.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

pw-bot: new

Re: [PATCH net] net: phy: Fix deadlocking in phy_error() invocation

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Fri, 18 Aug 2023 15:54:45 +0300 you wrote:
> Since commit 91a7cda1f4b8 ("net: phy: Fix race condition on link status
> change") all the phy_error() method invocations have been causing the
> nested-mutex-lock deadlock because it's normally done in the PHY-driver
> threaded IRQ handlers which since that change have been called with the
> phydev->lock mutex held. Here is the calls thread:
> 
> IRQ: phy_interrupt()
>      +-> mutex_lock(&phydev->lock); <--------------------+
>          drv->handle_interrupt()                         | Deadlock due
>          +-> ERROR: phy_error()                          + to the nested
>                     +-> phy_process_error()              | mutex lock
>                         +-> mutex_lock(&phydev->lock); <-+
>                             phydev->state = PHY_ERROR;
>                             mutex_unlock(&phydev->lock);
>          mutex_unlock(&phydev->lock);
> 
> [...]

Here is the summary with links:
  - [net] net: phy: Fix deadlocking in phy_error() invocation
    https://git.kernel.org/netdev/net/c/a0e026e7b37e

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html