On Mon, Sep 05, 2022 at 08:35:28PM -0400, Sweet Tea Dorminy wrote:
> +static int btrfs_fscrypt_get_context(struct inode *inode, void *ctx, size_t len)
> +{
> + struct btrfs_root *root = BTRFS_I(inode)->root;
> + struct inode *put_inode = NULL;
> + struct btrfs_key key;
> + struct btrfs_path *path;
> + struct extent_buffer *leaf;
> + unsigned long ptr;
> + int ret;
> +
> + if (btrfs_root_flags(&root->root_item) & BTRFS_ROOT_SUBVOL_FSCRYPT) {
> + inode = btrfs_iget(inode->i_sb, BTRFS_FIRST_FREE_OBJECTID,
> + root);
> + if (IS_ERR(inode))
> + return PTR_ERR(inode);
> + put_inode = inode;
> + }
> +
> + path = btrfs_alloc_path();
> + if (!path)
> + return -ENOMEM;
> +
> + key = (struct btrfs_key) {
> + .objectid = btrfs_ino(BTRFS_I(inode)),
> + .type = BTRFS_FSCRYPT_CTXT_ITEM_KEY,
> + .offset = 0,
> + };
Please use the following for key initialization.
key.objectid = ...;
key.type = ...;
key.offset = ...;
> +static int btrfs_fscrypt_set_context(struct inode *inode, const void *ctx,
> + size_t len, void *fs_data)
> +{
> + struct btrfs_root *root = BTRFS_I(inode)->root;
> + struct btrfs_trans_handle *trans;
> + int is_subvolume = inode->i_ino == BTRFS_FIRST_FREE_OBJECTID;
> + int ret;
> + struct btrfs_path *path;
> + struct btrfs_key key = {
> + .objectid = btrfs_ino(BTRFS_I(inode)),
> + .type = BTRFS_FSCRYPT_CTXT_ITEM_KEY,
> + .offset = 0,
> + };
This kind of initialization in the declaration block is ok, possibly
with only the simple initializers like btrfs_ino or normal constants.
> + if (btrfs_root_flags(&root->root_item) & BTRFS_ROOT_SUBVOL_FSCRYPT) {
> + bool same_policy;
> + struct inode *root_inode = NULL;
Newlines between declrataions and statements
> + root_inode = btrfs_iget(inode->i_sb, BTRFS_FIRST_FREE_OBJECTID,
> + root);
> + if (IS_ERR(inode))
> + return PTR_ERR(inode);
> + same_policy = fscrypt_have_same_policy(inode, root_inode);
> + iput(root_inode);
> + if (same_policy)
> + return 0;
> + }
> +
> + if (fs_data) {
> + /*
> + * We are setting the context as part of an existing
> + * transaction. This happens when we are inheriting the context
> + * for a new inode.
> + */
> + trans = fs_data;
> + } else {
> + /*
> + * 1 for the inode item
> + * 1 for the fscrypt item
> + * 1 for the root item if the inode is a subvolume
> + */
> + trans = btrfs_start_transaction(root, 2 + is_subvolume);
> + if (IS_ERR(trans))
> + return PTR_ERR(trans);
> + }
> +
> + path = btrfs_alloc_path();
> + if (!path)
> + return -ENOMEM;
> + ret = btrfs_search_slot(trans, BTRFS_I(inode)->root, &key, path, 0, 1);
> + if (ret == 0) {
> + struct extent_buffer *leaf = path->nodes[0];
> + unsigned long ptr = btrfs_item_ptr_offset(leaf, path->slots[0]);
Newlines between declrataions and statements, you'll find the rest
> + len = min_t(size_t, len, btrfs_item_size(leaf, path->slots[0]));
> + write_extent_buffer(leaf, ctx, ptr, len);
> + btrfs_mark_buffer_dirty(leaf);
> + btrfs_free_path(path);
> + goto out;
> + } else if (ret < 0) {
> + goto out;
> + }
> + btrfs_free_path(path);
> +
> + ret = btrfs_insert_item(trans, BTRFS_I(inode)->root, &key, (void *) ctx, len);
> + if (ret)
> + goto out;
> +
> + BTRFS_I(inode)->flags |= BTRFS_INODE_FSCRYPT_CONTEXT;
> + btrfs_sync_inode_flags_to_i_flags(inode);
> + inode_inc_iversion(inode);
> + inode->i_ctime = current_time(inode);
> + ret = btrfs_update_inode(trans, root, BTRFS_I(inode));
> + if (ret)
> + goto out;
> +
> + /*
> + * For new subvolumes, the root item is already initialized with
> + * the BTRFS_ROOT_SUBVOL_FSCRYPT flag.
> + */
> + if (!fs_data && is_subvolume) {
> + u64 root_flags = btrfs_root_flags(&root->root_item);
> +
> + btrfs_set_root_flags(&root->root_item,
> + root_flags |
> + BTRFS_ROOT_SUBVOL_FSCRYPT);
> + ret = btrfs_update_root(trans, root->fs_info->tree_root,
> + &root->root_key,
> + &root->root_item);
> + }
> +out:
> + if (fs_data)
> + return ret;
> +
> + if (ret)
> + btrfs_abort_transaction(trans, ret);
> + else
> + btrfs_end_transaction(trans);
> + return ret;
> +}
> +
> + if (args->encrypt)
> + (*trans_num_items)++; /* 1 to add fscrypt item */
Please put the comment on a separate line, like it's on the lines below.
> if (args->orphan) {
> /* 1 to add orphan item */
> (*trans_num_items)++;
> @@ -787,6 +788,13 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir,
> return -ETXTBSY;
> }
>
> + if ((btrfs_root_flags(&root->root_item) & BTRFS_ROOT_SUBVOL_FSCRYPT) &&
> + !IS_ENCRYPTED(dir)) {
> + btrfs_warn(fs_info,
> + "cannot snapshot encrypted volume to unencrypted destination");
Do we want to print that to the log? There are several EXDEV causes,
only another one prints a message and I think it should not.
> + return -EXDEV;
> + }
> +
> pending_snapshot = kzalloc(sizeof(*pending_snapshot), GFP_KERNEL);
> if (!pending_snapshot)
> return -ENOMEM;
> --- a/include/uapi/linux/btrfs_tree.h
> +++ b/include/uapi/linux/btrfs_tree.h
> @@ -144,6 +144,8 @@
> #define BTRFS_VERITY_DESC_ITEM_KEY 36
> #define BTRFS_VERITY_MERKLE_ITEM_KEY 37
>
> +#define BTRFS_FSCRYPT_CTXT_ITEM_KEY 41
The context is per inode, so OK the key is needed and the number is
leaving enough space in case we'd need to add more.
|