netdev - Re: Re: [B.A.T.M.A.N.] [PATCH] net: fix possible deadlocks in rtnl

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: <20121201200153.GA1002@pandem0nium>
Date:	Sat, 1 Dec 2012 21:01:53 +0100
From:	Simon Wunderlich <simon.wunderlich@...03.tu-chemnitz.de>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Sven Eckelmann <sven@...fation.org>,
	b.a.t.m.a.n@...ts.open-mesh.org,
	Simon Wunderlich <simon.wunderlich@...03.tu-chemnitz.de>,
	netdev@...r.kernel.org, davem@...emloft.net,
	Simon Wunderlich <siwu@....tu-chemnitz.de>
Subject: Re: Re: [B.A.T.M.A.N.] [PATCH] net: fix possible deadlocks in
 rtnl_trylock/unlock

On Sat, Dec 01, 2012 at 11:44:34AM -0800, Eric Dumazet wrote:
> On Sat, 2012-12-01 at 20:04 +0100, Sven Eckelmann wrote:
> > On Saturday 01 December 2012 10:36:12 Eric Dumazet wrote:
> >  
> > > > diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
> > > > index c5c0593..c122782 100644
> > > > --- a/net/bridge/br_sysfs_br.c
> > > > +++ b/net/bridge/br_sysfs_br.c
> > > > @@ -142,7 +142,7 @@ static ssize_t store_stp_state(struct device *d,
> > > > 
> > > >  	if (!rtnl_trylock())
> > > >  	
> > > >  		return restart_syscall();
> > > >  	
> > > >  	br_stp_set_enabled(br, val);
> > > > 
> > > > -	rtnl_unlock();
> > > > +	__rtnl_unlock();
> > > > 
> > > >  	return len;
> > > >  
> > > >  }
> > > 
> > > I have no idea of why you believe there is a problem here.
> > > 
> > > Could you explain how net_todo_list could be not empty ?
> > > 
> > > As long as no device is unregistered between
> > > rtnl_trylock()/rtnl_unlock(), there is no possible deadlock.
> > 
> > I am not sure what "here" means for your. At least batman-adv tries to 
> > unregister a device -> problem [1]. I will not make any judgements about the 
> > other uses in the kernel/other parts patched by Simon.
> > 
> 
> I was reacting to the change in net/bridge/br_sysfs_br.c
> 
> rtnl_trylock() could set a boolean flag to explicitly WARN_ON()
> in case we try to unregister a device.

Well, I'm not sure if this can happen in the bridge code, but from looking at the
code it doesn't appear to be impossible. It would be better to be sure that it can't
deadlock IMHO.

(Although doing unlock/lock/unlock in an unlock function is also a little "uncommon").

Cheers,
	Simon

Download attachment "signature.asc" of type "application/pgp-signature" (199 bytes)