lkml.org 
[lkml]   [2009]   [Jul]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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


    \
     
     \ /
      Last update: 2009-07-02 21:23    [W:0.043 / U:59.168 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site