Re: [PATCH RFC tip/core/rcu 04/23] net: Make accesses to ->br_port safe for sparse RCU

[Stephen Hemminger <shemminger@linux-foundation.org>]

On Wed, 12 May 2010 15:35:25 -0700
"Paul E. McKenney" <paulmck@linux.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@linux.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.

--