[PATCH net v2 0/2] Fix missing rtnl lock in suspend path

[PATCH net v2 0/2] Fix missing rtnl lock in suspend path

[Kory Maincent]

Fix the suspend path by ensuring the rtnl lock is held where required.
Calls to open, close and WOL operations must be performed under the
rtnl lock to prevent conflicts with ongoing ndo operations.

Discussion about this issue can be found here:
https://lore.kernel.org/netdev/20250120141926.1290763-1-kory.maincent@bootlin.com/

While working on the ravb fix, it was discovered that the sh_eth driver
has the same issue. This patch series addresses both drivers.

I do not have access to hardware for either of these MACs, so it would
be great if maintainers or others with the relevant boards could test
these fixes.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
Changes in v2:
- Move the rtnl_lock before ravb_wol_setup() and ravb_wol_restore()
  instead of before the if condition.
- Link to v1: https://lore.kernel.org/r/20250122-fix_missing_rtnl_lock_phy_disconnect-v1-0-8cb9f6f88fd1@bootlin.com

---
Kory Maincent (2):
      net: ravb: Fix missing rtnl lock in suspend path
      net: sh_eth: Fix missing rtnl lock in suspend path

 drivers/net/ethernet/renesas/ravb_main.c | 14 ++++++++++++--
 drivers/net/ethernet/renesas/sh_eth.c    |  4 ++++
 2 files changed, 16 insertions(+), 2 deletions(-)
---
base-commit: b1754a69e7be48a64b3cdb0df60a96d97959da73
change-id: 20250122-fix_missing_rtnl_lock_phy_disconnect-fc1d6f5db705

Best regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[PATCH net v2 1/2] net: ravb: Fix missing rtnl lock in suspend path

[Kory Maincent]

Fix the suspend path by ensuring the rtnl lock is held where required.
Calls to ravb_open, ravb_close and wol operations must be performed under
the rtnl lock to prevent conflicts with ongoing ndo operations.

Without this fix, the following warning is triggered:
[   39.032969] =============================
[   39.032983] WARNING: suspicious RCU usage
[   39.033019] -----------------------------
[   39.033033] drivers/net/phy/phy_device.c:2004 suspicious
rcu_dereference_protected() usage!
...
[   39.033597] stack backtrace:
[   39.033613] CPU: 0 UID: 0 PID: 174 Comm: python3 Not tainted
6.13.0-rc7-next-20250116-arm64-renesas-00002-g35245dfdc62c #7
[   39.033623] Hardware name: Renesas SMARC EVK version 2 based on
r9a08g045s33 (DT)
[   39.033628] Call trace:
[   39.033633]  show_stack+0x14/0x1c (C)
[   39.033652]  dump_stack_lvl+0xb4/0xc4
[   39.033664]  dump_stack+0x14/0x1c
[   39.033671]  lockdep_rcu_suspicious+0x16c/0x22c
[   39.033682]  phy_detach+0x160/0x190
[   39.033694]  phy_disconnect+0x40/0x54
[   39.033703]  ravb_close+0x6c/0x1cc
[   39.033714]  ravb_suspend+0x48/0x120
[   39.033721]  dpm_run_callback+0x4c/0x14c
[   39.033731]  device_suspend+0x11c/0x4dc
[   39.033740]  dpm_suspend+0xdc/0x214
[   39.033748]  dpm_suspend_start+0x48/0x60
[   39.033758]  suspend_devices_and_enter+0x124/0x574
[   39.033769]  pm_suspend+0x1ac/0x274
[   39.033778]  state_store+0x88/0x124
[   39.033788]  kobj_attr_store+0x14/0x24
[   39.033798]  sysfs_kf_write+0x48/0x6c
[   39.033808]  kernfs_fop_write_iter+0x118/0x1a8
[   39.033817]  vfs_write+0x27c/0x378
[   39.033825]  ksys_write+0x64/0xf4
[   39.033833]  __arm64_sys_write+0x18/0x20
[   39.033841]  invoke_syscall+0x44/0x104
[   39.033852]  el0_svc_common.constprop.0+0xb4/0xd4
[   39.033862]  do_el0_svc+0x18/0x20
[   39.033870]  el0_svc+0x3c/0xf0
[   39.033880]  el0t_64_sync_handler+0xc0/0xc4
[   39.033888]  el0t_64_sync+0x154/0x158
[   39.041274] ravb 11c30000.ethernet eth0: Link is Down

Reported-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Closes: https://lore.kernel.org/netdev/4c6419d8-c06b-495c-b987-d66c2e1ff848@tuxon.dev/
Fixes: 0184165b2f42 ("ravb: add sleep PM suspend/resume support")
Reviewed-by: Paul Barker <paul.barker.ct@bp.renesas.com>
Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---

Changes in v2:
- Move the rtnl_lock before ravb_wol_setup() and and ravb_wol_restore()
  instead of before the if condition.
---
 drivers/net/ethernet/renesas/ravb_main.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index bc395294a32d..cfe4f0f364f3 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -3217,10 +3217,16 @@ static int ravb_suspend(struct device *dev)
 
 	netif_device_detach(ndev);
 
-	if (priv->wol_enabled)
-		return ravb_wol_setup(ndev);
+	if (priv->wol_enabled) {
+		rtnl_lock();
+		ret = ravb_wol_setup(ndev);
+		rtnl_unlock();
+		return ret;
+	}
 
+	rtnl_lock();
 	ret = ravb_close(ndev);
+	rtnl_unlock();
 	if (ret)
 		return ret;
 
@@ -3247,7 +3253,9 @@ static int ravb_resume(struct device *dev)
 
 	/* If WoL is enabled restore the interface. */
 	if (priv->wol_enabled) {
+		rtnl_lock();
 		ret = ravb_wol_restore(ndev);
+		rtnl_unlock();
 		if (ret)
 			return ret;
 	} else {
@@ -3257,7 +3265,9 @@ static int ravb_resume(struct device *dev)
 	}
 
 	/* Reopening the interface will restore the device to the working state. */
+	rtnl_lock();
 	ret = ravb_open(ndev);
+	rtnl_unlock();
 	if (ret < 0)
 		goto out_rpm_put;
 

-- 
2.34.1

[PATCH net v2 2/2] net: sh_eth: Fix missing rtnl lock in suspend path

[Kory Maincent]

Fix the suspend path by ensuring the rtnl lock is held where required.
Calls to sh_eth_close, sh_eth_open and wol operations must be performed
under the rtnl lock to prevent conflicts with ongoing ndo operations.

Fixes: b71af04676e9 ("sh_eth: add more PM methods")
Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/net/ethernet/renesas/sh_eth.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 8887b8921009..5fc8027c92c7 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -3494,10 +3494,12 @@ static int sh_eth_suspend(struct device *dev)
 
 	netif_device_detach(ndev);
 
+	rtnl_lock();
 	if (mdp->wol_enabled)
 		ret = sh_eth_wol_setup(ndev);
 	else
 		ret = sh_eth_close(ndev);
+	rtnl_unlock();
 
 	return ret;
 }
@@ -3511,10 +3513,12 @@ static int sh_eth_resume(struct device *dev)
 	if (!netif_running(ndev))
 		return 0;
 
+	rtnl_lock();
 	if (mdp->wol_enabled)
 		ret = sh_eth_wol_restore(ndev);
 	else
 		ret = sh_eth_open(ndev);
+	rtnl_unlock();
 
 	if (ret < 0)
 		return ret;

-- 
2.34.1

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

[Paul Barker]

[-- Attachment #1.1.1: Type: text/plain, Size: 4488 bytes --]

On 23/01/2025 16:58, Kory Maincent wrote:
> Fix the suspend path by ensuring the rtnl lock is held where required.
> Calls to ravb_open, ravb_close and wol operations must be performed under
> the rtnl lock to prevent conflicts with ongoing ndo operations.
> 
> Without this fix, the following warning is triggered:
> [   39.032969] =============================
> [   39.032983] WARNING: suspicious RCU usage
> [   39.033019] -----------------------------
> [   39.033033] drivers/net/phy/phy_device.c:2004 suspicious
> rcu_dereference_protected() usage!
> ...
> [   39.033597] stack backtrace:
> [   39.033613] CPU: 0 UID: 0 PID: 174 Comm: python3 Not tainted
> 6.13.0-rc7-next-20250116-arm64-renesas-00002-g35245dfdc62c #7
> [   39.033623] Hardware name: Renesas SMARC EVK version 2 based on
> r9a08g045s33 (DT)
> [   39.033628] Call trace:
> [   39.033633]  show_stack+0x14/0x1c (C)
> [   39.033652]  dump_stack_lvl+0xb4/0xc4
> [   39.033664]  dump_stack+0x14/0x1c
> [   39.033671]  lockdep_rcu_suspicious+0x16c/0x22c
> [   39.033682]  phy_detach+0x160/0x190
> [   39.033694]  phy_disconnect+0x40/0x54
> [   39.033703]  ravb_close+0x6c/0x1cc
> [   39.033714]  ravb_suspend+0x48/0x120
> [   39.033721]  dpm_run_callback+0x4c/0x14c
> [   39.033731]  device_suspend+0x11c/0x4dc
> [   39.033740]  dpm_suspend+0xdc/0x214
> [   39.033748]  dpm_suspend_start+0x48/0x60
> [   39.033758]  suspend_devices_and_enter+0x124/0x574
> [   39.033769]  pm_suspend+0x1ac/0x274
> [   39.033778]  state_store+0x88/0x124
> [   39.033788]  kobj_attr_store+0x14/0x24
> [   39.033798]  sysfs_kf_write+0x48/0x6c
> [   39.033808]  kernfs_fop_write_iter+0x118/0x1a8
> [   39.033817]  vfs_write+0x27c/0x378
> [   39.033825]  ksys_write+0x64/0xf4
> [   39.033833]  __arm64_sys_write+0x18/0x20
> [   39.033841]  invoke_syscall+0x44/0x104
> [   39.033852]  el0_svc_common.constprop.0+0xb4/0xd4
> [   39.033862]  do_el0_svc+0x18/0x20
> [   39.033870]  el0_svc+0x3c/0xf0
> [   39.033880]  el0t_64_sync_handler+0xc0/0xc4
> [   39.033888]  el0t_64_sync+0x154/0x158
> [   39.041274] ravb 11c30000.ethernet eth0: Link is Down
> 
> Reported-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> Closes: https://lore.kernel.org/netdev/4c6419d8-c06b-495c-b987-d66c2e1ff848@tuxon.dev/
> Fixes: 0184165b2f42 ("ravb: add sleep PM suspend/resume support")
> Reviewed-by: Paul Barker <paul.barker.ct@bp.renesas.com>
> Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
> 
> Changes in v2:
> - Move the rtnl_lock before ravb_wol_setup() and and ravb_wol_restore()
>   instead of before the if condition.
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index bc395294a32d..cfe4f0f364f3 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -3217,10 +3217,16 @@ static int ravb_suspend(struct device *dev)
>  
>  	netif_device_detach(ndev);
>  
> -	if (priv->wol_enabled)
> -		return ravb_wol_setup(ndev);
> +	if (priv->wol_enabled) {
> +		rtnl_lock();
> +		ret = ravb_wol_setup(ndev);
> +		rtnl_unlock();
> +		return ret;
> +	}
>  
> +	rtnl_lock();
>  	ret = ravb_close(ndev);
> +	rtnl_unlock();
>  	if (ret)
>  		return ret;
>  
> @@ -3247,7 +3253,9 @@ static int ravb_resume(struct device *dev)
>  
>  	/* If WoL is enabled restore the interface. */
>  	if (priv->wol_enabled) {
> +		rtnl_lock();
>  		ret = ravb_wol_restore(ndev);
> +		rtnl_unlock();
>  		if (ret)
>  			return ret;
>  	} else {
> @@ -3257,7 +3265,9 @@ static int ravb_resume(struct device *dev)
>  	}
>  
>  	/* Reopening the interface will restore the device to the working state. */
> +	rtnl_lock();
>  	ret = ravb_open(ndev);
> +	rtnl_unlock();
>  	if (ret < 0)
>  		goto out_rpm_put;
>  
> 

Please remove Reviewed-by tags when making changes like this in a
subsequent version of a patch series, this is no longer the patch I
reviewed.

I don't like the multiple lock/unlock calls in each function. I think v1
was better, where we take the lock once in each function and then unlock
when it is no longer needed or when we're about to return.

Thanks,

-- 
Paul Barker

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Kory Maincent]

On Thu, 23 Jan 2025 17:23:11 +0000
Paul Barker <paul.barker.ct@bp.renesas.com> wrote:

> On 23/01/2025 16:58, Kory Maincent wrote:
> > Fix the suspend path by ensuring the rtnl lock is held where required.
> > Calls to ravb_open, ravb_close and wol operations must be performed under
> > the rtnl lock to prevent conflicts with ongoing ndo operations.

...

> >  
> > @@ -3247,7 +3253,9 @@ static int ravb_resume(struct device *dev)
> >  
> >  	/* If WoL is enabled restore the interface. */
> >  	if (priv->wol_enabled) {
> > +		rtnl_lock();
> >  		ret = ravb_wol_restore(ndev);
> > +		rtnl_unlock();
> >  		if (ret)
> >  			return ret;
> >  	} else {
> > @@ -3257,7 +3265,9 @@ static int ravb_resume(struct device *dev)
> >  	}
> >  
> >  	/* Reopening the interface will restore the device to the working
> > state. */
> > +	rtnl_lock();
> >  	ret = ravb_open(ndev);
> > +	rtnl_unlock();
> >  	if (ret < 0)
> >  		goto out_rpm_put;
> >  
> >   
> 
> Please remove Reviewed-by tags when making changes like this in a
> subsequent version of a patch series, this is no longer the patch I
> reviewed.

Oh, sorry for that!

> I don't like the multiple lock/unlock calls in each function. I think v1
> was better, where we take the lock once in each function and then unlock
> when it is no longer needed or when we're about to return.

You will need to achieve a consensus on it with Claudiu. His point of view has
that the locking scheme looks complicated.

On my side I don't have really an opinion, maybe a small preference for v1 which
is protecting wol_enabled flag even if it is not needed.

---
pw-bot: cr

-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

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

[Kory Maincent]

On Thu, 23 Jan 2025 18:33:58 +0100
Kory Maincent <kory.maincent@bootlin.com> wrote:
 
> > >  
> > > @@ -3247,7 +3253,9 @@ static int ravb_resume(struct device *dev)
> > >  
> > >  	/* If WoL is enabled restore the interface. */
> > >  	if (priv->wol_enabled) {
> > > +		rtnl_lock();
> > >  		ret = ravb_wol_restore(ndev);
> > > +		rtnl_unlock();
> > >  		if (ret)
> > >  			return ret;
> > >  	} else {
> > > @@ -3257,7 +3265,9 @@ static int ravb_resume(struct device *dev)
> > >  	}
> > >  
> > >  	/* Reopening the interface will restore the device to the working
> > > state. */
> > > +	rtnl_lock();
> > >  	ret = ravb_open(ndev);
> > > +	rtnl_unlock();
> > >  	if (ret < 0)
> > >  		goto out_rpm_put;
>
> > I don't like the multiple lock/unlock calls in each function. I think v1
> > was better, where we take the lock once in each function and then unlock
> > when it is no longer needed or when we're about to return.
> 
> You will need to achieve a consensus on it with Claudiu. His point of view has
> that the locking scheme looks complicated.
> 
> On my side I don't have really an opinion, maybe a small preference for v1
> which is protecting wol_enabled flag even if it is not needed.

Claudiu any remarks?

If not I will come back to the first version as asked by Paul who is the
Maintainer of the ravb driver.

Sergey have asked to remove the duplicate of the if condition.
Paul is this ok for you?

@@ -3245,19 +3250,21 @@ 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) {
+       if (priv->wol_enabled)
                ret = ravb_wol_restore(ndev);
-               if (ret)
-                       return ret;
-       } else {
+       else
                ret = pm_runtime_force_resume(dev);
-               if (ret)
-                       return ret;
+
+       if (ret) {
+               rtnl_unlock();
+               return ret;
        }
 
        /* Reopening the interface will restore the device to the working
state. */
        ret = ravb_open(ndev);
+       rtnl_unlock();
        if (ret < 0)
                goto out_rpm_put;


Note: Sergey, I have received your mail as spam. 

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

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

[Claudiu Beznea]

On 27.01.2025 12:28, Kory Maincent wrote:
> On Thu, 23 Jan 2025 18:33:58 +0100
> Kory Maincent <kory.maincent@bootlin.com> wrote:
>  
>>>>  
>>>> @@ -3247,7 +3253,9 @@ static int ravb_resume(struct device *dev)
>>>>  
>>>>  	/* If WoL is enabled restore the interface. */
>>>>  	if (priv->wol_enabled) {
>>>> +		rtnl_lock();
>>>>  		ret = ravb_wol_restore(ndev);
>>>> +		rtnl_unlock();
>>>>  		if (ret)
>>>>  			return ret;
>>>>  	} else {
>>>> @@ -3257,7 +3265,9 @@ static int ravb_resume(struct device *dev)
>>>>  	}
>>>>  
>>>>  	/* Reopening the interface will restore the device to the working
>>>> state. */
>>>> +	rtnl_lock();
>>>>  	ret = ravb_open(ndev);
>>>> +	rtnl_unlock();
>>>>  	if (ret < 0)
>>>>  		goto out_rpm_put;
>>
>>> I don't like the multiple lock/unlock calls in each function. I think v1
>>> was better, where we take the lock once in each function and then unlock
>>> when it is no longer needed or when we're about to return.
>>
>> You will need to achieve a consensus on it with Claudiu. His point of view has
>> that the locking scheme looks complicated.
>>
>> On my side I don't have really an opinion, maybe a small preference for v1
>> which is protecting wol_enabled flag even if it is not needed.
> 
> Claudiu any remarks?

Sorry for the delay. I still consider it safe as proposed (taking the lock
around the individual functions) due to the above reasons:

1/ in ravb_suspend():
- the execution just returns after ravb_wol_setup()
- there is no need to lock around runtime PM function
  (pm_runtime_force_suspend()) as the execution through it reach this
  driver only though the driver specific runtime PM function which is a nop
  (and FMPOV it should be removed)

2/ in ravb_resume():
- locking only around ravb_wol_restore() and ravb_open() mimics what is
  done when the interface is open/closed through user space; in that
  scenario the ravb_close()/ravb_open() are called with rtnl_lock() held
  through devinet_ioctl()
- and for the above mentioned reason there is no need to lock around
  pm_runtime_force_resume()

Please follow the approach preferred by the maintainers.

Thank you,
Claudiu

> If not I will come back to the first version as asked by Paul who is the
> Maintainer of the ravb driver.
> 
> Sergey have asked to remove the duplicate of the if condition.
> Paul is this ok for you?
> 
> @@ -3245,19 +3250,21 @@ 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) {
> +       if (priv->wol_enabled)
>                 ret = ravb_wol_restore(ndev);
> -               if (ret)
> -                       return ret;
> -       } else {
> +       else
>                 ret = pm_runtime_force_resume(dev);
> -               if (ret)
> -                       return ret;
> +
> +       if (ret) {
> +               rtnl_unlock();
> +               return ret;
>         }
>  
>         /* Reopening the interface will restore the device to the working
> state. */
>         ret = ravb_open(ndev);
> +       rtnl_unlock();
>         if (ret < 0)
>                 goto out_rpm_put;
> 
> 
> Note: Sergey, I have received your mail as spam. 
> 
> Regards,