Re: [PATCH v2 11/14] fscrypt: add creation/usage/freeing of per-extent infos

[Josef Bacik <josef@toxicpanda.com>]

On Sun, Jul 09, 2023 at 02:53:44PM -0400, Sweet Tea Dorminy wrote:
> This change adds the superblock function pointer to get the info
> corresponding to a specific block in an inode for a filesystem using
> per-extent infos. It allows creating a info for a new extent and freeing
> that info, and uses the extent's info if appropriate in encrypting
> blocks of data.
> 
> This change does not deal with saving and loading an extent's info, but
> introduces the mechanics necessary therefore.
> 
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> ---
>  fs/crypto/crypto.c          |  2 +
>  fs/crypto/fscrypt_private.h | 76 +++++++++++++++++++++----------------
>  fs/crypto/keyring.c         |  9 +----
>  fs/crypto/keysetup.c        | 58 +++++++++++++++++++++++++++-
>  include/linux/fscrypt.h     | 48 ++++++++++++++++++-----
>  5 files changed, 141 insertions(+), 52 deletions(-)
> 
> diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
> index d75f1b3f5795..0f0c721e40fe 100644
> --- a/fs/crypto/crypto.c
> +++ b/fs/crypto/crypto.c
> @@ -113,6 +113,8 @@ int fscrypt_crypt_block(const struct inode *inode, fscrypt_direction_t rw,
>  	struct crypto_skcipher *tfm = ci->ci_enc_key->tfm;
>  	int res = 0;
>  
> +	if (!ci)
> +		return -EINVAL;
>  	if (WARN_ON_ONCE(len <= 0))
>  		return -EINVAL;
>  	if (WARN_ON_ONCE(len % FSCRYPT_CONTENTS_ALIGNMENT != 0))
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index 926531597e7b..6e6020f7746c 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -286,6 +286,38 @@ typedef enum {
>  	FS_ENCRYPT,
>  } fscrypt_direction_t;
>  
> +/**
> + * fscrypt_fs_uses_extent_encryption() -- whether a filesystem uses per-extent
> + *				          encryption
> + *
> + * @sb: the superblock of the filesystem in question
> + *
> + * Return: true if the fs uses per-extent fscrypt_infos, false otherwise
> + */
> +static inline bool
> +fscrypt_fs_uses_extent_encryption(const struct super_block *sb)
> +{
> +	return !!sb->s_cop->get_extent_info;
> +}
> +
> +/**
> + * fscrypt_uses_extent_encryption() -- whether an inode uses per-extent
> + *				       encryption
> + *
> + * @inode: the inode in question
> + *
> + * Return: true if the inode uses per-extent fscrypt_infos, false otherwise
> + */
> +static inline bool fscrypt_uses_extent_encryption(const struct inode *inode)
> +{
> +	// Non-regular files don't have extents

Wrong comment format.

> +	if (!S_ISREG(inode->i_mode))
> +		return false;
> +
> +	return fscrypt_fs_uses_extent_encryption(inode->i_sb);
> +}
> +
> +
>  /**
>   * fscrypt_get_lblk_info() - get the fscrypt_info to crypt a particular block
>   *
> @@ -306,6 +338,17 @@ static inline struct fscrypt_info *
>  fscrypt_get_lblk_info(const struct inode *inode, u64 lblk, u64 *offset,
>  		      u64 *extent_len)
>  {
> +	if (fscrypt_uses_extent_encryption(inode)) {
> +		struct fscrypt_info *info;
> +		int res;
> +
> +		res = inode->i_sb->s_cop->get_extent_info(inode, lblk, &info,
> +							  offset, extent_len);
> +		if (res == 0)
> +			return info;
> +		return NULL;
> +	}
> +
>  	if (offset)
>  		*offset = lblk;
>  	if (extent_len)
> @@ -314,39 +357,6 @@ fscrypt_get_lblk_info(const struct inode *inode, u64 lblk, u64 *offset,
>  	return inode->i_crypt_info;
>  }
>  
> -/**
> - * fscrypt_uses_extent_encryption() -- whether an inode uses per-extent
> - *				       encryption
> - *
> - * @inode: the inode in question
> - *
> - * Return: true if the inode uses per-extent fscrypt_infos, false otherwise
> - */
> -static inline bool fscrypt_uses_extent_encryption(const struct inode *inode)
> -{
> -	// Non-regular files don't have extents
> -	if (!S_ISREG(inode->i_mode))
> -		return false;
> -
> -	// No filesystems currently use per-extent infos
> -	return false;
> -}
> -
> -/**
> - * fscrypt_fs_uses_extent_encryption() -- whether a filesystem uses per-extent
> - *				          encryption
> - *
> - * @sb: the superblock of the filesystem in question
> - *
> - * Return: true if the fs uses per-extent fscrypt_infos, false otherwise
> - */
> -static inline bool
> -fscrypt_fs_uses_extent_encryption(const struct super_block *sb)
> -{
> -	// No filesystems currently use per-extent infos
> -	return false;
> -}
> -
>  /**
>   * fscrypt_get_info_ino() - get the ino or ino equivalent for an info
>   *
> diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
> index 59748d333b89..8e4065d1e422 100644
> --- a/fs/crypto/keyring.c
> +++ b/fs/crypto/keyring.c
> @@ -880,15 +880,8 @@ static void evict_dentries_for_decrypted_inodes(struct fscrypt_master_key *mk)
>  
>  	list_for_each_entry(ci, &mk->mk_decrypted_inodes, ci_master_key_link) {
>  		inode = ci->ci_inode;
> -		if (!inode) {
> -			if (!ci->ci_sb->s_cop->forget_extent_info)
> -				continue;
> -
> -			spin_unlock(&mk->mk_decrypted_inodes_lock);
> -			ci->ci_sb->s_cop->forget_extent_info(ci->ci_info_ptr);
> -			spin_lock(&mk->mk_decrypted_inodes_lock);
> +		if (ci->ci_info_ptr)
>  			continue;
> -		}
>  
>  		spin_lock(&inode->i_lock);
>  		if (inode->i_state & (I_FREEING | I_WILL_FREE | I_NEW)) {
> diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
> index 2b4fca6814a7..c8cdcd4fe835 100644
> --- a/fs/crypto/keysetup.c
> +++ b/fs/crypto/keysetup.c
> @@ -676,8 +676,8 @@ fscrypt_setup_encryption_info(struct inode *inode,
>  
>  	if (fscrypt_uses_extent_encryption(inode) && info_for_extent)
>  		crypt_info->ci_info_ptr = info_ptr;
> -	else
> -		crypt_info->ci_inode = inode;
> +
> +	crypt_info->ci_inode = inode;

You changed this and now you're changing it back, go back to the original patch
and leave this the way it was.

>  
>  	crypt_info->ci_sb = inode->i_sb;
>  	crypt_info->ci_policy = *policy;
> @@ -917,6 +917,60 @@ int fscrypt_prepare_new_inode(struct inode *dir, struct inode *inode,
>  }
>  EXPORT_SYMBOL_GPL(fscrypt_prepare_new_inode);
>  
> +/**
> + * fscrypt_prepare_new_extent() - set up the fscrypt_info for a new extent
> + * @inode: the inode to which the extent belongs
> + * @info_ptr: a pointer to return the extent's fscrypt_info into. Should be
> + *	      a pointer to a member of the extent struct, as it will be passed
> + *	      back to the filesystem if key removal demands removal of the
> + *	      info from the extent
> + * @encrypt_ret: (output) set to %true if the new inode will be encrypted
> + *
> + * If the extent is part of an encrypted inode, set up its fscrypt_info in
> + * preparation for encrypting data and set *encrypt_ret=true.
> + *
> + * This isn't %GFP_NOFS-safe, and therefore it should be called before starting
> + * any filesystem transaction to create the inode.
> + *
> + * This doesn't persist the new inode's encryption context.  That still needs to
> + * be done later by calling fscrypt_set_context().
> + *
> + * Return: 0 on success, -ENOKEY if the encryption key is missing, or another
> + *	   -errno code
> + */
> +int fscrypt_prepare_new_extent(struct inode *inode,
> +			       struct fscrypt_info **info_ptr)
> +{
> +	const union fscrypt_policy *policy;
> +	u8 nonce[FSCRYPT_FILE_NONCE_SIZE];
> +
> +	policy = fscrypt_policy_to_inherit(inode);
> +	if (policy == NULL)
> +		return 0;
> +	if (IS_ERR(policy))
> +		return PTR_ERR(policy);
> +
> +	/* Only regular files can have extents.  */
> +	if (WARN_ON_ONCE(!S_ISREG(inode->i_mode)))
> +		return -EINVAL;
> +
> +	get_random_bytes(nonce, FSCRYPT_FILE_NONCE_SIZE);
> +	return fscrypt_setup_encryption_info(inode, policy, nonce,
> +					     false, info_ptr);
> +}
> +EXPORT_SYMBOL_GPL(fscrypt_prepare_new_extent);
> +
> +/**
> + * fscrypt_free_extent_info() - free an extent's fscrypt_info
> + * @info_ptr: a pointer containing the extent's fscrypt_info pointer.
> + */
> +void fscrypt_free_extent_info(struct fscrypt_info **info_ptr)
> +{
> +	put_crypt_info(*info_ptr);
> +	*info_ptr = NULL;
> +}
> +EXPORT_SYMBOL_GPL(fscrypt_free_extent_info);
> +
>  /**
>   * fscrypt_put_encryption_info() - free most of an inode's fscrypt data
>   * @inode: an inode being evicted
> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
> index 22affbb15706..e39165fbed41 100644
> --- a/include/linux/fscrypt.h
> +++ b/include/linux/fscrypt.h
> @@ -113,6 +113,29 @@ struct fscrypt_operations {
>  	int (*set_context)(struct inode *inode, const void *ctx, size_t len,
>  			   void *fs_data);
>  
> +	/*
> +	 * Get the fscrypt_info for the given inode at the given block, for
> +	 * extent-based encryption only.
> +	 *
> +	 * @inode: the inode in question
> +	 * @lblk: the logical block number in question
> +	 * @ci: a pointer to return the fscrypt_info
> +	 * @offset: a pointer to return the offset of @lblk into the extent,
> +	 *          in blocks (may be NULL)
> +	 * @extent_len: a pointer to return the number of blocks in this extent
> +	 *              starting at this point (may be NULL)
> +	 *
> +	 * May cause the filesystem to allocate memory, which the filesystem
> +	 * must do with %GFP_NOFS, including calls into fscrypt to create or
> +	 * load an fscrypt_info.
> +	 *
> +	 * Return: 0 if an extent is found with an info, -ENODATA if the key is
> +	 *         unavailable, or another -errno.
> +	 */
> +	int (*get_extent_info)(const struct inode *inode, u64 lblk,
> +			       struct fscrypt_info **ci, u64 *offset,
> +			       u64 *extent_len);
> +
>  	/*
>  	 * Get the dummy fscrypt policy in use on the filesystem (if any).
>  	 *
> @@ -129,15 +152,6 @@ struct fscrypt_operations {
>  	 */
>  	bool (*empty_dir)(struct inode *inode);
>  
> -	/*
> -	 * Inform the filesystem that a particular extent must forget its
> -	 * fscrypt_info (for instance, for a key removal).
> -	 *
> -	 * @info_ptr: a pointer to the location storing the fscrypt_info pointer
> -	 *            within the opaque extent whose info is to be freed
> -	 */
> -	void (*forget_extent_info)(struct fscrypt_info **info_ptr);
> -

You've done this again, backed out a change you did before.  Rework the series
to simply not need this thing in the first place.  Thanks,

Josef