From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Moore Subject: Re: [PATCH] NETLABEL: Fix an RCU warning Date: Mon, 29 Mar 2010 11:30:10 -0400 Message-ID: <201003291130.10752.paul.moore@hp.com> References: <20100325110621.5348.32020.stgit@warthog.procyon.org.uk> <1269516484.3626.21.camel@edumazet-laptop> <20100329152453.GC2569@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: Text/Plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Eric Dumazet , David Howells , netdev@vger.kernel.org To: paulmck@linux.vnet.ibm.com Return-path: Received: from g5t0007.atlanta.hp.com ([15.192.0.44]:24800 "EHLO g5t0007.atlanta.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752338Ab0C2PaN convert rfc822-to-8bit (ORCPT ); Mon, 29 Mar 2010 11:30:13 -0400 In-Reply-To: <20100329152453.GC2569@linux.vnet.ibm.com> Sender: netdev-owner@vger.kernel.org List-ID: 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 =E0 11:06 +0000, David Howells a =E9crit : > > > Fix an RCU warning in the netlabel code due to missing rcu read l= ocking > > > around an rcu_dereference() in netlbl_unlhsh_hash() when called f= rom > > > netlbl_unlhsh_netdev_handler(): > > >=20 > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D > > > [ INFO: suspicious rcu_dereference_check() usage. ] > > > --------------------------------------------------- > > > net/netlabel/netlabel_unlabeled.c:246 invoked rcu_dereference_che= ck() > > > without protection! > > >=20 > > > other info that might help us debug this: > > >=20 > > >=20 > > > rcu_scheduler_active =3D 1, debug_locks =3D 0 > > >=20 > > > 2 locks held by ip/5108: > > > #0: (rtnl_mutex){+.+.+.}, at: [] > > > rtnl_lock+0x12/0x14 #1: (netlbl_unlhsh_lock){+.+...}, at: > > > [] netlbl_unlhsh_netdev_handler+0x1e/0x86 > > >=20 > > > stack backtrace: > > > Pid: 5108, comm: ip Not tainted 2.6.34-rc2-cachefs #114 > > >=20 > > > Call Trace: > > > [] lockdep_rcu_dereference+0xaa/0xb2 > > > [] netlbl_unlhsh_hash+0x3e/0x50 > > > [] netlbl_unlhsh_search_iface+0xe/0x84 > > > [] netlbl_unlhsh_netdev_handler+0x29/0x86 > > > [] notifier_call_chain+0x32/0x5e > > > [] raw_notifier_call_chain+0xf/0x11 > > > [] call_netdevice_notifiers+0x16/0x18 > > > [] __dev_notify_flags+0x37/0x5b > > > [] dev_change_flags+0x46/0x52 > > > [] do_setlink+0x250/0x3cd > > > [] rtnl_newlink+0x2b6/0x49d > > > [] ? rtnl_newlink+0xab/0x49d > > > [] rtnetlink_rcv_msg+0x1b7/0x1d2 > > > [] ? rtnetlink_rcv_msg+0x0/0x1d2 > > > [] netlink_rcv_skb+0x3e/0x8f > > > [] rtnetlink_rcv+0x21/0x28 > > > [] netlink_unicast+0x218/0x28f > > > [] netlink_sendmsg+0x26b/0x27a > > > [] sock_sendmsg+0xd4/0xf5 > > > [] ? might_fault+0x4e/0x9e > > > [] ? might_fault+0x4e/0x9e > > > [] ? might_fault+0x97/0x9e > > > [] ? might_fault+0x4e/0x9e > > > [] ? verify_iovec+0x59/0x97 > > > [] sys_sendmsg+0x209/0x273 > > > [] ? __do_fault+0x395/0x3cd > > > [] ? handle_mm_fault+0x324/0x69d > > > [] ? trace_hardirqs_on_caller+0x10c/0x130 > > > [] ? audit_syscall_entry+0x17d/0x1b0 > > > [] ? trace_hardirqs_on_thunk+0x3a/0x3f > > > [] system_call_fastpath+0x16/0x1b > > >=20 > > > Signed-off-by: David Howells > > > --- > > >=20 > > > net/netlabel/netlabel_unlabeled.c | 2 ++ > > > 1 files changed, 2 insertions(+), 0 deletions(-) > > >=20 > > > 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(struc= t > > > notifier_block *this, > > >=20 > > > /* XXX - should this be a check for NETDEV_DOWN or _UNREGISTER?= */ > > > if (event =3D=3D NETDEV_DOWN) { > > >=20 > > > + rcu_read_lock(); > > >=20 > > > spin_lock(&netlbl_unlhsh_lock); > > > iface =3D netlbl_unlhsh_search_iface(dev->ifindex); > > > if (iface !=3D NULL && iface->valid) { > > >=20 > > > @@ -807,6 +808,7 @@ static int netlbl_unlhsh_netdev_handler(struc= t > > > notifier_block *this, > > >=20 > > > } else > > > =09 > > > iface =3D NULL; > > > =09 > > > spin_unlock(&netlbl_unlhsh_lock); > > >=20 > > > + rcu_read_unlock(); > > >=20 > > > } > > > =09 > > > if (iface !=3D NULL) > > >=20 > > > -- > >=20 > > Sorry this is not the right fix. > >=20 > > Fix is to change the dereference check to take into account the loc= k > > owned here. >=20 > So we need the rcu_dereference() in netlbl_unlhsh_search_iface() > to become someething like the following? >=20 > bkt_list =3D &rcu_dereference_check(netlbl_unlhsh, > rcu_read_lock_held() || > lockdep_is_held(&netlbl_unlhsh_lock))->tbl[bkt]; >=20 > Or is this the wrong lock? As Eric pointed out in response to the message above, I believe the sol= ution=20 is to simply remove the rcu_dereference() call in the netlbl_unlhsh_has= h()=20 function. --=20 paul moore linux @ hp