Re: [PATCH v2 02/11] fscrypt: split and rename setup_file_encryption_key()

[Eric Biggers <ebiggers@kernel.org>]

On Mon, Apr 10, 2023 at 03:39:55PM -0400, Sweet Tea Dorminy wrote:
>  /*
> - * Find the master key, then set up the inode's actual encryption key.
> + * Find and lock the master key.
>   *
>   * If the master key is found in the filesystem-level keyring, then it is
>   * returned in *mk_ret with its semaphore read-locked.  This is needed to ensure
> @@ -434,9 +471,8 @@ static bool fscrypt_valid_master_key_size(const struct fscrypt_master_key *mk,
>   * multiple tasks may race to create an fscrypt_info for the same inode), and to
>   * synchronize the master key being removed with a new inode starting to use it.
>   */
> -static int setup_file_encryption_key(struct fscrypt_info *ci,
> -				     bool need_dirhash_key,
> -				     struct fscrypt_master_key **mk_ret)
> +static int find_and_lock_master_key(const struct fscrypt_info *ci,
> +				    struct fscrypt_master_key **mk_ret)
>  {
>  	struct super_block *sb = ci->ci_inode->i_sb;
>  	struct fscrypt_key_specifier mk_spec;
> @@ -466,17 +502,13 @@ static int setup_file_encryption_key(struct fscrypt_info *ci,
>  			mk = fscrypt_find_master_key(sb, &mk_spec);
>  		}
>  	}
> +
>  	if (unlikely(!mk)) {
>  		if (ci->ci_policy.version != FSCRYPT_POLICY_V1)
>  			return -ENOKEY;
>  
> -		/*
> -		 * As a legacy fallback for v1 policies, search for the key in
> -		 * the current task's subscribed keyrings too.  Don't move this
> -		 * to before the search of ->s_master_keys, since users
> -		 * shouldn't be able to override filesystem-level keys.
> -		 */
> -		return fscrypt_setup_v1_file_key_via_subscribed_keyrings(ci);
> +		*mk_ret = NULL;
> +		return 0;

While this change may be a benefit overall, it does split the code that handles
the legacy case of "v1 policy using process-subscribed keyrings" into two
places.  That makes it a little more difficult to understand.  I think a comment
would be helpful here, at least?

- Eric