Messages in this thread | | | Date | Thu, 02 Jul 2009 14:20:55 -0500 | From | Tyler Hicks <> | Subject | Re: [PATCH] eCryptfs: Fix lockdep-reported AB-BA mutex issue |
| |
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: [<ffffffff8121591e>] ecryptfs_find_global_auth_tok_for_sig+0x2e/0x90 > > but task is already holding lock: > (&crypt_stat->keysig_list_mutex){+.+.+.}, at: [<ffffffff81217728>] 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){+.+.+.}: > [<ffffffff8108c897>] check_prev_add+0x2a7/0x370 > [<ffffffff8108cfc1>] validate_chain+0x661/0x750 > [<ffffffff8108d2e7>] __lock_acquire+0x237/0x430 > [<ffffffff8108d585>] lock_acquire+0xa5/0x150 > [<ffffffff815526cd>] __mutex_lock_common+0x4d/0x3d0 > [<ffffffff81552b56>] mutex_lock_nested+0x46/0x60 > [<ffffffff8121526a>] ecryptfs_add_keysig+0x5a/0xb0 > [<ffffffff81213299>] ecryptfs_copy_mount_wide_sigs_to_inode_sigs+0x59/0xb0 > [<ffffffff81214b06>] ecryptfs_new_file_context+0xa6/0x1a0 > [<ffffffff8120e42a>] ecryptfs_initialize_file+0x4a/0x140 > [<ffffffff8120e54d>] ecryptfs_create+0x2d/0x60 > [<ffffffff8113a7d4>] vfs_create+0xb4/0xe0 > [<ffffffff8113a8c4>] __open_namei_create+0xc4/0x110 > [<ffffffff8113d1c1>] do_filp_open+0xa01/0xae0 > [<ffffffff8112d8d9>] do_sys_open+0x69/0x140 > [<ffffffff8112d9f0>] sys_open+0x20/0x30 > [<ffffffff81013132>] system_call_fastpath+0x16/0x1b > [<ffffffffffffffff>] 0xffffffffffffffff > > -> #0 (&mount_crypt_stat->global_auth_tok_list_mutex){+.+.+.}: > [<ffffffff8108c675>] check_prev_add+0x85/0x370 > [<ffffffff8108cfc1>] validate_chain+0x661/0x750 > [<ffffffff8108d2e7>] __lock_acquire+0x237/0x430 > [<ffffffff8108d585>] lock_acquire+0xa5/0x150 > [<ffffffff815526cd>] __mutex_lock_common+0x4d/0x3d0 > [<ffffffff81552b56>] mutex_lock_nested+0x46/0x60 > [<ffffffff8121591e>] ecryptfs_find_global_auth_tok_for_sig+0x2e/0x90 > [<ffffffff812177d5>] ecryptfs_generate_key_packet_set+0x105/0x2b0 > [<ffffffff81212f49>] ecryptfs_write_headers_virt+0xc9/0x120 > [<ffffffff8121306d>] ecryptfs_write_metadata+0xcd/0x200 > [<ffffffff8120e44b>] ecryptfs_initialize_file+0x6b/0x140 > [<ffffffff8120e54d>] ecryptfs_create+0x2d/0x60 > [<ffffffff8113a7d4>] vfs_create+0xb4/0xe0 > [<ffffffff8113a8c4>] __open_namei_create+0xc4/0x110 > [<ffffffff8113d1c1>] do_filp_open+0xa01/0xae0 > [<ffffffff8112d8d9>] do_sys_open+0x69/0x140 > [<ffffffff8112d9f0>] sys_open+0x20/0x30 > [<ffffffff81013132>] system_call_fastpath+0x16/0x1b > [<ffffffffffffffff>] 0xffffffffffffffff > > other info that might help us debug this: > > 2 locks held by gdm/2640: > #0: (&sb->s_type->i_mutex_key#11){+.+.+.}, at: [<ffffffff8113cb8b>] do_filp_open+0x3cb/0xae0 > #1: (&crypt_stat->keysig_list_mutex){+.+.+.}, at: [<ffffffff81217728>] 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: > [<ffffffff8108b988>] print_circular_bug_tail+0xa8/0xf0 > [<ffffffff8108c675>] check_prev_add+0x85/0x370 > [<ffffffff81094912>] ? __module_text_address+0x12/0x60 > [<ffffffff8108cfc1>] validate_chain+0x661/0x750 > [<ffffffff81017275>] ? print_context_stack+0x85/0x140 > [<ffffffff81089c68>] ? find_usage_backwards+0x38/0x160 > [<ffffffff8108d2e7>] __lock_acquire+0x237/0x430 > [<ffffffff8108d585>] lock_acquire+0xa5/0x150 > [<ffffffff8121591e>] ? ecryptfs_find_global_auth_tok_for_sig+0x2e/0x90 > [<ffffffff8108b0b0>] ? check_usage_backwards+0x0/0xb0 > [<ffffffff815526cd>] __mutex_lock_common+0x4d/0x3d0 > [<ffffffff8121591e>] ? ecryptfs_find_global_auth_tok_for_sig+0x2e/0x90 > [<ffffffff8121591e>] ? ecryptfs_find_global_auth_tok_for_sig+0x2e/0x90 > [<ffffffff8108c02c>] ? mark_held_locks+0x6c/0xa0 > [<ffffffff81125b0d>] ? kmem_cache_alloc+0xfd/0x1a0 > [<ffffffff8108c34d>] ? trace_hardirqs_on_caller+0x14d/0x190 > [<ffffffff81552b56>] mutex_lock_nested+0x46/0x60 > [<ffffffff8121591e>] ecryptfs_find_global_auth_tok_for_sig+0x2e/0x90 > [<ffffffff812177d5>] ecryptfs_generate_key_packet_set+0x105/0x2b0 > [<ffffffff81212f49>] ecryptfs_write_headers_virt+0xc9/0x120 > [<ffffffff8121306d>] ecryptfs_write_metadata+0xcd/0x200 > [<ffffffff81210240>] ? ecryptfs_init_persistent_file+0x60/0xe0 > [<ffffffff8120e44b>] ecryptfs_initialize_file+0x6b/0x140 > [<ffffffff8120e54d>] ecryptfs_create+0x2d/0x60 > [<ffffffff8113a7d4>] vfs_create+0xb4/0xe0 > [<ffffffff8113a8c4>] __open_namei_create+0xc4/0x110 > [<ffffffff8113d1c1>] do_filp_open+0xa01/0xae0 > [<ffffffff8129a93e>] ? _raw_spin_unlock+0x5e/0xb0 > [<ffffffff8155410b>] ? _spin_unlock+0x2b/0x40 > [<ffffffff81139e9b>] ? getname+0x3b/0x240 > [<ffffffff81148a5a>] ? alloc_fd+0xfa/0x140 > [<ffffffff8112d8d9>] do_sys_open+0x69/0x140 > [<ffffffff81553b8f>] ? trace_hardirqs_on_thunk+0x3a/0x3f > [<ffffffff8112d9f0>] sys_open+0x20/0x30 > [<ffffffff81013132>] system_call_fastpath+0x16/0x1b > > Signed-off-by: Roland Dreier <rolandd@cisco.com> > --- > 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
| |