linux-kernel - Re: [PATCH RFC tip/core/rcu 04/23] net: Make accesses to ->br

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]

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/