netdev - Re: [PATCH net v2 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: <20250127112850.05d7769b@kmaincent-XPS-13-7390>
Date: Mon, 27 Jan 2025 11:28:50 +0100
From: Kory Maincent <kory.maincent@...tlin.com>
To: Paul Barker <paul.barker.ct@...renesas.com>
Cc: 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>, Sergei Shtylyov
 <sergei.shtylyov@...entembedded.com>, 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, Sergey
 Shtylyov <s.shtylyov@....ru>
Subject: Re: [PATCH net v2 1/2] net: ravb: Fix missing rtnl lock in suspend
 path

On Thu, 23 Jan 2025 18:33:58 +0100
Kory Maincent <kory.maincent@...tlin.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