Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756039AbZGAWs1 (ORCPT ); Wed, 1 Jul 2009 18:48:27 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752612AbZGAWsU (ORCPT ); Wed, 1 Jul 2009 18:48:20 -0400 Received: from sj-iport-6.cisco.com ([171.71.176.117]:56355 "EHLO sj-iport-6.cisco.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752450AbZGAWsT (ORCPT ); Wed, 1 Jul 2009 18:48:19 -0400 X-IronPort-AV: E=Sophos;i="4.42,328,1243814400"; d="scan'208";a="335816481" From: Roland Dreier To: tyhicks@linux.vnet.ibm.com, kirkland@canonical.com, ecryptfs-devel@lists.launchpad.net Cc: linux-kernel@vger.kernel.org Subject: [PATCH] eCryptfs: Fix lockdep-reported AB-BA mutex issue References: X-Message-Flag: Warning: May contain useful information Date: Wed, 01 Jul 2009 15:48:18 -0700 In-Reply-To: (Roland Dreier's message of "Wed, 01 Jul 2009 10:06:33 -0700") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.0.91 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-OriginalArrivalTime: 01 Jul 2009 22:48:19.0162 (UTC) FILETIME=[0755F3A0:01C9FA9E] Authentication-Results: sj-dkim-3; header.From=rdreier@cisco.com; dkim=pass ( sig from cisco.com/sjdkim3002 verified; ); Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8832 Lines: 202 Lockdep reports the following valid-looking possible AB-BA deadlock with global_auth_tok_list_mutex and keysig_list_mutex: ecryptfs_new_file_context() -> ecryptfs_copy_mount_wide_sigs_to_inode_sigs() -> mutex_lock(&mount_crypt_stat->global_auth_tok_list_mutex); -> ecryptfs_add_keysig() -> mutex_lock(&crypt_stat->keysig_list_mutex); vs ecryptfs_generate_key_packet_set() -> mutex_lock(&crypt_stat->keysig_list_mutex); -> ecryptfs_find_global_auth_tok_for_sig() -> mutex_lock(&mount_crypt_stat->global_auth_tok_list_mutex); ie the two mutexes are taken in opposite orders in the two different code paths. I'm not sure if this is a real bug where two threads could actually hit the two paths in parallel and deadlock, but it at least makes lockdep impossible to use with ecryptfs since this report triggers every time and disables future lockdep reporting. Since ecryptfs_add_keysig() is called only from the single callsite in ecryptfs_copy_mount_wide_sigs_to_inode_sigs(), the simplest fix seems to be to move the lock of keysig_list_mutex back up outside of the where global_auth_tok_list_mutex is taken. This patch does that, and fixes the lockdep report on my system (and ecryptfs still works OK). The full output of lockdep fixed by this patch is: ======================================================= [ INFO: possible circular locking dependency detected ] 2.6.31-2-generic #14~rbd2 ------------------------------------------------------- gdm/2640 is trying to acquire lock: (&mount_crypt_stat->global_auth_tok_list_mutex){+.+.+.}, at: [] ecryptfs_find_global_auth_tok_for_sig+0x2e/0x90 but task is already holding lock: (&crypt_stat->keysig_list_mutex){+.+.+.}, at: [] ecryptfs_generate_key_packet_set+0x58/0x2b0 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #1 (&crypt_stat->keysig_list_mutex){+.+.+.}: [] check_prev_add+0x2a7/0x370 [] validate_chain+0x661/0x750 [] __lock_acquire+0x237/0x430 [] lock_acquire+0xa5/0x150 [] __mutex_lock_common+0x4d/0x3d0 [] mutex_lock_nested+0x46/0x60 [] ecryptfs_add_keysig+0x5a/0xb0 [] ecryptfs_copy_mount_wide_sigs_to_inode_sigs+0x59/0xb0 [] ecryptfs_new_file_context+0xa6/0x1a0 [] ecryptfs_initialize_file+0x4a/0x140 [] ecryptfs_create+0x2d/0x60 [] vfs_create+0xb4/0xe0 [] __open_namei_create+0xc4/0x110 [] do_filp_open+0xa01/0xae0 [] do_sys_open+0x69/0x140 [] sys_open+0x20/0x30 [] system_call_fastpath+0x16/0x1b [] 0xffffffffffffffff -> #0 (&mount_crypt_stat->global_auth_tok_list_mutex){+.+.+.}: [] check_prev_add+0x85/0x370 [] validate_chain+0x661/0x750 [] __lock_acquire+0x237/0x430 [] lock_acquire+0xa5/0x150 [] __mutex_lock_common+0x4d/0x3d0 [] mutex_lock_nested+0x46/0x60 [] ecryptfs_find_global_auth_tok_for_sig+0x2e/0x90 [] ecryptfs_generate_key_packet_set+0x105/0x2b0 [] ecryptfs_write_headers_virt+0xc9/0x120 [] ecryptfs_write_metadata+0xcd/0x200 [] ecryptfs_initialize_file+0x6b/0x140 [] ecryptfs_create+0x2d/0x60 [] vfs_create+0xb4/0xe0 [] __open_namei_create+0xc4/0x110 [] do_filp_open+0xa01/0xae0 [] do_sys_open+0x69/0x140 [] sys_open+0x20/0x30 [] system_call_fastpath+0x16/0x1b [] 0xffffffffffffffff other info that might help us debug this: 2 locks held by gdm/2640: #0: (&sb->s_type->i_mutex_key#11){+.+.+.}, at: [] do_filp_open+0x3cb/0xae0 #1: (&crypt_stat->keysig_list_mutex){+.+.+.}, at: [] ecryptfs_generate_key_packet_set+0x58/0x2b0 stack backtrace: Pid: 2640, comm: gdm Tainted: G C 2.6.31-2-generic #14~rbd2 Call Trace: [] print_circular_bug_tail+0xa8/0xf0 [] check_prev_add+0x85/0x370 [] ? __module_text_address+0x12/0x60 [] validate_chain+0x661/0x750 [] ? print_context_stack+0x85/0x140 [] ? find_usage_backwards+0x38/0x160 [] __lock_acquire+0x237/0x430 [] lock_acquire+0xa5/0x150 [] ? ecryptfs_find_global_auth_tok_for_sig+0x2e/0x90 [] ? check_usage_backwards+0x0/0xb0 [] __mutex_lock_common+0x4d/0x3d0 [] ? ecryptfs_find_global_auth_tok_for_sig+0x2e/0x90 [] ? ecryptfs_find_global_auth_tok_for_sig+0x2e/0x90 [] ? mark_held_locks+0x6c/0xa0 [] ? kmem_cache_alloc+0xfd/0x1a0 [] ? trace_hardirqs_on_caller+0x14d/0x190 [] mutex_lock_nested+0x46/0x60 [] ecryptfs_find_global_auth_tok_for_sig+0x2e/0x90 [] ecryptfs_generate_key_packet_set+0x105/0x2b0 [] ecryptfs_write_headers_virt+0xc9/0x120 [] ecryptfs_write_metadata+0xcd/0x200 [] ? ecryptfs_init_persistent_file+0x60/0xe0 [] ecryptfs_initialize_file+0x6b/0x140 [] ecryptfs_create+0x2d/0x60 [] vfs_create+0xb4/0xe0 [] __open_namei_create+0xc4/0x110 [] do_filp_open+0xa01/0xae0 [] ? _raw_spin_unlock+0x5e/0xb0 [] ? _spin_unlock+0x2b/0x40 [] ? getname+0x3b/0x240 [] ? alloc_fd+0xfa/0x140 [] do_sys_open+0x69/0x140 [] ? trace_hardirqs_on_thunk+0x3a/0x3f [] sys_open+0x20/0x30 [] system_call_fastpath+0x16/0x1b Signed-off-by: Roland Dreier --- fs/ecryptfs/crypto.c | 8 +++++--- fs/ecryptfs/keystore.c | 11 ++++------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c index b91851f..1920a9a 100644 --- a/fs/ecryptfs/crypto.c +++ b/fs/ecryptfs/crypto.c @@ -925,7 +925,9 @@ static int ecryptfs_copy_mount_wide_sigs_to_inode_sigs( struct ecryptfs_global_auth_tok *global_auth_tok; int rc = 0; + mutex_lock(&crypt_stat->keysig_list_mutex); mutex_lock(&mount_crypt_stat->global_auth_tok_list_mutex); + list_for_each_entry(global_auth_tok, &mount_crypt_stat->global_auth_tok_list, mount_crypt_stat_list) { @@ -934,13 +936,13 @@ static int ecryptfs_copy_mount_wide_sigs_to_inode_sigs( rc = ecryptfs_add_keysig(crypt_stat, global_auth_tok->sig); if (rc) { printk(KERN_ERR "Error adding keysig; rc = [%d]\n", rc); - mutex_unlock( - &mount_crypt_stat->global_auth_tok_list_mutex); goto out; } } - mutex_unlock(&mount_crypt_stat->global_auth_tok_list_mutex); + out: + mutex_unlock(&mount_crypt_stat->global_auth_tok_list_mutex); + mutex_unlock(&crypt_stat->keysig_list_mutex); return rc; } diff --git a/fs/ecryptfs/keystore.c b/fs/ecryptfs/keystore.c index af737bb..e85ca8e 100644 --- a/fs/ecryptfs/keystore.c +++ b/fs/ecryptfs/keystore.c @@ -2353,21 +2353,18 @@ struct kmem_cache *ecryptfs_key_sig_cache; int ecryptfs_add_keysig(struct ecryptfs_crypt_stat *crypt_stat, char *sig) { struct ecryptfs_key_sig *new_key_sig; - int rc = 0; new_key_sig = kmem_cache_alloc(ecryptfs_key_sig_cache, GFP_KERNEL); if (!new_key_sig) { - rc = -ENOMEM; printk(KERN_ERR "Error allocating from ecryptfs_key_sig_cache\n"); - goto out; + return -ENOMEM; } memcpy(new_key_sig->keysig, sig, ECRYPTFS_SIG_SIZE_HEX); - mutex_lock(&crypt_stat->keysig_list_mutex); + /* Caller must hold keysig_list_mutex */ list_add(&new_key_sig->crypt_stat_list, &crypt_stat->keysig_list); - mutex_unlock(&crypt_stat->keysig_list_mutex); -out: - return rc; + + return 0; } struct kmem_cache *ecryptfs_global_auth_tok_cache; -- 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/