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: <c3c57c62-f5f7-4b80-aa8f-d6edefbb5493@omp.ru>
Date: Wed, 22 Jan 2025 21:33:15 +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

Hello!

  My Cogent Embedded tenure is long over, so I dropped my old email... :-)

On 1/22/25 7:19 PM, Kory Maincent 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?

[...]

MBR, Sergey