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: <20100512183359.4829ff0e@nehalam>
Date:	Wed, 12 May 2010 18:33:59 -0700
From:	Stephen Hemminger <shemminger@...ux-foundation.org>
To:	paulmck@...ux.vnet.ibm.com
Cc:	linux-kernel@...r.kernel.org, mingo@...e.hu, laijs@...fujitsu.com,
	dipankar@...ibm.com, akpm@...ux-foundation.org,
	mathieu.desnoyers@...ymtl.ca, josh@...htriplett.org,
	dvhltc@...ibm.com, niv@...ibm.com, tglx@...utronix.de,
	peterz@...radead.org, rostedt@...dmis.org, Valdis.Kletnieks@...edu,
	dhowells@...hat.com, eric.dumazet@...il.com,
	Arnd Bergmann <arnd@...db.de>,
	David Miller <davem@...emloft.net>
Subject: Re: [PATCH RFC tip/core/rcu 04/23] net: Make accesses to ->br_port
 safe for sparse RCU

On Wed, 12 May 2010 15:35:25 -0700
"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com> wrote:

> On Wed, May 12, 2010 at 02:44:53PM -0700, Stephen Hemminger wrote:
> > On Wed, 12 May 2010 14:33:23 -0700
> > "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com> wrote:
> > 
> > > diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> > > index 9101a4e..3f66cd1 100644
> > > --- a/net/bridge/br_fdb.c
> > > +++ b/net/bridge/br_fdb.c
> > > @@ -246,7 +246,7 @@ int br_fdb_test_addr(struct net_device *dev, unsigned char *addr)
> > >  		return 0;
> > >  
> > >  	rcu_read_lock();
> > > -	fdb = __br_fdb_get(dev->br_port->br, addr);
> > > +	fdb = __br_fdb_get(br_port(dev)->br, addr);
> > >  	ret = fdb && fdb->dst->dev != dev &&
> > >  		fdb->dst->state == BR_STATE_FORWARDING;
> > >  	rcu_read_unlock();
> > > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> > > index 846d7d1..4fedb60 100644
> > > --- a/net/bridge/br_private.h
> > > +++ b/net/bridge/br_private.h
> > > @@ -229,6 +229,14 @@ static inline int br_is_root_bridge(const struct net_bridge *br)
> > >  	return !memcmp(&br->bridge_id, &br->designated_root, 8);
> > >  }
> > >  
> > > +static inline struct net_bridge_port *br_port(const struct net_device *dev)
> > > +{
> > > +	if (!dev)
> > > +		return NULL;
> > > +
> > > +	return rcu_dereference(dev->br_port);
> > > +}
> > 
> > Looks like this is wrapping existing problems, and hurting not helping.
> > 
> > Why introduce a wrapper that could return NULL and not check the
> > result?
> 
> Fair point!
> 
> > I would rather that:
> >    1. dev should never be null in this cases so the first if() is 
> >       unnecessary, and confuses the semantics.
> >    2. don't use wrapper br_port()
> >    3. have callers check that rcu_dereference(dev->br_port) did not
> >       return NULL.
> >       If they derefernce does return NULL, then it means other CPU
> >       has started tear down and this CPU should just go home quietly.
> 
> OK.
> 
> The reason for br_port() is to allow ->br_port to be a void*.  If we
> eliminate br_port(), then it is necessary to make the definition of the
> struct net_bridge_port available everywhere that ->br_port is given to
> rcu_dereference().  The reason for this is that Arnd's sparse-based RCU
> checking code uses __rcu to tag the data pointed to by an RCU-protected
> pointer.  This in turn means that rcu_dereference() and friends must
> now have access to the pointed-to type, as is done in patch 6 in this
> series.

Then ok. leave the wrapper, but get rid of the !dev part.

I can do it if you want.

Still don't like changing working code to conform to code checking tools.
Especially when code checking tool is missing bad RCU usage that already
exists (like this case). It is a big problem if code assumes rcu_deref
always returns non NULL.

-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ