linux-kernel - Re: [PATCH net 1/2] net: ravb: Fix missing rtnl lock in suspend path

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: <d2511df5-cf68-4dfb-ae22-f4987af830d8@omp.ru>
Date: Wed, 22 Jan 2025 21:53:02 +0300
From: Sergey Shtylyov <s.shtylyov@....ru>
To: Kory Maincent <kory.maincent@...tlin.com>, Paul Barker
	<paul.barker.ct@...renesas.com>, Niklas Söderlund
	<niklas.soderlund@...natech.se>, Andrew Lunn <andrew+netdev@...n.ch>, "David
 S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub
 Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Mikhail Ulyanov
	<mikhail.ulyanov@...entembedded.com>
CC: Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	Niklas Söderlund <niklas.soderlund+renesas@...natech.se>,
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>, <netdev@...r.kernel.org>,
	<linux-renesas-soc@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net 1/2] net: ravb: Fix missing rtnl lock in suspend path

On 1/22/25 9:33 PM, Sergey Shtylyov wrote:
[...]

>> Fix the suspend path by ensuring the rtnl lock is held where required.
> 
>    Maybe suspend/resume path (the same w/the subject)?
> 
>> Calls to ravb_open, ravb_close and wol operations must be performed under
>> the rtnl lock to prevent conflicts with ongoing ndo operations.
> 
> [...]
> 
>> Reported-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
>> Closes: https://lore.kernel.org/netdev/4c6419d8-c06b-495c-b987-d66c2e1ff848@tuxon.dev/
>> Fixes: 0184165b2f42 ("ravb: add sleep PM suspend/resume support")
>> Signed-off-by: Kory Maincent <kory.maincent@...tlin.com>
> 
>     FWIW:
> 
> Reviewed-by: Sergey Shtylyov <s.shtylyov@....ru>
> 
> [...]
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index bc395294a32d..2c6d8e4966c3 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
>> @@ -3245,19 +3250,25 @@ static int ravb_resume(struct device *dev)
>>  	if (!netif_running(ndev))
>>  		return 0;
>>  
>> +	rtnl_lock();
>>  	/* If WoL is enabled restore the interface. */
>>  	if (priv->wol_enabled) {
>>  		ret = ravb_wol_restore(ndev);
>> -		if (ret)
>> +		if (ret)  {
>> +			rtnl_unlock();
>>  			return ret;
>> +		}
>>  	} else {
>>  		ret = pm_runtime_force_resume(dev);
>> -		if (ret)
>> +		if (ret) {
>> +			rtnl_unlock();
>>  			return ret;
> 
>    Hm, are you sure we need to have rtnl_lock around pm_runtime_force_resume() too?
 
   Anyway, the above *if* statements are needlessly duplicated, I think it's time
that we do away with this! :-)

[...]

MBR, Sergey