netdev - Re: [PATCH net] ravb: Fix races between ravb_tx_timeout

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7b153bc6-2094-eee5-f506-0c1615032edb@omp.ru>
Date: Tue, 17 Oct 2023 21:58:36 +0300
From: Sergey Shtylyov <s.shtylyov@....ru>
To: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<pabeni@...hat.com>
CC: <netdev@...r.kernel.org>, <linux-renesas-soc@...r.kernel.org>
Subject: Re: [PATCH net] ravb: Fix races between ravb_tx_timeout_work() and
 net related ops

Hello!

On 10/17/23 11:53 AM, 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() calls rtnl_trylock() to avoid a deadlock.

   I don't quite follow... how calling cancel_work_sync() is a problem?
I thought the problem was that unregister_netdev() can be called with
the TX timeout work still pending? And, more generally, shouldn't we
protect against the TX timeout work being executed on a different CPU
than the {net_device|ethtool}_ops methods are being executed (is that
possible?)?
   I also had a suspicion that we still miss the spinlock calls in
ravb_tx_timeout_work() but nothing in particular jumped at me...
mind looking into that? :-)

> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 0ef0b88b7145..b53533ab4599 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -1907,6 +1910,7 @@ static void ravb_tx_timeout_work(struct work_struct *work)
>  		 */
>  		netdev_err(ndev, "%s: ravb_dmac_init() failed, error %d\n",
>  			   __func__, error);
> +		rtnl_unlock();
>  		return;

   Perhaps *goto* instead here?

>  	}
>  	ravb_emac_init(ndev);
> @@ -1917,6 +1921,7 @@ static void ravb_tx_timeout_work(struct work_struct *work)
>  		ravb_ptp_init(ndev, priv->pdev);
>  
>  	netif_tx_start_all_queues(ndev);

   ... and add label here?

> +	rtnl_unlock();
>  }

MBR, Sergey