This message generated a parse failure. Raw output follows here. Please use 'back' to navigate. From devnull@lkml.org Wed May 1 17:08:25 2024 Delivery-date: Mon, 29 Mar 2010 20:57:31 +0200 Received: from mx1.redhat.com ([209.132.183.28]:24618 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752642Ab0C2S46 (ORCPT ); Mon, 29 Mar 2010 14:56:58 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o2TIuhGj022769 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 29 Mar 2010 14 Received: from redhat.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o2TIucVv009678; Mon, 29 Mar 2010 14:56:39 -0400 Organization: Red Hat UK Ltd. Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SI4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 3798903 From: David Howells In-Reply-To: <1269882883.3181.25.camel@localhost.localdomain> References: <1269882883.3181.25.camel@localhost.localdomain> <1269531400.3648.1.camel@localhost.localdomain> <20100329152557.GD2569@linux.vnet.ibm.com> To: Trond Myklebust , Arnd Bergmann Cc: dhowells@redhat.com, paulmck@linux.vnet.ibm.com, Zdenek Kabelac , Linux Kernel Mailing List , linux-nfs@vger.kernel.org Subject: Re: INFO: suspicious rcu_dereference_check() Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Date: Mon, 29 Mar 2010 19:56:37 +0100 Message-Id: <18617.1269888997@redhat.com> X-Scanned-By: MIMEDefang 2.67 on 10.5.11.11 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-Id: X-Mailing-List: linux-kernel@vger.kernel.org --=-=-= Trond Myklebust wrote: > I don't mind whether you or I push them to Linus, but IIRC you had a > couple of comments about the last patchset I saw from David, so I was > expecting to see either a reply from him or patch update. Did I miss > that reply? (Ccing: David) I was waiting to see what Arnd did, since Paul said: This looks good at the moment, however, the sparse changes that Arnd Bergmann is working on will invalidate a couple of the changes below. Of course, better a future problem than a here-and-now problem, but is there an easy way to fix both? My message and Paul's reply are attached for Arnd's convenience. Maybe Arnd could include my changes, or supply me with his patch? David --=-=-= Content-Type: message/rfc822 Content-Disposition: attachment; filename=10878 Return-Path: dhowells@redhat.com Received: from mail.corp.redhat.com [10.5.5.52] by warthog.procyon.org.uk with IMAP (fetchmail-6.3.11) for (single-drop); Thu, 18 Mar 2010 13:33:37 +0000 (GMT) Received: from zmta02.collab.prod.int.phx2.redhat.com (LHLO zmta02.collab.prod.int.phx2.redhat.com) (10.5.5.32) by mail06.corp.redhat.com with LMTP; Thu, 18 Mar 2010 09:33:06 -0400 (EDT) Received: from localhost (localhost.localdomain [127.0.0.1]) by zmta02.collab.prod.int.phx2.redhat.com (Postfix) with ESMTP id 5A0EA12800A for ; Thu, 18 Mar 2010 09:33:06 -0400 (EDT) Received: from zmta02.collab.prod.int.phx2.redhat.com ([127.0.0.1]) by localhost (zmta02.collab.prod.int.phx2.redhat.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id cgBPB+f0SN5D for ; Thu, 18 Mar 2010 09:33:06 -0400 (EDT) Received: from int-mx03.intmail.prod.int.phx2.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.16]) by zmta02.collab.prod.int.phx2.redhat.com (Postfix) with ESMTP id 4922F128003 for ; Thu, 18 Mar 2010 09:33:06 -0400 (EDT) Received: from warthog.cambridge.redhat.com (kibblesnbits.boston.devel.redhat.com [10.16.60.12]) by int-mx03.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o2IDX35W021809 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 18 Mar 2010 09:33:05 -0400 Received: from [127.0.0.1] (helo=warthog.procyon.org.uk) by warthog.cambridge.redhat.com with esmtp (Exim 4.69 #1 (Red Hat Linux)) id 1NsFqQ-0007kP-E5; Thu, 18 Mar 2010 13:33:02 +0000 Organization: Red Hat UK Ltd. Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SI4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 3798903 From: David Howells Subject: [PATCH] NFS: Fix RCU warnings in nfs_inode_return_delegation_noreclaim() [ver #2] To: Trond.Myklebust@netapp.com, paulmck@linux.vnet.ibm.com Cc: linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org, David Howells Date: Thu, 18 Mar 2010 13:33:02 +0000 Message-ID: <20100318133302.29754.1584.stgit@warthog.procyon.org.uk> User-Agent: StGIT/0.14.3 MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.67 on 10.5.11.16 Fix a number of RCU warnings in nfs_inode_return_delegation_noreclaim(). nfs_inode_return_delegation_noreclaim() and nfs_inode_return_delegation() don't need to use rcu_dereference() outside the spinlocked region as they merely examin the pointer and don't follow it, thus rendering unnecessary the need to impose a partial ordering over the one item of interest. nfs_detach_delegation_locked() doesn't need rcu_derefence() because it can only be called if nfs_client::cl_lock is held, and that guards against anyone changing nfsi->delegation under it. Furthermore, the barrier in rcu_derefence() is superfluous, given that the spin_lock() is also a barrier. nfs_free_delegation() should be using rcu_dereference_check() to validate the state that the data is in (the delegation inode must have been cleared). By this point, the delegation is being released, so no one else should be attempting to use the saved credentials, and they can be cleared. However, rcu_assign_pointer() should be used to clear them, and the delegation itself must still use call_rcu() as the list of delegations could be being traversed. [ INFO: suspicious rcu_dereference_check() usage. ] --------------------------------------------------- fs/nfs/delegation.c:332 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 mount.nfs4/2281: #0: (&type->s_umount_key#34){+.+...}, at: [] deactivate_super+0x60/0x80 #1: (iprune_sem){+.+...}, at: [] invalidate_inodes+0x39/0x13a stack backtrace: Pid: 2281, comm: mount.nfs4 Not tainted 2.6.34-rc1-cachefs #110 Call Trace: [] lockdep_rcu_dereference+0xaa/0xb2 [] nfs_inode_return_delegation_noreclaim+0x5b/0xa0 [nfs] [] nfs4_clear_inode+0x11/0x1e [nfs] [] clear_inode+0x9e/0xf8 [] dispose_list+0x67/0x10e [] invalidate_inodes+0x11c/0x13a [] generic_shutdown_super+0x42/0xf4 [] kill_anon_super+0x11/0x4f [] nfs4_kill_super+0x3f/0x72 [nfs] [] deactivate_super+0x68/0x80 [] mntput_no_expire+0xbb/0xf8 [] release_mounts+0x9a/0xb0 [] put_mnt_ns+0x6a/0x79 [] nfs_follow_remote_path+0x5a/0x146 [nfs] [] ? nfs_do_root_mount+0x82/0x95 [nfs] [] nfs4_try_mount+0x75/0xaf [nfs] [] nfs4_get_sb+0x291/0x31a [nfs] [] vfs_kern_mount+0xb8/0x177 [] do_kern_mount+0x48/0xe8 [] do_mount+0x782/0x7f9 [] sys_mount+0x83/0xbe [] system_call_fastpath+0x16/0x1b Also on: fs/nfs/delegation.c:215 invoked rcu_dereference_check() without protection! [] lockdep_rcu_dereference+0xaa/0xb2 [] nfs_inode_set_delegation+0xfe/0x219 [nfs] [] nfs4_opendata_to_nfs4_state+0x2c2/0x30d [nfs] [] nfs4_do_open+0x2a6/0x3a6 [nfs] ... And: fs/nfs/delegation.c:40 invoked rcu_dereference_check() without protection! [] lockdep_rcu_dereference+0xaa/0xb2 [] nfs_free_delegation+0x3d/0x6e [nfs] [] nfs_do_return_delegation+0x26/0x30 [nfs] [] __nfs_inode_return_delegation+0x1ef/0x1fe [nfs] [] nfs_client_return_marked_delegations+0xc9/0x124 [nfs] ... Signed-off-by: David Howells --- fs/nfs/delegation.c | 11 ++++++----- 1 files changed, 6 insertions(+), 5 deletions(-) diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c index 2563beb..fa9b7c5 100644 --- a/fs/nfs/delegation.c +++ b/fs/nfs/delegation.c @@ -37,7 +37,8 @@ static void nfs_free_delegation(struct nfs_delegation *delegation) { struct rpc_cred *cred; - cred = rcu_dereference(delegation->cred); + cred = rcu_dereference_check(delegation->cred, + delegation->inode == NULL); rcu_assign_pointer(delegation->cred, NULL); call_rcu(&delegation->rcu, nfs_free_delegation_callback); if (cred) @@ -167,7 +168,7 @@ static struct inode *nfs_delegation_grab_inode(struct nfs_delegation *delegation static struct nfs_delegation *nfs_detach_delegation_locked(struct nfs_inode *nfsi, const nfs4_stateid *stateid) { - struct nfs_delegation *delegation = rcu_dereference(nfsi->delegation); + struct nfs_delegation *delegation = nfsi->delegation; if (delegation == NULL) goto nomatch; @@ -212,7 +213,7 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct spin_lock_init(&delegation->lock); spin_lock(&clp->cl_lock); - if (rcu_dereference(nfsi->delegation) != NULL) { + if (nfsi->delegation != NULL) { if (memcmp(&delegation->stateid, &nfsi->delegation->stateid, sizeof(delegation->stateid)) == 0 && delegation->type == nfsi->delegation->type) { @@ -329,7 +330,7 @@ void nfs_inode_return_delegation_noreclaim(struct inode *inode) struct nfs_inode *nfsi = NFS_I(inode); struct nfs_delegation *delegation; - if (rcu_dereference(nfsi->delegation) != NULL) { + if (nfsi->delegation != NULL) { spin_lock(&clp->cl_lock); delegation = nfs_detach_delegation_locked(nfsi, NULL); spin_unlock(&clp->cl_lock); @@ -345,7 +346,7 @@ int nfs_inode_return_delegation(struct inode *inode) struct nfs_delegation *delegation; int err = 0; - if (rcu_dereference(nfsi->delegation) != NULL) { + if (nfsi->delegation != NULL) { spin_lock(&clp->cl_lock); delegation = nfs_detach_delegation_locked(nfsi, NULL); spin_unlock(&clp->cl_lock); --=-=-= Content-Type: message/rfc822 Content-Disposition: attachment; filename=10889 Return-Path: paulmck@paulmck-laptop.linux.vnet.ibm.com Received: from mail.corp.redhat.com [10.5.5.52] by warthog.procyon.org.uk with IMAP (fetchmail-6.3.11) for (single-drop); Fri, 19 Mar 2010 02:26:05 +0000 (GMT) Received: from zmta02.collab.prod.int.phx2.redhat.com (LHLO zmta02.collab.prod.int.phx2.redhat.com) (10.5.5.32) by mail06.corp.redhat.com with LMTP; Thu, 18 Mar 2010 22:25:41 -0400 (EDT) Received: from localhost (localhost.localdomain [127.0.0.1]) by zmta02.collab.prod.int.phx2.redhat.com (Postfix) with ESMTP id E31519E6CC for ; Thu, 18 Mar 2010 22:25:41 -0400 (EDT) Received: from zmta02.collab.prod.int.phx2.redhat.com ([127.0.0.1]) by localhost (zmta02.collab.prod.int.phx2.redhat.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id zc8aqHS8mlKB for ; Thu, 18 Mar 2010 22:25:41 -0400 (EDT) Received: from int-mx04.intmail.prod.int.phx2.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.17]) by zmta02.collab.prod.int.phx2.redhat.com (Postfix) with ESMTP id C14609E6CB for ; Thu, 18 Mar 2010 22:25:41 -0400 (EDT) Received: from mx1.redhat.com (ext-mx04.extmail.prod.ext.phx2.redhat.com [10.5.110.8]) by int-mx04.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o2J2Pfcr013945 for ; Thu, 18 Mar 2010 22:25:41 -0400 Received: from e5.ny.us.ibm.com (e5.ny.us.ibm.com [32.97.182.145]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o2J2PUlC025727 for ; Thu, 18 Mar 2010 22:25:30 -0400 Received: from d01relay06.pok.ibm.com (d01relay06.pok.ibm.com [9.56.227.116]) by e5.ny.us.ibm.com (8.14.3/8.13.1) with ESMTP id o2J2BB7V009683 for ; Thu, 18 Mar 2010 22:11:11 -0400 Received: from d01av04.pok.ibm.com (d01av04.pok.ibm.com [9.56.224.64]) by d01relay06.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id o2J2PTPk1245220 for ; Thu, 18 Mar 2010 22:25:29 -0400 Received: from d01av04.pok.ibm.com (loopback [127.0.0.1]) by d01av04.pok.ibm.com (8.14.3/8.13.1/NCO v10.0 AVout) with ESMTP id o2J2PTjt020272 for ; Thu, 18 Mar 2010 22:25:29 -0400 Received: from paulmck-laptop.localdomain (sig-9-65-43-134.mts.ibm.com [9.65.43.134]) by d01av04.pok.ibm.com (8.14.3/8.13.1/NCO v10.0 AVin) with ESMTP id o2J2PSNA020249; Thu, 18 Mar 2010 22:25:28 -0400 Received: by paulmck-laptop.localdomain (Postfix, from userid 1000) id CC80118179B; Thu, 18 Mar 2010 19:25:27 -0700 (PDT) Date: Thu, 18 Mar 2010 19:25:27 -0700 From: "Paul E. McKenney" To: David Howells Cc: Trond.Myklebust@netapp.com, linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] NFS: Fix RCU warnings in nfs_inode_return_delegation_noreclaim() [ver #2] Message-ID: <20100319022527.GC2894@linux.vnet.ibm.com> Reply-To: paulmck@linux.vnet.ibm.com References: <20100318133302.29754.1584.stgit@warthog.procyon.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20100318133302.29754.1584.stgit@warthog.procyon.org.uk> User-Agent: Mutt/1.5.20 (2009-06-14) X-RedHat-Spam-Score: -2.31 (RCVD_IN_DNSWL_MED,T_RP_MATCHES_RCVD) X-Scanned-By: MIMEDefang 2.67 on 10.5.11.17 X-Scanned-By: MIMEDefang 2.67 on 10.5.110.8 On Thu, Mar 18, 2010 at 01:33:02PM +0000, David Howells wrote: > Fix a number of RCU warnings in nfs_inode_return_delegation_noreclaim(). > nfs_inode_return_delegation_noreclaim() and nfs_inode_return_delegation() don't > need to use rcu_dereference() outside the spinlocked region as they merely > examin the pointer and don't follow it, thus rendering unnecessary the need to > impose a partial ordering over the one item of interest. > > nfs_detach_delegation_locked() doesn't need rcu_derefence() because it can only > be called if nfs_client::cl_lock is held, and that guards against anyone > changing nfsi->delegation under it. Furthermore, the barrier in > rcu_derefence() is superfluous, given that the spin_lock() is also a barrier. > > nfs_free_delegation() should be using rcu_dereference_check() to validate the > state that the data is in (the delegation inode must have been cleared). By > this point, the delegation is being released, so no one else should be > attempting to use the saved credentials, and they can be cleared. However, > rcu_assign_pointer() should be used to clear them, and the delegation itself > must still use call_rcu() as the list of delegations could be being traversed. Thank you for fixing these up! This looks good at the moment, however, the sparse changes that Arnd Bergmann is working on will invalidate a couple of the changes below. Of course, better a future problem than a here-and-now problem, but is there an easy way to fix both? Thanx, Paul > [ INFO: suspicious rcu_dereference_check() usage. ] > --------------------------------------------------- > fs/nfs/delegation.c:332 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 mount.nfs4/2281: > #0: (&type->s_umount_key#34){+.+...}, at: [] deactivate_super+0x60/0x80 > #1: (iprune_sem){+.+...}, at: [] invalidate_inodes+0x39/0x13a > > stack backtrace: > Pid: 2281, comm: mount.nfs4 Not tainted 2.6.34-rc1-cachefs #110 > Call Trace: > [] lockdep_rcu_dereference+0xaa/0xb2 > [] nfs_inode_return_delegation_noreclaim+0x5b/0xa0 [nfs] > [] nfs4_clear_inode+0x11/0x1e [nfs] > [] clear_inode+0x9e/0xf8 > [] dispose_list+0x67/0x10e > [] invalidate_inodes+0x11c/0x13a > [] generic_shutdown_super+0x42/0xf4 > [] kill_anon_super+0x11/0x4f > [] nfs4_kill_super+0x3f/0x72 [nfs] > [] deactivate_super+0x68/0x80 > [] mntput_no_expire+0xbb/0xf8 > [] release_mounts+0x9a/0xb0 > [] put_mnt_ns+0x6a/0x79 > [] nfs_follow_remote_path+0x5a/0x146 [nfs] > [] ? nfs_do_root_mount+0x82/0x95 [nfs] > [] nfs4_try_mount+0x75/0xaf [nfs] > [] nfs4_get_sb+0x291/0x31a [nfs] > [] vfs_kern_mount+0xb8/0x177 > [] do_kern_mount+0x48/0xe8 > [] do_mount+0x782/0x7f9 > [] sys_mount+0x83/0xbe > [] system_call_fastpath+0x16/0x1b > > Also on: > > fs/nfs/delegation.c:215 invoked rcu_dereference_check() without protection! > [] lockdep_rcu_dereference+0xaa/0xb2 > [] nfs_inode_set_delegation+0xfe/0x219 [nfs] > [] nfs4_opendata_to_nfs4_state+0x2c2/0x30d [nfs] > [] nfs4_do_open+0x2a6/0x3a6 [nfs] > ... > > And: > > fs/nfs/delegation.c:40 invoked rcu_dereference_check() without protection! > [] lockdep_rcu_dereference+0xaa/0xb2 > [] nfs_free_delegation+0x3d/0x6e [nfs] > [] nfs_do_return_delegation+0x26/0x30 [nfs] > [] __nfs_inode_return_delegation+0x1ef/0x1fe [nfs] > [] nfs_client_return_marked_delegations+0xc9/0x124 [nfs] > ... > > > Signed-off-by: David Howells > --- > > fs/nfs/delegation.c | 11 ++++++----- > 1 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c > index 2563beb..fa9b7c5 100644 > --- a/fs/nfs/delegation.c > +++ b/fs/nfs/delegation.c > @@ -37,7 +37,8 @@ static void nfs_free_delegation(struct nfs_delegation *delegation) > { > struct rpc_cred *cred; > > - cred = rcu_dereference(delegation->cred); > + cred = rcu_dereference_check(delegation->cred, > + delegation->inode == NULL); > rcu_assign_pointer(delegation->cred, NULL); > call_rcu(&delegation->rcu, nfs_free_delegation_callback); > if (cred) > @@ -167,7 +168,7 @@ static struct inode *nfs_delegation_grab_inode(struct nfs_delegation *delegation > > static struct nfs_delegation *nfs_detach_delegation_locked(struct nfs_inode *nfsi, const nfs4_stateid *stateid) > { > - struct nfs_delegation *delegation = rcu_dereference(nfsi->delegation); > + struct nfs_delegation *delegation = nfsi->delegation; Arnd's work will flag this one. > > if (delegation == NULL) > goto nomatch; > @@ -212,7 +213,7 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct > spin_lock_init(&delegation->lock); > > spin_lock(&clp->cl_lock); > - if (rcu_dereference(nfsi->delegation) != NULL) { > + if (nfsi->delegation != NULL) { And this one. I thought that Trond said that clp->cl_lock protects this one, in which case this should work: if (rcu_dereference_check(nfsi->delegation, lockdep_is_held(&clp->cl_lock)) != NULL) { > if (memcmp(&delegation->stateid, &nfsi->delegation->stateid, > sizeof(delegation->stateid)) == 0 && > delegation->type == nfsi->delegation->type) { > @@ -329,7 +330,7 @@ void nfs_inode_return_delegation_noreclaim(struct inode *inode) > struct nfs_inode *nfsi = NFS_I(inode); > struct nfs_delegation *delegation; > > - if (rcu_dereference(nfsi->delegation) != NULL) { > + if (nfsi->delegation != NULL) { And this one, although the check for cp->cl_lock obviously won't work here. > spin_lock(&clp->cl_lock); > delegation = nfs_detach_delegation_locked(nfsi, NULL); > spin_unlock(&clp->cl_lock); > @@ -345,7 +346,7 @@ int nfs_inode_return_delegation(struct inode *inode) > struct nfs_delegation *delegation; > int err = 0; > > - if (rcu_dereference(nfsi->delegation) != NULL) { > + if (nfsi->delegation != NULL) { Ditto... > spin_lock(&clp->cl_lock); > delegation = nfs_detach_delegation_locked(nfsi, NULL); > spin_unlock(&clp->cl_lock); > --=-=-=-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/