Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753196AbZGBTVS (ORCPT ); Thu, 2 Jul 2009 15:21:18 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751983AbZGBTVE (ORCPT ); Thu, 2 Jul 2009 15:21:04 -0400 Received: from e39.co.us.ibm.com ([32.97.110.160]:43874 "EHLO e39.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751782AbZGBTVD (ORCPT ); Thu, 2 Jul 2009 15:21:03 -0400 Message-ID: <4A4D0897.6060504@linux.vnet.ibm.com> Date: Thu, 02 Jul 2009 14:20:55 -0500 From: Tyler Hicks User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1b3pre) Gecko/20090513 Fedora/3.0-2.3.beta2.fc11 Thunderbird/3.0b2 MIME-Version: 1.0 To: Roland Dreier CC: kirkland@canonical.com, ecryptfs-devel@lists.launchpad.net, linux-kernel@vger.kernel.org Subject: Re: [PATCH] eCryptfs: Fix lockdep-reported AB-BA mutex issue References: In-Reply-To: X-Enigmail-Version: 0.96a OpenPGP: id=5D35E502 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9696 Lines: 217 On 07/01/2009 05:48 PM, Roland Dreier wrote: > 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. After looking at the code paths from a slightly higher level, I don't think deadlock would ever occur from this issue. But, there's no need in keeping it the way it is, especially with it disabling 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). Looks like a reasonable approach. > > 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; This patch looks good. I really appreciate you tracking down and fixing this problem. It will take me a little bit longer before I can get to the other 2 patches. Tyler -- 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/