| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1268744823.3155.15.camel@localhost.localdomain>
Date: Tue, 16 Mar 2010 09:07:03 -0400
From: Trond Myklebust <Trond.Myklebust@...app.com>
To: David Howells <dhowells@...hat.com>
Cc: linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] NFS: Fix RCU warnings in
nfs_inode_return_delegation_noreclaim()
On Tue, 2010-03-16 at 11:51 +0000, David Howells wrote:
> Fix a number of RCU warnings in nfs_inode_return_delegation_noreclaim(). They
> look like simple cases of missing rcu_read_lock/unlock() calls.
>
> ===================================================
> [ 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: [<ffffffff810b25b4>] deactivate_super+0x60/0x80
> #1: (iprune_sem){+.+...}, at: [<ffffffff810c332a>] invalidate_inodes+0x39/0x13a
>
> stack backtrace:
> Pid: 2281, comm: mount.nfs4 Not tainted 2.6.34-rc1-cachefs #110
> Call Trace:
> [<ffffffff8105149f>] lockdep_rcu_dereference+0xaa/0xb2
> [<ffffffffa00b4591>] nfs_inode_return_delegation_noreclaim+0x5b/0xa0 [nfs]
> [<ffffffffa0095d63>] nfs4_clear_inode+0x11/0x1e [nfs]
> [<ffffffff810c2d92>] clear_inode+0x9e/0xf8
> [<ffffffff810c3028>] dispose_list+0x67/0x10e
> [<ffffffff810c340d>] invalidate_inodes+0x11c/0x13a
> [<ffffffff810b1dc1>] generic_shutdown_super+0x42/0xf4
> [<ffffffff810b1ebe>] kill_anon_super+0x11/0x4f
> [<ffffffffa009893c>] nfs4_kill_super+0x3f/0x72 [nfs]
> [<ffffffff810b25bc>] deactivate_super+0x68/0x80
> [<ffffffff810c6744>] mntput_no_expire+0xbb/0xf8
> [<ffffffff810c681b>] release_mounts+0x9a/0xb0
> [<ffffffff810c689b>] put_mnt_ns+0x6a/0x79
> [<ffffffffa00983a1>] nfs_follow_remote_path+0x5a/0x146 [nfs]
> [<ffffffffa0098334>] ? nfs_do_root_mount+0x82/0x95 [nfs]
> [<ffffffffa00985a9>] nfs4_try_mount+0x75/0xaf [nfs]
> [<ffffffffa0098874>] nfs4_get_sb+0x291/0x31a [nfs]
> [<ffffffff810b2059>] vfs_kern_mount+0xb8/0x177
> [<ffffffff810b2176>] do_kern_mount+0x48/0xe8
> [<ffffffff810c810b>] do_mount+0x782/0x7f9
> [<ffffffff810c8205>] sys_mount+0x83/0xbe
> [<ffffffff81001eeb>] system_call_fastpath+0x16/0x1b
>
> Also on:
>
> fs/nfs/delegation.c:215 invoked rcu_dereference_check() without protection!
> [<ffffffff8105149f>] lockdep_rcu_dereference+0xaa/0xb2
> [<ffffffffa00b4223>] nfs_inode_set_delegation+0xfe/0x219 [nfs]
> [<ffffffffa00a9c6f>] nfs4_opendata_to_nfs4_state+0x2c2/0x30d [nfs]
> [<ffffffffa00aa15d>] nfs4_do_open+0x2a6/0x3a6 [nfs]
> ...
>
> And:
>
> fs/nfs/delegation.c:40 invoked rcu_dereference_check() without protection!
> [<ffffffff8105149f>] lockdep_rcu_dereference+0xaa/0xb2
> [<ffffffffa00b3bef>] nfs_free_delegation+0x3d/0x6e [nfs]
> [<ffffffffa00b3e71>] nfs_do_return_delegation+0x26/0x30 [nfs]
> [<ffffffffa00b406a>] __nfs_inode_return_delegation+0x1ef/0x1fe [nfs]
> [<ffffffffa00b448a>] nfs_client_return_marked_delegations+0xc9/0x124 [nfs]
> ...
>
> Signed-off-by: David Howells <dhowells@...hat.com>
> ---
>
> fs/nfs/delegation.c | 15 ++++++++++++---
> 1 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> index 2563beb..a77c735 100644
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c
> @@ -37,8 +37,10 @@ static void nfs_free_delegation(struct nfs_delegation *delegation)
> {
> struct rpc_cred *cred;
>
> + rcu_read_lock();
> cred = rcu_dereference(delegation->cred);
> rcu_assign_pointer(delegation->cred, NULL);
> + rcu_read_unlock();
> call_rcu(&delegation->rcu, nfs_free_delegation_callback);
> if (cred)
> put_rpccred(cred);
That's bogus. We're in the process of freeing the delegation, so we
don't need to rely on rcu to read delegation->cred.
Better to just convert that rcu_dereference() into an ordinary pointer
dereference.
> @@ -212,10 +214,12 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
> spin_lock_init(&delegation->lock);
>
> spin_lock(&clp->cl_lock);
> + rcu_read_lock();
> if (rcu_dereference(nfsi->delegation) != NULL) {
> if (memcmp(&delegation->stateid, &nfsi->delegation->stateid,
> sizeof(delegation->stateid)) == 0 &&
> delegation->type == nfsi->delegation->type) {
> + rcu_read_unlock();
> goto out;
> }
> /*
The spinlock already provides protection. Again we can just convert the
rcu_dereference() into a pointer dereference.
> @@ -228,6 +232,7 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
> if (delegation->type <= nfsi->delegation->type) {
> freeme = delegation;
> delegation = NULL;
> + rcu_read_lock();
> goto out;
> }
> freeme = nfs_detach_delegation_locked(nfsi, NULL);
> @@ -236,6 +241,7 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
> nfsi->delegation_state = delegation->type;
> rcu_assign_pointer(nfsi->delegation, delegation);
> delegation = NULL;
> + rcu_read_unlock();
>
> /* Ensure we revalidate the attributes and page cache! */
> spin_lock(&inode->i_lock);
> @@ -327,15 +333,18 @@ void nfs_inode_return_delegation_noreclaim(struct inode *inode)
> {
> struct nfs_client *clp = NFS_SERVER(inode)->nfs_client;
> struct nfs_inode *nfsi = NFS_I(inode);
> - struct nfs_delegation *delegation;
> + struct nfs_delegation *delegation = NULL;
>
> + rcu_read_lock();
> if (rcu_dereference(nfsi->delegation) != NULL) {
> spin_lock(&clp->cl_lock);
> delegation = nfs_detach_delegation_locked(nfsi, NULL);
> spin_unlock(&clp->cl_lock);
> - if (delegation != NULL)
> - nfs_do_return_delegation(inode, delegation, 0);
> }
> + rcu_read_unlock();
We cannot hold the rcu read lock across the entire RPC call in
nfs_do_return_delegation(). All we want to do above is to check that
nfsi->delegation != NULL.
> +
> + if (delegation)
> + nfs_do_return_delegation(inode, delegation, 0);
> }
>
> int nfs_inode_return_delegation(struct inode *inode)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists