Re: [PATCH 2/2] eCryptfs: lock requested keys for writing

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

On Fri Jan 28, 2011 at 11:42:04AM +0100, Roberto Sassu <roberto.sassu@polito.it> wrote:
> Keys requested by eCryptfs are exclusively locked in order to prevent
> modifications by other subjects. In particular those which description is
> specified at mount time are locked until the filesystem is unmounted, these

I don't think the approach taken for the mount wide keys will work for a
couple reasons. The first is that lockdep reports this after a mount:

================================================
[ BUG: lock held when returning to user space! ]
------------------------------------------------
mount/1019 is leaving the kernel with locks still held!
1 lock held by mount/1019:
#0:  (&key->sem){+.+.+.}, at: [<ffffffffa021ff12>] ecryptfs_keyring_auth_tok_for_sig+0xe6/0x195 [ecryptfs]

The second is that while this does prevent key_update() from updating
the key underneath us, it just results in `keyctl update` on a mount key
to become a hung task until unmounting the eCryptfs mount.

It looks like we'll have to have more efficient locking for mount keys.

Tyler

> required to decrypt a single file are unlocked immediately after the open.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
> Reported-by: David Howells <dhowells@redhat.com>
> ---
>  fs/ecryptfs/crypto.c   |    4 +++-
>  fs/ecryptfs/keystore.c |   15 ++++++++++++---
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
> index bfd8b68..6b32776 100644
> --- a/fs/ecryptfs/crypto.c
> +++ b/fs/ecryptfs/crypto.c
> @@ -268,8 +268,10 @@ void ecryptfs_destroy_mount_crypt_stat(
>  		list_del(&auth_tok->mount_crypt_stat_list);
>  		mount_crypt_stat->num_global_auth_toks--;
>  		if (auth_tok->global_auth_tok_key
> -		    && !(auth_tok->flags & ECRYPTFS_AUTH_TOK_INVALID))
> +		    && !(auth_tok->flags & ECRYPTFS_AUTH_TOK_INVALID)) {
> +			up_write(&(auth_tok->global_auth_tok_key->sem));
>  			key_put(auth_tok->global_auth_tok_key);
> +		}
>  		kmem_cache_free(ecryptfs_global_auth_tok_cache, auth_tok);
>  	}
>  	mutex_unlock(&mount_crypt_stat->global_auth_tok_list_mutex);
> diff --git a/fs/ecryptfs/keystore.c b/fs/ecryptfs/keystore.c
> index 4feb78c..c90456a 100644
> --- a/fs/ecryptfs/keystore.c
> +++ b/fs/ecryptfs/keystore.c
> @@ -765,8 +765,10 @@ out_free_unlock:
>  out_unlock:
>  	mutex_unlock(s->tfm_mutex);
>  out:
> -	if (auth_tok_key)
> +	if (auth_tok_key) {
> +		up_write(&(auth_tok_key->sem));
>  		key_put(auth_tok_key);
> +	}
>  	kfree(s);
>  	return rc;
>  }
> @@ -1002,8 +1004,10 @@ out:
>  		(*filename_size) = 0;
>  		(*filename) = NULL;
>  	}
> -	if (auth_tok_key)
> +	if (auth_tok_key) {
> +		up_write(&(auth_tok_key->sem));
>  		key_put(auth_tok_key);
> +	}
>  	kfree(s);
>  	return rc;
>  }
> @@ -1566,6 +1570,7 @@ int ecryptfs_keyring_auth_tok_for_sig(struct key **auth_tok_key,
>  		(*auth_tok_key) = NULL;
>  		goto out;
>  	}
> +	down_write(&((*auth_tok_key)->sem));
>  	(*auth_tok) = ecryptfs_get_key_payload_data(*auth_tok_key);
>  	if (ecryptfs_verify_version((*auth_tok)->version)) {
>  		printk(KERN_ERR
> @@ -1587,6 +1592,7 @@ int ecryptfs_keyring_auth_tok_for_sig(struct key **auth_tok_key,
>  	}
>  out_release_key:
>  	if (rc) {
> +		up_write(&((*auth_tok_key)->sem));
>  		key_put(*auth_tok_key);
>  		(*auth_tok_key) = NULL;
>  	}
> @@ -1810,6 +1816,7 @@ int ecryptfs_parse_packet_set(struct ecryptfs_crypt_stat *crypt_stat,
>  find_next_matching_auth_tok:
>  	found_auth_tok = 0;
>  	if (auth_tok_key) {
> +		up_write(&(auth_tok_key->sem));
>  		key_put(auth_tok_key);
>  		auth_tok_key = NULL;
>  	}
> @@ -1896,8 +1903,10 @@ found_matching_auth_tok:
>  out_wipe_list:
>  	wipe_auth_tok_list(&auth_tok_list);
>  out:
> -	if (auth_tok_key)
> +	if (auth_tok_key) {
> +		up_write(&(auth_tok_key->sem));
>  		key_put(auth_tok_key);
> +	}
>  	return rc;
>  }
> 
> -- 
> 1.7.3.4
>