Re: [PATCH v2 08/20] btrfs: use fscrypt_names instead of name/len everywhere.

[David Sterba <dsterba@suse.cz>]

On Mon, Sep 05, 2022 at 08:35:23PM -0400, Sweet Tea Dorminy wrote:
> For encryption, the plaintext filenames provided by the VFS will need to
> be translated to ciphertext filenames on disk. Fscrypt provides a struct
> to encapsulate a potentially encrypted filename, struct fscrypt_name.
> This change converts every (name, len) pair to be a struct fscrypt_name,
> statically initialized, for ease of review and uniformity.

Is there some clear boundary where the name needs to be encoded or
decoded? I don't think we should use fscrypt_name in so many functions,
namely internal helpers that really only care about the plain name +
length. Such widespread use fscrypt structure would make it hard to
synchronize with the user space sources.

What we could do to ease the integragtion with the fscrypt name is to
use the qstr structure, ie. something that's easily convertible to the
fscrypt_name::usr_fname.

> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -21,11 +21,13 @@
>  #include <linux/pagemap.h>
>  #include <linux/btrfs.h>
>  #include <linux/btrfs_tree.h>
> +#include <linux/fscrypt.h>
>  #include <linux/workqueue.h>
>  #include <linux/security.h>
>  #include <linux/sizes.h>
>  #include <linux/dynamic_debug.h>
>  #include <linux/refcount.h>
> +#include <linux/crc32.h>
>  #include <linux/crc32c.h>
>  #include <linux/iomap.h>
>  #include "extent-io-tree.h"
> @@ -2803,18 +2805,19 @@ static inline void btrfs_crc32c_final(u32 crc, u8 *result)
>  	put_unaligned_le32(~crc, result);
>  }
>  
> -static inline u64 btrfs_name_hash(const char *name, int len)
> +static inline u64 btrfs_name_hash(const struct fscrypt_name *name)
>  {
> -       return crc32c((u32)~1, name, len);
> +	return crc32c((u32)~1, fname_name(name), fname_len(name));

This for example is a primitive helper that just hashes the correct
bytes and does not need to know anything about whether it's encrypted or
not. That should be set up higher in the call chain.

> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -3863,11 +3863,19 @@ static noinline int acls_after_inode_item(struct extent_buffer *leaf,
>  	static u64 xattr_default = 0;
>  	int scanned = 0;
>  
> +	struct fscrypt_name name_access = {
> +		.disk_name = FSTR_INIT(XATTR_NAME_POSIX_ACL_ACCESS,
> +				       strlen(XATTR_NAME_POSIX_ACL_ACCESS))
> +	};
> +
> +	struct fscrypt_name name_default = {
> +		.disk_name = FSTR_INIT(XATTR_NAME_POSIX_ACL_DEFAULT,
> +				       strlen(XATTR_NAME_POSIX_ACL_DEFAULT))
> +	};
> +
>  	if (!xattr_access) {
> -		xattr_access = btrfs_name_hash(XATTR_NAME_POSIX_ACL_ACCESS,
> -					strlen(XATTR_NAME_POSIX_ACL_ACCESS));
> -		xattr_default = btrfs_name_hash(XATTR_NAME_POSIX_ACL_DEFAULT,
> -					strlen(XATTR_NAME_POSIX_ACL_DEFAULT));
> +		xattr_access = btrfs_name_hash(&name_access);
> +		xattr_default = btrfs_name_hash(&name_default);

And here it needs extra structure just to pass plain strings.

> +			   __func__, fname_name(&fname), btrfs_ino(BTRFS_I(dir)),
>  			   location->objectid, location->type, location->offset);
>  	}
>  	if (!ret)
> @@ -6243,6 +6258,14 @@ int btrfs_new_inode_prepare(struct btrfs_new_inode_args *args,
>  	if (ret)
>  		return ret;
>  
> +	if (!args->orphan) {
> +		char *name = (char *) args->dentry->d_name.name;
> +		int name_len = args->dentry->d_name.len;

Please put a newline between declaration and statement block.

> +		args->fname = (struct fscrypt_name) {
> +			.disk_name = FSTR_INIT(name, name_len),
> +		};

Please don't use this construct to intialize compounds, we don't use it
anywhere. There are more examples for other structures too.

> +	}
> +
>  	/* 1 to add inode item */
>  	*trans_num_items = 1;
>  	/* 1 to add compression property */
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1596,8 +1596,9 @@ static int qgroup_account_snapshot(struct btrfs_trans_handle *trans,
>   * happens, we should return the error number. If the error which just affect
>   * the creation of the pending snapshots, just return 0.
>   */
> -static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
> -				   struct btrfs_pending_snapshot *pending)
> +static noinline int
> +create_pending_snapshot(struct btrfs_trans_handle *trans,
> +			struct btrfs_pending_snapshot *pending)

Please keep the specifiers and type on the same line as the function
name, the parameters can slightly overfrlow the 80 char limit if it
avoids a line break, otherwise the patameters go on the next line.