linux-kernel - Re: [PATCH net 1/3] net: mvneta: Fix spinlock usage

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: <20160309161326.708db252@xhacker>
Date:	Wed, 9 Mar 2016 16:13:26 +0800
From:	Jisheng Zhang <jszhang@...vell.com>
To:	Gregory CLEMENT <gregory.clement@...e-electrons.com>
CC:	"David S. Miller" <davem@...emloft.net>,
	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
	<linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>,
	Lior Amsalem <alior@...vell.com>, Andrew Lunn <andrew@...n.ch>,
	Jason Cooper <jason@...edaemon.net>,
	Ofer Heifetz <oferh@...vell.com>,
	Nadav Haklai <nadavh@...vell.com>,
	Patrick Uiterwijk <patrick@...terwijk.org>,
	Marcin Wojtas <mw@...ihalf.com>,
	Dimitri Epshtein <dima@...vell.com>,
	<linux-arm-kernel@...ts.infradead.org>,
	"Sebastian Hesselbarth" <sebastian.hesselbarth@...il.com>
Subject: Re: [PATCH net 1/3] net: mvneta: Fix spinlock usage

Dear Gregory,

On Wed, 9 Mar 2016 08:49:40 +0100 Gregory CLEMENT wrote:

> Hi Jisheng,
>  
>  On mer., mars 09 2016, Jisheng Zhang <jszhang@...vell.com> wrote:
> 
> > Dear Gregory,
> >
> > On Tue, 8 Mar 2016 13:57:04 +0100 Gregory CLEMENT wrote:
> >  
> >> In the previous patch, the spinlock was not initialized. While it didn't
> >> cause any trouble yet it could be a problem to use it uninitialized.
> >> 
> >> The most annoying part was the critical section protected by the spinlock
> >> in mvneta_stop(). Some of the functions could sleep as pointed when
> >> activated CONFIG_DEBUG_ATOMIC_SLEEP. Actually, in mvneta_stop() we only
> >> need to protect the is_stopped flagged, indeed the code of the notifier
> >> for CPU online is protected by the same spinlock, so when we get the
> >> lock, the notifer work is done.
> >> 
> >> Reported-by: Patrick Uiterwijk <patrick@...terwijk.org>
> >> Signed-off-by: Gregory CLEMENT <gregory.clement@...e-electrons.com>
> >> ---
> >>  drivers/net/ethernet/marvell/mvneta.c | 11 ++++++-----
> >>  1 file changed, 6 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> >> index b0ae69f84493..8dc7df2edff6 100644
> >> --- a/drivers/net/ethernet/marvell/mvneta.c
> >> +++ b/drivers/net/ethernet/marvell/mvneta.c
> >> @@ -3070,17 +3070,17 @@ static int mvneta_stop(struct net_device *dev)
> >>  	struct mvneta_port *pp = netdev_priv(dev);
> >>  
> >>  	/* Inform that we are stopping so we don't want to setup the
> >> -	 * driver for new CPUs in the notifiers
> >> +	 * driver for new CPUs in the notifiers. The code of the
> >> +	 * notifier for CPU online is protected by the same spinlock,
> >> +	 * so when we get the lock, the notifer work is done.
> >>  	 */
> >>  	spin_lock(&pp->lock);
> >>  	pp->is_stopped = true;
> >> +	spin_unlock(&pp->lock);  
> >
> > This fix sleep in atomic issue. But
> > I see race here. Let's assume is_stopped is false.  
> 
> You forgot that the lock was hold in the mvneta_percpu_notifier so your
> scenario can't happen.
> 
> >
> > cpu0:                           	cpu1:
> > mvneta_percpu_notifier():		mvneta_stop():
> >  
> 
> spin_lock(&pp->lock);
> 
> > if (pp->is_stopped) {
> > 	spin_unlock(&pp->lock);
> > 	break;
> > }

OOPS, I misread the code here as "the lock will be unlocked" ;)
Sorry for noise.

If you want, feel free to add

Reviewed-by: Jisheng Zhang <jszhang@...vell.com>

> >  
> 
>                                       the lock is hold in
>                                       mvneta_percpu_notifier(), so as
>                                       said in the comment this cpu is
>                                       waiting for on the following
>                                       line:
>                                       spin_lock(&pp->lock);
> 
>                                       This code will be executed only
>                                       when the lock will be released
> > 					pp->is_stopped = true;
> > 					spin_unlock(&pp->lock);
> >
> >
> > netif_tx_stop_all_queues(pp->dev);
> > for_each_online_cpu(other_cpu) {
> > ....
> >  
> So what will happen is:
> cpu0:                           	cpu1:
> mvneta_percpu_notifier():		mvneta_stop():
> 
> spin_lock(&pp->lock);
> if (pp->is_stopped) {
> 	spin_unlock(&pp->lock);
> 	break;
> }
>                                         spin_lock(&pp->lock);
> 
> netif_tx_stop_all_queues(pp->dev);
> for_each_online_cpu(other_cpu) {
> ....
> spin_unlock(&pp->lock);
> 					pp->is_stopped = true;
> 					spin_unlock(&pp->lock);
> 
> 
> Gregory
>