[PATCH] NETLABEL: Fix an RCU warning

[PATCH] NETLABEL: Fix an RCU warning

[David Howells]

Fix an RCU warning in the netlabel code due to missing rcu read locking around
an rcu_dereference() in netlbl_unlhsh_hash() when called from
netlbl_unlhsh_netdev_handler():

===================================================
[ INFO: suspicious rcu_dereference_check() usage. ]
---------------------------------------------------
net/netlabel/netlabel_unlabeled.c:246 invoked rcu_dereference_check() without protection!

other info that might help us debug this:


rcu_scheduler_active = 1, debug_locks = 0
2 locks held by ip/5108:
 #0:  (rtnl_mutex){+.+.+.}, at: [<ffffffff812c4a36>] rtnl_lock+0x12/0x14
 #1:  (netlbl_unlhsh_lock){+.+...}, at: [<ffffffff8134daa4>] netlbl_unlhsh_netdev_handler+0x1e/0x86

stack backtrace:
Pid: 5108, comm: ip Not tainted 2.6.34-rc2-cachefs #114
Call Trace:
 [<ffffffff8105121f>] lockdep_rcu_dereference+0xaa/0xb2
 [<ffffffff8134d781>] netlbl_unlhsh_hash+0x3e/0x50
 [<ffffffff8134d7a1>] netlbl_unlhsh_search_iface+0xe/0x84
 [<ffffffff8134daaf>] netlbl_unlhsh_netdev_handler+0x29/0x86
 [<ffffffff81048362>] notifier_call_chain+0x32/0x5e
 [<ffffffff810483fe>] raw_notifier_call_chain+0xf/0x11
 [<ffffffff812ba924>] call_netdevice_notifiers+0x16/0x18
 [<ffffffff812bac22>] __dev_notify_flags+0x37/0x5b
 [<ffffffff812bac8c>] dev_change_flags+0x46/0x52
 [<ffffffff812c41af>] do_setlink+0x250/0x3cd
 [<ffffffff812c4ee8>] rtnl_newlink+0x2b6/0x49d
 [<ffffffff812c4cdd>] ? rtnl_newlink+0xab/0x49d
 [<ffffffff812c4c17>] rtnetlink_rcv_msg+0x1b7/0x1d2
 [<ffffffff812c4a60>] ? rtnetlink_rcv_msg+0x0/0x1d2
 [<ffffffff812cc1dc>] netlink_rcv_skb+0x3e/0x8f
 [<ffffffff812c4a59>] rtnetlink_rcv+0x21/0x28
 [<ffffffff812cbefe>] netlink_unicast+0x218/0x28f
 [<ffffffff812cc76e>] netlink_sendmsg+0x26b/0x27a
 [<ffffffff812a9f1d>] sock_sendmsg+0xd4/0xf5
 [<ffffffff81096dca>] ? might_fault+0x4e/0x9e
 [<ffffffff81096dca>] ? might_fault+0x4e/0x9e
 [<ffffffff81096e13>] ? might_fault+0x97/0x9e
 [<ffffffff81096dca>] ? might_fault+0x4e/0x9e
 [<ffffffff812b4122>] ? verify_iovec+0x59/0x97
 [<ffffffff812aa1d9>] sys_sendmsg+0x209/0x273
 [<ffffffff810976dc>] ? __do_fault+0x395/0x3cd
 [<ffffffff8109928f>] ? handle_mm_fault+0x324/0x69d
 [<ffffffff81051e4e>] ? trace_hardirqs_on_caller+0x10c/0x130
 [<ffffffff81074972>] ? audit_syscall_entry+0x17d/0x1b0
 [<ffffffff81364154>] ? trace_hardirqs_on_thunk+0x3a/0x3f
 [<ffffffff81001eeb>] system_call_fastpath+0x16/0x1b

Signed-off-by: David Howells <dhowells@redhat.com>
---

 net/netlabel/netlabel_unlabeled.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
index 852d9d7..7ea64e4 100644
--- a/net/netlabel/netlabel_unlabeled.c
+++ b/net/netlabel/netlabel_unlabeled.c
@@ -799,6 +799,7 @@ static int netlbl_unlhsh_netdev_handler(struct notifier_block *this,
 
 	/* XXX - should this be a check for NETDEV_DOWN or _UNREGISTER? */
 	if (event == NETDEV_DOWN) {
+		rcu_read_lock();
 		spin_lock(&netlbl_unlhsh_lock);
 		iface = netlbl_unlhsh_search_iface(dev->ifindex);
 		if (iface != NULL && iface->valid) {
@@ -807,6 +808,7 @@ static int netlbl_unlhsh_netdev_handler(struct notifier_block *this,
 		} else
 			iface = NULL;
 		spin_unlock(&netlbl_unlhsh_lock);
+		rcu_read_unlock();
 	}
 
 	if (iface != NULL)

Re: [PATCH] NETLABEL: Fix an RCU warning

[Eric Dumazet]

Le jeudi 25 mars 2010 à 11:06 +0000, David Howells a écrit :
> Fix an RCU warning in the netlabel code due to missing rcu read locking around
> an rcu_dereference() in netlbl_unlhsh_hash() when called from
> netlbl_unlhsh_netdev_handler():
> 
> ===================================================
> [ INFO: suspicious rcu_dereference_check() usage. ]
> ---------------------------------------------------
> net/netlabel/netlabel_unlabeled.c:246 invoked rcu_dereference_check() without protection!
> 
> other info that might help us debug this:
> 
> 
> rcu_scheduler_active = 1, debug_locks = 0
> 2 locks held by ip/5108:
>  #0:  (rtnl_mutex){+.+.+.}, at: [<ffffffff812c4a36>] rtnl_lock+0x12/0x14
>  #1:  (netlbl_unlhsh_lock){+.+...}, at: [<ffffffff8134daa4>] netlbl_unlhsh_netdev_handler+0x1e/0x86
> 
> stack backtrace:
> Pid: 5108, comm: ip Not tainted 2.6.34-rc2-cachefs #114
> Call Trace:
>  [<ffffffff8105121f>] lockdep_rcu_dereference+0xaa/0xb2
>  [<ffffffff8134d781>] netlbl_unlhsh_hash+0x3e/0x50
>  [<ffffffff8134d7a1>] netlbl_unlhsh_search_iface+0xe/0x84
>  [<ffffffff8134daaf>] netlbl_unlhsh_netdev_handler+0x29/0x86
>  [<ffffffff81048362>] notifier_call_chain+0x32/0x5e
>  [<ffffffff810483fe>] raw_notifier_call_chain+0xf/0x11
>  [<ffffffff812ba924>] call_netdevice_notifiers+0x16/0x18
>  [<ffffffff812bac22>] __dev_notify_flags+0x37/0x5b
>  [<ffffffff812bac8c>] dev_change_flags+0x46/0x52
>  [<ffffffff812c41af>] do_setlink+0x250/0x3cd
>  [<ffffffff812c4ee8>] rtnl_newlink+0x2b6/0x49d
>  [<ffffffff812c4cdd>] ? rtnl_newlink+0xab/0x49d
>  [<ffffffff812c4c17>] rtnetlink_rcv_msg+0x1b7/0x1d2
>  [<ffffffff812c4a60>] ? rtnetlink_rcv_msg+0x0/0x1d2
>  [<ffffffff812cc1dc>] netlink_rcv_skb+0x3e/0x8f
>  [<ffffffff812c4a59>] rtnetlink_rcv+0x21/0x28
>  [<ffffffff812cbefe>] netlink_unicast+0x218/0x28f
>  [<ffffffff812cc76e>] netlink_sendmsg+0x26b/0x27a
>  [<ffffffff812a9f1d>] sock_sendmsg+0xd4/0xf5
>  [<ffffffff81096dca>] ? might_fault+0x4e/0x9e
>  [<ffffffff81096dca>] ? might_fault+0x4e/0x9e
>  [<ffffffff81096e13>] ? might_fault+0x97/0x9e
>  [<ffffffff81096dca>] ? might_fault+0x4e/0x9e
>  [<ffffffff812b4122>] ? verify_iovec+0x59/0x97
>  [<ffffffff812aa1d9>] sys_sendmsg+0x209/0x273
>  [<ffffffff810976dc>] ? __do_fault+0x395/0x3cd
>  [<ffffffff8109928f>] ? handle_mm_fault+0x324/0x69d
>  [<ffffffff81051e4e>] ? trace_hardirqs_on_caller+0x10c/0x130
>  [<ffffffff81074972>] ? audit_syscall_entry+0x17d/0x1b0
>  [<ffffffff81364154>] ? trace_hardirqs_on_thunk+0x3a/0x3f
>  [<ffffffff81001eeb>] system_call_fastpath+0x16/0x1b
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  net/netlabel/netlabel_unlabeled.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
> index 852d9d7..7ea64e4 100644
> --- a/net/netlabel/netlabel_unlabeled.c
> +++ b/net/netlabel/netlabel_unlabeled.c
> @@ -799,6 +799,7 @@ static int netlbl_unlhsh_netdev_handler(struct notifier_block *this,
>  
>  	/* XXX - should this be a check for NETDEV_DOWN or _UNREGISTER? */
>  	if (event == NETDEV_DOWN) {
> +		rcu_read_lock();
>  		spin_lock(&netlbl_unlhsh_lock);
>  		iface = netlbl_unlhsh_search_iface(dev->ifindex);
>  		if (iface != NULL && iface->valid) {
> @@ -807,6 +808,7 @@ static int netlbl_unlhsh_netdev_handler(struct notifier_block *this,
>  		} else
>  			iface = NULL;
>  		spin_unlock(&netlbl_unlhsh_lock);
> +		rcu_read_unlock();
>  	}
>  
>  	if (iface != NULL)
> 
> --

Sorry this is not the right fix.

Fix is to change the dereference check to take into account the lock
owned here.

Re: [PATCH] NETLABEL: Fix an RCU warning

[David Howells]

Eric Dumazet <eric.dumazet@gmail.com> wrote:

> Sorry this is not the right fix.
> 
> Fix is to change the dereference check to take into account the lock
> owned here.

Then the comments on netlbl_unlhsh_hash(), netlbl_unlhsh_search_iface(),
netlbl_unlhsh_search_iface_def() and netlbl_unlhsh_add_iface() are all wrong,
for all of them say:

	* The caller is responsible for calling the rcu_read_[un]lock()
	* functions.

Furthermore, netlabel_unlhsh_add() and netlabel_unlhsh_remove() _do_ wrap the
calls to those functions in rcu_read_lock'd sections.

David

Re: [PATCH] NETLABEL: Fix an RCU warning

[Eric Dumazet]

Le jeudi 25 mars 2010 à 11:37 +0000, David Howells a écrit :
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
> > Sorry this is not the right fix.
> > 
> > Fix is to change the dereference check to take into account the lock
> > owned here.
> 
> Then the comments on netlbl_unlhsh_hash(), netlbl_unlhsh_search_iface(),
> netlbl_unlhsh_search_iface_def() and netlbl_unlhsh_add_iface() are all wrong,
> for all of them say:
> 
> 	* The caller is responsible for calling the rcu_read_[un]lock()
> 	* functions.
> 
> Furthermore, netlabel_unlhsh_add() and netlabel_unlhsh_remove() _do_ wrap the
> calls to those functions in rcu_read_lock'd sections.

Current code is probably fine.
Comments are not up to date (as many other comments BTW)

Only the dereference check is bad, as it assumes the rcu_read lock is
held.

Its not the case, we own a spinlock.

You suggest adding a surrounding rcu lock, but this surrounding lock
adds overhead on normal kernels, to correct checker warnings only.

If a mutex was protecting existing code, instead of a spinlock, then
adding rcu_read_lock() would be no correct anyway (existing code would
not be allowed to call a might_sleep function)

Please take a look at rcu_dereference_check() in :

__sk_free() (file net/core/sock.c) 
__in6_dev_get() (file include/net/addrconf.h)
rcu_dereference_check_fdtable (file include/linux/fdtable.h)
task_subsys_state() (file include/linux/cgroup.h)
rcu_dereference_check_sched_domain (file kernel/sched.c)
...

for examples of proper checks.

Yes, its more difficult, but its the right thing.

Re: [PATCH] NETLABEL: Fix an RCU warning

[Paul Moore]

On Thursday 25 March 2010 09:32:20 am Eric Dumazet wrote:
> Le jeudi 25 mars 2010 à 11:37 +0000, David Howells a écrit :
> > Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > > Sorry this is not the right fix.
> > > 
> > > Fix is to change the dereference check to take into account the lock
> > > owned here.
> > 
> > Then the comments on netlbl_unlhsh_hash(), netlbl_unlhsh_search_iface(),
> > netlbl_unlhsh_search_iface_def() and netlbl_unlhsh_add_iface() are all
> > wrong,
> > 
> > for all of them say:
> > 	* The caller is responsible for calling the rcu_read_[un]lock()
> > 	* functions.
> > 
> > Furthermore, netlabel_unlhsh_add() and netlabel_unlhsh_remove() _do_ wrap
> > the calls to those functions in rcu_read_lock'd sections.
> 
> Current code is probably fine.
> Comments are not up to date (as many other comments BTW)
> 
> Only the dereference check is bad, as it assumes the rcu_read lock is
> held.
> 
> Its not the case, we own a spinlock.
> 
> You suggest adding a surrounding rcu lock, but this surrounding lock
> adds overhead on normal kernels, to correct checker warnings only.

The rcu_dereference call, and the outdated comments for that matter, are 
likely artifacts from an early patch.  When the code was originally being 
developed I had assumed that you _always_ needed to hold the RCU read lock 
when doing RCU operations, regardless of if you were already holding the 
associated spinlock.  Thankfully, Paul McKenney was able to review the code 
and helped me to understand RCU a bit better, unfortunately, it looks like I 
still missed a few spots :/

Give me a day or two to go through the comments again and do some sanity 
checks and I'll send out a patch.  However, if you've already got something 
that is tested and ready to go don't let me stand in your way.

-- 
paul moore
linux @ hp

Re: [PATCH] NETLABEL: Fix an RCU warning

[Paul E. McKenney]

On Thu, Mar 25, 2010 at 12:28:04PM +0100, Eric Dumazet wrote:
> Le jeudi 25 mars 2010 à 11:06 +0000, David Howells a écrit :
> > Fix an RCU warning in the netlabel code due to missing rcu read locking around
> > an rcu_dereference() in netlbl_unlhsh_hash() when called from
> > netlbl_unlhsh_netdev_handler():
> > 
> > ===================================================
> > [ INFO: suspicious rcu_dereference_check() usage. ]
> > ---------------------------------------------------
> > net/netlabel/netlabel_unlabeled.c:246 invoked rcu_dereference_check() without protection!
> > 
> > other info that might help us debug this:
> > 
> > 
> > rcu_scheduler_active = 1, debug_locks = 0
> > 2 locks held by ip/5108:
> >  #0:  (rtnl_mutex){+.+.+.}, at: [<ffffffff812c4a36>] rtnl_lock+0x12/0x14
> >  #1:  (netlbl_unlhsh_lock){+.+...}, at: [<ffffffff8134daa4>] netlbl_unlhsh_netdev_handler+0x1e/0x86
> > 
> > stack backtrace:
> > Pid: 5108, comm: ip Not tainted 2.6.34-rc2-cachefs #114
> > Call Trace:
> >  [<ffffffff8105121f>] lockdep_rcu_dereference+0xaa/0xb2
> >  [<ffffffff8134d781>] netlbl_unlhsh_hash+0x3e/0x50
> >  [<ffffffff8134d7a1>] netlbl_unlhsh_search_iface+0xe/0x84
> >  [<ffffffff8134daaf>] netlbl_unlhsh_netdev_handler+0x29/0x86
> >  [<ffffffff81048362>] notifier_call_chain+0x32/0x5e
> >  [<ffffffff810483fe>] raw_notifier_call_chain+0xf/0x11
> >  [<ffffffff812ba924>] call_netdevice_notifiers+0x16/0x18
> >  [<ffffffff812bac22>] __dev_notify_flags+0x37/0x5b
> >  [<ffffffff812bac8c>] dev_change_flags+0x46/0x52
> >  [<ffffffff812c41af>] do_setlink+0x250/0x3cd
> >  [<ffffffff812c4ee8>] rtnl_newlink+0x2b6/0x49d
> >  [<ffffffff812c4cdd>] ? rtnl_newlink+0xab/0x49d
> >  [<ffffffff812c4c17>] rtnetlink_rcv_msg+0x1b7/0x1d2
> >  [<ffffffff812c4a60>] ? rtnetlink_rcv_msg+0x0/0x1d2
> >  [<ffffffff812cc1dc>] netlink_rcv_skb+0x3e/0x8f
> >  [<ffffffff812c4a59>] rtnetlink_rcv+0x21/0x28
> >  [<ffffffff812cbefe>] netlink_unicast+0x218/0x28f
> >  [<ffffffff812cc76e>] netlink_sendmsg+0x26b/0x27a
> >  [<ffffffff812a9f1d>] sock_sendmsg+0xd4/0xf5
> >  [<ffffffff81096dca>] ? might_fault+0x4e/0x9e
> >  [<ffffffff81096dca>] ? might_fault+0x4e/0x9e
> >  [<ffffffff81096e13>] ? might_fault+0x97/0x9e
> >  [<ffffffff81096dca>] ? might_fault+0x4e/0x9e
> >  [<ffffffff812b4122>] ? verify_iovec+0x59/0x97
> >  [<ffffffff812aa1d9>] sys_sendmsg+0x209/0x273
> >  [<ffffffff810976dc>] ? __do_fault+0x395/0x3cd
> >  [<ffffffff8109928f>] ? handle_mm_fault+0x324/0x69d
> >  [<ffffffff81051e4e>] ? trace_hardirqs_on_caller+0x10c/0x130
> >  [<ffffffff81074972>] ? audit_syscall_entry+0x17d/0x1b0
> >  [<ffffffff81364154>] ? trace_hardirqs_on_thunk+0x3a/0x3f
> >  [<ffffffff81001eeb>] system_call_fastpath+0x16/0x1b
> > 
> > Signed-off-by: David Howells <dhowells@redhat.com>
> > ---
> > 
> >  net/netlabel/netlabel_unlabeled.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
> > index 852d9d7..7ea64e4 100644
> > --- a/net/netlabel/netlabel_unlabeled.c
> > +++ b/net/netlabel/netlabel_unlabeled.c
> > @@ -799,6 +799,7 @@ static int netlbl_unlhsh_netdev_handler(struct notifier_block *this,
> >  
> >  	/* XXX - should this be a check for NETDEV_DOWN or _UNREGISTER? */
> >  	if (event == NETDEV_DOWN) {
> > +		rcu_read_lock();
> >  		spin_lock(&netlbl_unlhsh_lock);
> >  		iface = netlbl_unlhsh_search_iface(dev->ifindex);
> >  		if (iface != NULL && iface->valid) {
> > @@ -807,6 +808,7 @@ static int netlbl_unlhsh_netdev_handler(struct notifier_block *this,
> >  		} else
> >  			iface = NULL;
> >  		spin_unlock(&netlbl_unlhsh_lock);
> > +		rcu_read_unlock();
> >  	}
> >  
> >  	if (iface != NULL)
> > 
> > --
> 
> Sorry this is not the right fix.
> 
> Fix is to change the dereference check to take into account the lock
> owned here.

So we need the rcu_dereference() in netlbl_unlhsh_search_iface()
to become someething like the following?

	bkt_list = &rcu_dereference_check(netlbl_unlhsh,
					  rcu_read_lock_held() ||
					  lockdep_is_held(&netlbl_unlhsh_lock))->tbl[bkt];

Or is this the wrong lock?

							Thanx, Paul

Re: [PATCH] NETLABEL: Fix an RCU warning

[Paul Moore]

On Monday 29 March 2010 11:24:53 am Paul E. McKenney wrote:
> On Thu, Mar 25, 2010 at 12:28:04PM +0100, Eric Dumazet wrote:
> > Le jeudi 25 mars 2010 à 11:06 +0000, David Howells a écrit :
> > > Fix an RCU warning in the netlabel code due to missing rcu read locking
> > > around an rcu_dereference() in netlbl_unlhsh_hash() when called from
> > > netlbl_unlhsh_netdev_handler():
> > > 
> > > ===================================================
> > > [ INFO: suspicious rcu_dereference_check() usage. ]
> > > ---------------------------------------------------
> > > net/netlabel/netlabel_unlabeled.c:246 invoked rcu_dereference_check()
> > > without protection!
> > > 
> > > other info that might help us debug this:
> > > 
> > > 
> > > rcu_scheduler_active = 1, debug_locks = 0
> > > 
> > > 2 locks held by ip/5108:
> > >  #0:  (rtnl_mutex){+.+.+.}, at: [<ffffffff812c4a36>]
> > >  rtnl_lock+0x12/0x14 #1:  (netlbl_unlhsh_lock){+.+...}, at:
> > >  [<ffffffff8134daa4>] netlbl_unlhsh_netdev_handler+0x1e/0x86
> > > 
> > > stack backtrace:
> > > Pid: 5108, comm: ip Not tainted 2.6.34-rc2-cachefs #114
> > > 
> > > Call Trace:
> > >  [<ffffffff8105121f>] lockdep_rcu_dereference+0xaa/0xb2
> > >  [<ffffffff8134d781>] netlbl_unlhsh_hash+0x3e/0x50
> > >  [<ffffffff8134d7a1>] netlbl_unlhsh_search_iface+0xe/0x84
> > >  [<ffffffff8134daaf>] netlbl_unlhsh_netdev_handler+0x29/0x86
> > >  [<ffffffff81048362>] notifier_call_chain+0x32/0x5e
> > >  [<ffffffff810483fe>] raw_notifier_call_chain+0xf/0x11
> > >  [<ffffffff812ba924>] call_netdevice_notifiers+0x16/0x18
> > >  [<ffffffff812bac22>] __dev_notify_flags+0x37/0x5b
> > >  [<ffffffff812bac8c>] dev_change_flags+0x46/0x52
> > >  [<ffffffff812c41af>] do_setlink+0x250/0x3cd
> > >  [<ffffffff812c4ee8>] rtnl_newlink+0x2b6/0x49d
> > >  [<ffffffff812c4cdd>] ? rtnl_newlink+0xab/0x49d
> > >  [<ffffffff812c4c17>] rtnetlink_rcv_msg+0x1b7/0x1d2
> > >  [<ffffffff812c4a60>] ? rtnetlink_rcv_msg+0x0/0x1d2
> > >  [<ffffffff812cc1dc>] netlink_rcv_skb+0x3e/0x8f
> > >  [<ffffffff812c4a59>] rtnetlink_rcv+0x21/0x28
> > >  [<ffffffff812cbefe>] netlink_unicast+0x218/0x28f
> > >  [<ffffffff812cc76e>] netlink_sendmsg+0x26b/0x27a
> > >  [<ffffffff812a9f1d>] sock_sendmsg+0xd4/0xf5
> > >  [<ffffffff81096dca>] ? might_fault+0x4e/0x9e
> > >  [<ffffffff81096dca>] ? might_fault+0x4e/0x9e
> > >  [<ffffffff81096e13>] ? might_fault+0x97/0x9e
> > >  [<ffffffff81096dca>] ? might_fault+0x4e/0x9e
> > >  [<ffffffff812b4122>] ? verify_iovec+0x59/0x97
> > >  [<ffffffff812aa1d9>] sys_sendmsg+0x209/0x273
> > >  [<ffffffff810976dc>] ? __do_fault+0x395/0x3cd
> > >  [<ffffffff8109928f>] ? handle_mm_fault+0x324/0x69d
> > >  [<ffffffff81051e4e>] ? trace_hardirqs_on_caller+0x10c/0x130
> > >  [<ffffffff81074972>] ? audit_syscall_entry+0x17d/0x1b0
> > >  [<ffffffff81364154>] ? trace_hardirqs_on_thunk+0x3a/0x3f
> > >  [<ffffffff81001eeb>] system_call_fastpath+0x16/0x1b
> > > 
> > > Signed-off-by: David Howells <dhowells@redhat.com>
> > > ---
> > > 
> > >  net/netlabel/netlabel_unlabeled.c |    2 ++
> > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/net/netlabel/netlabel_unlabeled.c
> > > b/net/netlabel/netlabel_unlabeled.c index 852d9d7..7ea64e4 100644
> > > --- a/net/netlabel/netlabel_unlabeled.c
> > > +++ b/net/netlabel/netlabel_unlabeled.c
> > > @@ -799,6 +799,7 @@ static int netlbl_unlhsh_netdev_handler(struct
> > > notifier_block *this,
> > > 
> > >  	/* XXX - should this be a check for NETDEV_DOWN or _UNREGISTER? */
> > >  	if (event == NETDEV_DOWN) {
> > > 
> > > +		rcu_read_lock();
> > > 
> > >  		spin_lock(&netlbl_unlhsh_lock);
> > >  		iface = netlbl_unlhsh_search_iface(dev->ifindex);
> > >  		if (iface != NULL && iface->valid) {
> > > 
> > > @@ -807,6 +808,7 @@ static int netlbl_unlhsh_netdev_handler(struct
> > > notifier_block *this,
> > > 
> > >  		} else
> > >  		
> > >  			iface = NULL;
> > >  		
> > >  		spin_unlock(&netlbl_unlhsh_lock);
> > > 
> > > +		rcu_read_unlock();
> > > 
> > >  	}
> > >  	
> > >  	if (iface != NULL)
> > > 
> > > --
> > 
> > Sorry this is not the right fix.
> > 
> > Fix is to change the dereference check to take into account the lock
> > owned here.
> 
> So we need the rcu_dereference() in netlbl_unlhsh_search_iface()
> to become someething like the following?
> 
> 	bkt_list = &rcu_dereference_check(netlbl_unlhsh,
> 					  rcu_read_lock_held() ||
> 					  lockdep_is_held(&netlbl_unlhsh_lock))->tbl[bkt];
> 
> Or is this the wrong lock?

As Eric pointed out in response to the message above, I believe the solution 
is to simply remove the rcu_dereference() call in the netlbl_unlhsh_hash() 
function.

-- 
paul moore
linux @ hp

Re: [PATCH] NETLABEL: Fix an RCU warning

[Paul E. McKenney]

On Mon, Mar 29, 2010 at 11:30:10AM -0400, Paul Moore wrote:
> On Monday 29 March 2010 11:24:53 am Paul E. McKenney wrote:
> > On Thu, Mar 25, 2010 at 12:28:04PM +0100, Eric Dumazet wrote:
> > > Le jeudi 25 mars 2010 à 11:06 +0000, David Howells a écrit :
> > > > Fix an RCU warning in the netlabel code due to missing rcu read locking
> > > > around an rcu_dereference() in netlbl_unlhsh_hash() when called from
> > > > netlbl_unlhsh_netdev_handler():
> > > > 
> > > > ===================================================
> > > > [ INFO: suspicious rcu_dereference_check() usage. ]
> > > > ---------------------------------------------------
> > > > net/netlabel/netlabel_unlabeled.c:246 invoked rcu_dereference_check()
> > > > without protection!
> > > > 
> > > > other info that might help us debug this:
> > > > 
> > > > 
> > > > rcu_scheduler_active = 1, debug_locks = 0
> > > > 
> > > > 2 locks held by ip/5108:
> > > >  #0:  (rtnl_mutex){+.+.+.}, at: [<ffffffff812c4a36>]
> > > >  rtnl_lock+0x12/0x14 #1:  (netlbl_unlhsh_lock){+.+...}, at:
> > > >  [<ffffffff8134daa4>] netlbl_unlhsh_netdev_handler+0x1e/0x86
> > > > 
> > > > stack backtrace:
> > > > Pid: 5108, comm: ip Not tainted 2.6.34-rc2-cachefs #114
> > > > 
> > > > Call Trace:
> > > >  [<ffffffff8105121f>] lockdep_rcu_dereference+0xaa/0xb2
> > > >  [<ffffffff8134d781>] netlbl_unlhsh_hash+0x3e/0x50
> > > >  [<ffffffff8134d7a1>] netlbl_unlhsh_search_iface+0xe/0x84
> > > >  [<ffffffff8134daaf>] netlbl_unlhsh_netdev_handler+0x29/0x86
> > > >  [<ffffffff81048362>] notifier_call_chain+0x32/0x5e
> > > >  [<ffffffff810483fe>] raw_notifier_call_chain+0xf/0x11
> > > >  [<ffffffff812ba924>] call_netdevice_notifiers+0x16/0x18
> > > >  [<ffffffff812bac22>] __dev_notify_flags+0x37/0x5b
> > > >  [<ffffffff812bac8c>] dev_change_flags+0x46/0x52
> > > >  [<ffffffff812c41af>] do_setlink+0x250/0x3cd
> > > >  [<ffffffff812c4ee8>] rtnl_newlink+0x2b6/0x49d
> > > >  [<ffffffff812c4cdd>] ? rtnl_newlink+0xab/0x49d
> > > >  [<ffffffff812c4c17>] rtnetlink_rcv_msg+0x1b7/0x1d2
> > > >  [<ffffffff812c4a60>] ? rtnetlink_rcv_msg+0x0/0x1d2
> > > >  [<ffffffff812cc1dc>] netlink_rcv_skb+0x3e/0x8f
> > > >  [<ffffffff812c4a59>] rtnetlink_rcv+0x21/0x28
> > > >  [<ffffffff812cbefe>] netlink_unicast+0x218/0x28f
> > > >  [<ffffffff812cc76e>] netlink_sendmsg+0x26b/0x27a
> > > >  [<ffffffff812a9f1d>] sock_sendmsg+0xd4/0xf5
> > > >  [<ffffffff81096dca>] ? might_fault+0x4e/0x9e
> > > >  [<ffffffff81096dca>] ? might_fault+0x4e/0x9e
> > > >  [<ffffffff81096e13>] ? might_fault+0x97/0x9e
> > > >  [<ffffffff81096dca>] ? might_fault+0x4e/0x9e
> > > >  [<ffffffff812b4122>] ? verify_iovec+0x59/0x97
> > > >  [<ffffffff812aa1d9>] sys_sendmsg+0x209/0x273
> > > >  [<ffffffff810976dc>] ? __do_fault+0x395/0x3cd
> > > >  [<ffffffff8109928f>] ? handle_mm_fault+0x324/0x69d
> > > >  [<ffffffff81051e4e>] ? trace_hardirqs_on_caller+0x10c/0x130
> > > >  [<ffffffff81074972>] ? audit_syscall_entry+0x17d/0x1b0
> > > >  [<ffffffff81364154>] ? trace_hardirqs_on_thunk+0x3a/0x3f
> > > >  [<ffffffff81001eeb>] system_call_fastpath+0x16/0x1b
> > > > 
> > > > Signed-off-by: David Howells <dhowells@redhat.com>
> > > > ---
> > > > 
> > > >  net/netlabel/netlabel_unlabeled.c |    2 ++
> > > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/net/netlabel/netlabel_unlabeled.c
> > > > b/net/netlabel/netlabel_unlabeled.c index 852d9d7..7ea64e4 100644
> > > > --- a/net/netlabel/netlabel_unlabeled.c
> > > > +++ b/net/netlabel/netlabel_unlabeled.c
> > > > @@ -799,6 +799,7 @@ static int netlbl_unlhsh_netdev_handler(struct
> > > > notifier_block *this,
> > > > 
> > > >  	/* XXX - should this be a check for NETDEV_DOWN or _UNREGISTER? */
> > > >  	if (event == NETDEV_DOWN) {
> > > > 
> > > > +		rcu_read_lock();
> > > > 
> > > >  		spin_lock(&netlbl_unlhsh_lock);
> > > >  		iface = netlbl_unlhsh_search_iface(dev->ifindex);
> > > >  		if (iface != NULL && iface->valid) {
> > > > 
> > > > @@ -807,6 +808,7 @@ static int netlbl_unlhsh_netdev_handler(struct
> > > > notifier_block *this,
> > > > 
> > > >  		} else
> > > >  		
> > > >  			iface = NULL;
> > > >  		
> > > >  		spin_unlock(&netlbl_unlhsh_lock);
> > > > 
> > > > +		rcu_read_unlock();
> > > > 
> > > >  	}
> > > >  	
> > > >  	if (iface != NULL)
> > > > 
> > > > --
> > > 
> > > Sorry this is not the right fix.
> > > 
> > > Fix is to change the dereference check to take into account the lock
> > > owned here.
> > 
> > So we need the rcu_dereference() in netlbl_unlhsh_search_iface()
> > to become someething like the following?
> > 
> > 	bkt_list = &rcu_dereference_check(netlbl_unlhsh,
> > 					  rcu_read_lock_held() ||
> > 					  lockdep_is_held(&netlbl_unlhsh_lock))->tbl[bkt];
> > 
> > Or is this the wrong lock?
> 
> As Eric pointed out in response to the message above, I believe the solution 
> is to simply remove the rcu_dereference() call in the netlbl_unlhsh_hash() 
> function.

It would be at the moment, but this will break once Arnd Bergmann gets
his sparse-based checks done.  With these checks, we decorate RCU-protected
pointers, and then sparse yells if you access such a pointer without the
proper rcu_dereference() invocation.

							Thanx, Paul

Re: [PATCH] NETLABEL: Fix an RCU warning

[Paul Moore]

On Monday 29 March 2010 11:58:57 am Paul E. McKenney wrote:
> On Mon, Mar 29, 2010 at 11:30:10AM -0400, Paul Moore wrote:
> > On Monday 29 March 2010 11:24:53 am Paul E. McKenney wrote:
> > > On Thu, Mar 25, 2010 at 12:28:04PM +0100, Eric Dumazet wrote:
> > > > Le jeudi 25 mars 2010 à 11:06 +0000, David Howells a écrit :
> > > > > Fix an RCU warning in the netlabel code due to missing rcu read
> > > > > locking around an rcu_dereference() in netlbl_unlhsh_hash() when
> > > > > called from netlbl_unlhsh_netdev_handler():
> > > > > 
> > > > > ===================================================
> > > > > [ INFO: suspicious rcu_dereference_check() usage. ]
> > > > > ---------------------------------------------------
> > > > > net/netlabel/netlabel_unlabeled.c:246 invoked
> > > > > rcu_dereference_check() without protection!

...

> > As Eric pointed out in response to the message above, I believe the
> > solution is to simply remove the rcu_dereference() call in the
> > netlbl_unlhsh_hash() function.
> 
> It would be at the moment, but this will break once Arnd Bergmann gets
> his sparse-based checks done.  With these checks, we decorate RCU-protected
> pointers, and then sparse yells if you access such a pointer without the
> proper rcu_dereference() invocation.

Okay, is there a recommended approach towards accessing RCU-protected pointers 
both under a RCU read lock and under only a spinlock (or similar lock 
construct)?  I know I could do something based on querying the state of the 
RCU/etc. locks but that seems like a hack and could interfere with some of the 
logic used to detect coding problems.

-- 
paul moore
linux @ hp

Re: [PATCH] NETLABEL: Fix an RCU warning

[Eric Dumazet]

Le lundi 29 mars 2010 à 16:05 -0400, Paul Moore a écrit :

> Okay, is there a recommended approach towards accessing RCU-protected pointers 
> both under a RCU read lock and under only a spinlock (or similar lock 
> construct)?  I know I could do something based on querying the state of the 
> RCU/etc. locks but that seems like a hack and could interfere with some of the 
> logic used to detect coding problems.
> 

Say you use a common function func1(), that use RCU protection, from a
pure reader, and from a pure writer.

pure_reader()
{
	rcu_read_lock();
	res = func1();
	rcu_read_unlock();
}

pure_writer()
{
	spin_lock(&some_lock);
	res = func1();
	spin_unlock(&some_lock);
}

then, func1 could use

func1()
{
 ..... ptr = rcu_dereference_check(xxx->ptr,
				   rcu_read_lock_held() || 
				   lockdep_is_held(&some_lock));
...
}


They are numerous examples in tree.
If some iterators use implicit rcu_dereference(), you'll probably need
to define new iterators, so that full condition can be tested.

rcu_dereference() is a shorthand for 
	rcu_dereference_check(p, rcu_read_lock_held())

This only deals for pure readers, not potential pure writer :)