netdev - Re: [PATCH net] eth: alx: take rtnl

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: <36bb049a-2ca8-d3b3-1db4-2ea665e7ab80@gmail.com>
Date:   Thu, 29 Sep 2022 09:48:01 +0200
From:   Niels Dossche <dossche.niels@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc:     netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com,
        Zbynek Michl <zbynek.michl@...il.com>, chris.snook@...il.com,
        johannes@...solutions.net
Subject: Re: [PATCH net] eth: alx: take rtnl_lock on resume

On 9/28/22 20:12, Jakub Kicinski wrote:
> Zbynek reports that alx trips an rtnl assertion on resume:
> 
>  RTNL: assertion failed at net/core/dev.c (2891)
>  RIP: 0010:netif_set_real_num_tx_queues+0x1ac/0x1c0
>  Call Trace:
>   <TASK>
>   __alx_open+0x230/0x570 [alx]
>   alx_resume+0x54/0x80 [alx]
>   ? pci_legacy_resume+0x80/0x80
>   dpm_run_callback+0x4a/0x150
>   device_resume+0x8b/0x190
>   async_resume+0x19/0x30
>   async_run_entry_fn+0x30/0x130
>   process_one_work+0x1e5/0x3b0
> 
> indeed the driver does not hold rtnl_lock during its internal close
> and re-open functions during suspend/resume. Note that this is not
> a huge bug as the driver implements its own locking, and does not
> implement changing the number of queues, but we need to silence
> the splat.
> 
> Fixes: 4a5fe57e7751 ("alx: use fine-grained locking instead of RTNL")
> Reported-and-tested-by: Zbynek Michl <zbynek.michl@...il.com>
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
> CC: chris.snook@...il.com
> CC: dossche.niels@...il.com
> CC: johannes@...solutions.net
> ---
>  drivers/net/ethernet/atheros/alx/main.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ethernet/atheros/alx/main.c b/drivers/net/ethernet/atheros/alx/main.c
> index a89b93cb4e26..d5939586c82e 100644
> --- a/drivers/net/ethernet/atheros/alx/main.c
> +++ b/drivers/net/ethernet/atheros/alx/main.c
> @@ -1912,11 +1912,14 @@ static int alx_suspend(struct device *dev)
>  
>  	if (!netif_running(alx->dev))
>  		return 0;
> +
> +	rtnl_lock();
>  	netif_device_detach(alx->dev);
>  
>  	mutex_lock(&alx->mtx);
>  	__alx_stop(alx);
>  	mutex_unlock(&alx->mtx);
> +	rtnl_unlock();
>  
>  	return 0;
>  }
> @@ -1927,6 +1930,7 @@ static int alx_resume(struct device *dev)
>  	struct alx_hw *hw = &alx->hw;
>  	int err;
>  
> +	rtnl_lock();
>  	mutex_lock(&alx->mtx);
>  	alx_reset_phy(hw);
>  
> @@ -1943,6 +1947,7 @@ static int alx_resume(struct device *dev)
>  
>  unlock:
>  	mutex_unlock(&alx->mtx);
> +	rtnl_unlock();
>  	return err;
>  }
>  

Reviewed-by: Niels Dossche <dossche.niels@...il.com>