netdev - Re: [PATCH 1/2] net: dsa: RCU-protect dsa_ptr in struct net

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] [day] [month] [year] [list]

Message-ID: <36b492b1d880e8956f9ff63431e713f290e5526d.camel@siemens.com>
Date: Tue, 17 Sep 2024 11:08:09 +0000
From: "Sverdlin, Alexander" <alexander.sverdlin@...mens.com>
To: "gur.stavi@...wei.com" <gur.stavi@...wei.com>
CC: "Mark-MC.Lee@...iatek.com" <Mark-MC.Lee@...iatek.com>, "andrew@...n.ch"
	<andrew@...n.ch>, "bridge@...ts.linux.dev" <bridge@...ts.linux.dev>,
	"claudiu.manoil@....com" <claudiu.manoil@....com>, "dqfext@...il.com"
	<dqfext@...il.com>, "nbd@....name" <nbd@....name>, "davem@...emloft.net"
	<davem@...emloft.net>, "stable@...r.kernel.org" <stable@...r.kernel.org>,
	"angelogioacchino.delregno@...labora.com"
	<angelogioacchino.delregno@...labora.com>, "olteanv@...il.com"
	<olteanv@...il.com>, "linux-mediatek@...ts.infradead.org"
	<linux-mediatek@...ts.infradead.org>, "UNGLinuxDriver@...rochip.com"
	<UNGLinuxDriver@...rochip.com>, "bcm-kernel-feedback-list@...adcom.com"
	<bcm-kernel-feedback-list@...adcom.com>, "arinc.unal@...nc9.com"
	<arinc.unal@...nc9.com>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"alexandre.belloni@...tlin.com" <alexandre.belloni@...tlin.com>,
	"daniel@...rotopia.org" <daniel@...rotopia.org>, "razor@...ckwall.org"
	<razor@...ckwall.org>, "kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>, "roopa@...dia.com"
	<roopa@...dia.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"matthias.bgg@...il.com" <matthias.bgg@...il.com>, "f.fainelli@...il.com"
	<f.fainelli@...il.com>, "lorenzo@...nel.org" <lorenzo@...nel.org>,
	"sean.wang@...iatek.com" <sean.wang@...iatek.com>
Subject: Re: [PATCH 1/2] net: dsa: RCU-protect dsa_ptr in struct net_device

Hi Gur,

On Tue, 2024-09-17 at 13:30 +0300, Gur Stavi wrote:
> > > > @@ -1594,10 +1592,11 @@ void dsa_switch_shutdown(struct dsa_switch *ds)
> > > >    	}
> > > > 
> > > >    	/* Disconnect from further netdevice notifiers on the conduit,
> > > > -	 * since netdev_uses_dsa() will now return false.
> > > > +	 * from now on, netdev_uses_dsa_currently() will return false.
> > > >    	 */
> > > >    	dsa_switch_for_each_cpu_port(dp, ds)
> > > > -		dp->conduit->dsa_ptr = NULL;
> > > > +		rcu_assign_pointer(dp->conduit->dsa_ptr, NULL);
> > > > +	synchronize_rcu();
> > > > 
> > > >    	rtnl_unlock();
> > > >    out:
> > > 
> > > Hi, I am a newbie here. Thanks for the opportunity for learning more
> > > about rcu.
> > > Wouldn't it make more sense to call synchronize_rcu after rtnl_unlock?
> > 
> > This is indeed a question which is usually resolved other way around
> > (making locked section shorter), but in this particular case I thought that:
> > - we actually don't need giving rtnl lock sooner, which would potentially
> >    make synchronize_rcu() call longer (if another thread manages to wake up
> >    and claim the rtnl lock before synchronize_rcu())
> > - we are in shutdown phase, we don't need to minimize lock contention, we
> >    need to minimize the overall shutdown time
> 
> But isn't shutdown still multithreaded?
> 10 threads may have similar shutdown task: remove objects from different
> rcu protected data structures while holding rtnl. Then synchronize RCU
> and release the objects.
> Synchronizing RCU from within the lock will completely serialize all
> threads and postpone shutdown whereas outside the lock multiple
> synchronize_rcu could run in parallel.

my understanding is that all other RTNL-protected readers of dsa_ptr are
actually only unnecessary overhead after shutdown callback has been called
once (eventual network configuration running in parallel at the time of
shutdown).

All of them can be ignored (or lower-prioritized) after shutdown, we
need to synchronize_rcu() and free/close what we have to do afterwards
and just poweroff/reboot.

So the only effect switching synchronize_rcu() and rtnl_unlock() around
would be that
- it looks more logical
- it requires longer to reboot/shutdown

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com