Re: [PATCH] eCryptfs: Fix lockdep-reported AB-BA mutex issue

[Tyler Hicks <tyhicks@linux.vnet.ibm.com>]

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