Re: [PATCH net v4] ravb: Fix races between ravb_tx_timeout_work() and net related ops

From: Sergey Shtylyov <s.shtylyov@omp.ru>
To: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
	<davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
	<pabeni@redhat.com>
Cc: <netdev@vger.kernel.org>, <linux-renesas-soc@vger.kernel.org>
Subject: Re: [PATCH net v4] ravb: Fix races between ravb_tx_timeout_work() and net related ops
Date: Mon, 27 Nov 2023 19:27:35 +0300	[thread overview]
Message-ID: <995c2598-294e-3d80-05a8-b79e668fb2de@omp.ru> (raw)
In-Reply-To: <20231127122420.3706751-1-yoshihiro.shimoda.uh@renesas.com>

On 11/27/23 3:24 PM, Yoshihiro Shimoda wrote:

> Fix races between ravb_tx_timeout_work() and functions of net_device_ops
> and ethtool_ops by using rtnl_trylock() and rtnl_unlock(). Note that
> since ravb_close() is under the rtnl lock and calls cancel_work_sync(),
> ravb_tx_timeout_work() should calls rtnl_trylock(). Otherwise, a deadlock
> may happen in ravb_tx_timeout_work() like below:
> 
> CPU0			CPU1
> 			ravb_tx_timeout()
> 			schedule_work()
> ...
> __dev_close_many()
> // Under rtnl lock
> ravb_close()
> cancel_work_sync()
> // Waiting
> 			ravb_tx_timeout_work()
> 			rtnl_lock()
> 			// This is possible to cause a deadlock
> 
> If rtnl_trylock() fails, rescheduling the work with sleep for 1 msec.
> 
> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index c70cff80cc99..7c007ecd3ff6 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1874,6 +1874,12 @@ static void ravb_tx_timeout_work(struct work_struct *work)
>  	struct net_device *ndev = priv->ndev;
>  	int error;
>  
> +	if (!rtnl_trylock()) {
> +		usleep_range(1000, 2000);

   Why not msleep(1) though?

> +		schedule_work(&priv->work);
> +		return;
> +	}
> +
>  	netif_tx_stop_all_queues(ndev);
>  
>  	/* Stop PTP Clock driver */
[...]

MBR, Sergey