Re: [RFC][PATCH] identify in_dev_get rcu read-side critical sections

In reviewing the 44 kernel uses of __in_dev_get and seeing many 
cases in 13 of 20 C code files of insertions of rcu_read_lock with 
and without the rcu_dereference that is indicated, so it does appear 
often to be programmer intent.  

Of the programs using __in_dev_get that don't include rcu, devinet.c 
and igmp.c use an rtnl lock.  Five other programs that use __in_dev_get 
without rcu have rtnl locking in the program source code, but I need 
to actually look further into the call tree to say more.

Are there three cases then?  RCU protection with refcnt, RCU without refcnt,
and the bare cast of the dereference? 

Thank you very much for getting it back on track.

  > From paulmck@us.ibm.com  Thu Sep 29 17:23:18 2005

  > Is there any case where __in_dev_get() might be called without
  > needing to be wrapped with rcu_dereference()?  If so, then I
  > agree (FWIW, given my meagre knowledge of Linux networking).

  > If all __in_dev_get() invocations need to be wrapped in
  > rcu_dereference(), then it seems to me that there would be
  > motivation to bury rcu_dereference() in __in_dev_get().

  > > > Some callers of __in_dev_get() don't need rcu_dereference at all
  > > > because they're protected by the rtnl.
  > > 
  > > > BTW, could you please move the rcu_dereference in in_dev_get()
  > > > into the if clause? The barrier is not needed when ip_ptr is
  > > > NULL.
  > > 
  > > The trouble with that may be that there are three events, the
  > > dereference, the assignment, and the conditional test.  The
  > > rcu_dereference() is meant to assure deferred destruction
  > > throughout.

  > One only needs an rcu_dereference() once on the data-flow path from
  > fetching the RCU-protected pointer to dereferencing that pointer.
  > If the pointer is NULL, there is no way you can dereference it,
  > so, technically, Herbert is quite correct.

  > However, rcu_dereference() only generates a memory barrier on DEC
  > Alpha, so there is normally no penalty for using it in the NULL-pointer
  > case.  So, when using rcu_dereference() unconditionally simplifies
  > the code, it may make sense to "just do it".

  > 							Thanx, Paul

On Thu, Sep 29, 2005 at 06:06:50PM -0700, Suzanne Wood wrote:

Are there three cases then?  RCU protection with refcnt, RCU without refcnt,
and the bare cast of the dereference? 

Correct.

The following patch renames __in_dev_get() to __in_dev_get_rtnl() and
introduces __in_dev_get_rcu() to cover the second case.

1) RCU with refcnt should use in_dev_get().
2) RCU without refcnt should use __in_dev_get_rcu().
3) All others must hold RTNL and use __in_dev_get_rtnl().

There is one exception in net/ipv4/route.c which is in fact a pre-existing
race condition.  I've marked it as such so that we remember to fix it.

This patch is based on suggestions and prior work by Suzanne Wood and
Paul McKenney.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt