What protects rcu_dereference() in __in6_dev_get()?

What protects rcu_dereference() in __in6_dev_get()?

[Paul E. McKenney]

Hello, Yoshi,

Could you please tell me what protects the rcu_dereference() in
__in6_dev_get()?  I am adding lockdep-based checking to RCU, and
"git blame" said I should ask you about this one.

The current code, rcu_dereference(), assumes that this is protected only
by RCU.  My problem might be any of the following:

o	Some other flavor of RCU protects this, e.g., RCU-bh, which
	would require rcu_dereference_bh().

o	This is called from updates as well as from readers, and
	some lock protects the updates.

o	This is called during initialization, when this pointer is
	inaccessible to readers.
	
Please note that I can add a check to cover multiple possibilities.
For a real example in include/linux/fdtable.h:

	file = rcu_dereference_check(fdt->fd[fd],
				     rcu_read_lock_held() ||
				     lockdep_is_held(&files->file_lock) ||
				     atomic_read(&files->count) == 1);

The first argument is the pointer, and the second argument says that
this may be protected by either RCU (as opposed to RCU-bh, RCU-sched,
or SRCU), the files->file_lock as recorded by lockdep, or by being in
a single-threaded process as noted by the value of files->count.
(Please see http://lwn.net/Articles/368683/ for a recent patch, another
will go out soon.)

So, could you please tell me what protects the rcu_dereference()
in __in6_dev_get() so that I can craft the appropriate form of
rcu_dereference()?

							Thanx, Paul

Re: What protects rcu_dereference() in __in6_dev_get()?

[Eric Dumazet]

Le 14/01/2010 19:32, Paul E. McKenney a écrit :
> Hello, Yoshi,
> 
> Could you please tell me what protects the rcu_dereference() in
> __in6_dev_get()?  I am adding lockdep-based checking to RCU, and
> "git blame" said I should ask you about this one.
> 
> The current code, rcu_dereference(), assumes that this is protected only
> by RCU.  My problem might be any of the following:
> 
> o	Some other flavor of RCU protects this, e.g., RCU-bh, which
> 	would require rcu_dereference_bh().
> 
> o	This is called from updates as well as from readers, and
> 	some lock protects the updates.
> 
> o	This is called during initialization, when this pointer is
> 	inaccessible to readers.
> 	
> Please note that I can add a check to cover multiple possibilities.
> For a real example in include/linux/fdtable.h:
> 
> 	file = rcu_dereference_check(fdt->fd[fd],
> 				     rcu_read_lock_held() ||
> 				     lockdep_is_held(&files->file_lock) ||
> 				     atomic_read(&files->count) == 1);
> 
> The first argument is the pointer, and the second argument says that
> this may be protected by either RCU (as opposed to RCU-bh, RCU-sched,
> or SRCU), the files->file_lock as recorded by lockdep, or by being in
> a single-threaded process as noted by the value of files->count.
> (Please see http://lwn.net/Articles/368683/ for a recent patch, another
> will go out soon.)
> 
> So, could you please tell me what protects the rcu_dereference()
> in __in6_dev_get() so that I can craft the appropriate form of
> rcu_dereference()?
> 
> 							Thanx, Paul

Hi Paul

__in6_dev_get() is called either with rcu_read_lock()/rcu_read_unlock() protection,
or with the RTNL mutex held.

Well, thats the theory, we could have some bugs of course :)

Thanks

Re: What protects rcu_dereference() in __in6_dev_get()?

[Paul E. McKenney]

On Fri, Jan 15, 2010 at 06:50:15AM +0100, Eric Dumazet wrote:
> Le 14/01/2010 19:32, Paul E. McKenney a écrit :
> > Hello, Yoshi,
> > 
> > Could you please tell me what protects the rcu_dereference() in
> > __in6_dev_get()?  I am adding lockdep-based checking to RCU, and
> > "git blame" said I should ask you about this one.
> > 
> > The current code, rcu_dereference(), assumes that this is protected only
> > by RCU.  My problem might be any of the following:
> > 
> > o	Some other flavor of RCU protects this, e.g., RCU-bh, which
> > 	would require rcu_dereference_bh().
> > 
> > o	This is called from updates as well as from readers, and
> > 	some lock protects the updates.
> > 
> > o	This is called during initialization, when this pointer is
> > 	inaccessible to readers.
> > 	
> > Please note that I can add a check to cover multiple possibilities.
> > For a real example in include/linux/fdtable.h:
> > 
> > 	file = rcu_dereference_check(fdt->fd[fd],
> > 				     rcu_read_lock_held() ||
> > 				     lockdep_is_held(&files->file_lock) ||
> > 				     atomic_read(&files->count) == 1);
> > 
> > The first argument is the pointer, and the second argument says that
> > this may be protected by either RCU (as opposed to RCU-bh, RCU-sched,
> > or SRCU), the files->file_lock as recorded by lockdep, or by being in
> > a single-threaded process as noted by the value of files->count.
> > (Please see http://lwn.net/Articles/368683/ for a recent patch, another
> > will go out soon.)
> > 
> > So, could you please tell me what protects the rcu_dereference()
> > in __in6_dev_get() so that I can craft the appropriate form of
> > rcu_dereference()?
> > 
> > 							Thanx, Paul
> 
> Hi Paul
> 
> __in6_dev_get() is called either with rcu_read_lock()/rcu_read_unlock() protection,
> or with the RTNL mutex held.

Very good!  So I make a lockdep_rtnl_is_held() in net/core/rtnetlink.c:

	#ifdef CONFIG_PROVE_LOCKING
	int lockdep_rtnl_is_held(void)
	{
		return lockdep_is_held(&rtnl_mutex);
	}
	EXPORT_SYMBOL(lockdep_rtnl_is_held);
	#endif /* #ifdef CONFIG_PROVE_LOCKING */

Then I make __in6_dev_get() look as follows:

	static inline struct inet6_dev *
	__in6_dev_get(struct net_device *dev)
	{
		return rcu_dereference_check(dev->ip6_ptr,
					     rcu_read_lock_held() ||
					     lockdep_rtnl_is_held());
	}

Seem reasonable?

> Well, thats the theory, we could have some bugs of course :)

I know that feeling!  ;-)

						Thanx, Paul

Re: What protects rcu_dereference() in __in6_dev_get()?

[Eric Dumazet]

Le 15/01/2010 16:15, Paul E. McKenney a écrit :
> On Fri, Jan 15, 2010 at 06:50:15AM +0100, Eric Dumazet wrote:
>> __in6_dev_get() is called either with rcu_read_lock()/rcu_read_unlock() protection,
>> or with the RTNL mutex held.
> 
> Very good!  So I make a lockdep_rtnl_is_held() in net/core/rtnetlink.c:
> 
> 	#ifdef CONFIG_PROVE_LOCKING
> 	int lockdep_rtnl_is_held(void)
> 	{
> 		return lockdep_is_held(&rtnl_mutex);
> 	}
> 	EXPORT_SYMBOL(lockdep_rtnl_is_held);
> 	#endif /* #ifdef CONFIG_PROVE_LOCKING */
> 
> Then I make __in6_dev_get() look as follows:
> 
> 	static inline struct inet6_dev *
> 	__in6_dev_get(struct net_device *dev)
> 	{
> 		return rcu_dereference_check(dev->ip6_ptr,
> 					     rcu_read_lock_held() ||
> 					     lockdep_rtnl_is_held());
> 	}
> 
> Seem reasonable?

I guess so, but is lockdep_is_held(&mutex) actually cheking this mutex is owned by us ?

If another thread is the owner, we could miss a bug.

Re: What protects rcu_dereference() in __in6_dev_get()?

[Peter Zijlstra]

On Fri, 2010-01-15 at 16:29 +0100, Eric Dumazet wrote:

> I guess so, but is lockdep_is_held(&mutex) actually cheking this mutex is owned by us ?

Yep, assuming lockdep is still functional, otherwise its return value
will be undefined.

Re: What protects rcu_dereference() in __in6_dev_get()?

[Paul E. McKenney]

On Fri, Jan 15, 2010 at 04:29:14PM +0100, Eric Dumazet wrote:
> Le 15/01/2010 16:15, Paul E. McKenney a écrit :
> > On Fri, Jan 15, 2010 at 06:50:15AM +0100, Eric Dumazet wrote:
> >> __in6_dev_get() is called either with rcu_read_lock()/rcu_read_unlock() protection,
> >> or with the RTNL mutex held.
> > 
> > Very good!  So I make a lockdep_rtnl_is_held() in net/core/rtnetlink.c:
> > 
> > 	#ifdef CONFIG_PROVE_LOCKING
> > 	int lockdep_rtnl_is_held(void)
> > 	{
> > 		return lockdep_is_held(&rtnl_mutex);
> > 	}
> > 	EXPORT_SYMBOL(lockdep_rtnl_is_held);
> > 	#endif /* #ifdef CONFIG_PROVE_LOCKING */
> > 
> > Then I make __in6_dev_get() look as follows:
> > 
> > 	static inline struct inet6_dev *
> > 	__in6_dev_get(struct net_device *dev)
> > 	{
> > 		return rcu_dereference_check(dev->ip6_ptr,
> > 					     rcu_read_lock_held() ||
> > 					     lockdep_rtnl_is_held());
> > 	}
> > 
> > Seem reasonable?
> 
> I guess so, but is lockdep_is_held(&mutex) actually cheking this mutex is owned by us ?

Indeed it does!  But only if lockdep is enabled.  When lockdep is -not-
enabled, rcu_dereference_check() ignores its second argument.

> If another thread is the owner, we could miss a bug.

That s why I created a new lockdep_rtnl_is_held() rather than using the
existing rtnl_is_locked().

							Thanx, Paul