From: "Luís Henriques" <lhenriques@suse.de>
To: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
Cc: "Theodore Y. Ts'o" <tytso@mit.edu>,
Jaegeuk Kim <jaegeuk@kernel.org>,
Eric Biggers <ebiggers@kernel.org>, Chris Mason <clm@fb.com>,
Josef Bacik <josef@toxicpanda.com>,
David Sterba <dsterba@suse.com>,
linux-fscrypt@vger.kernel.org, linux-btrfs@vger.kernel.org,
kernel-team@meta.com, Omar Sandoval <osandov@osandov.com>
Subject: Re: [PATCH v2 04/17] btrfs: start using fscrypt hooks
Date: Mon, 17 Jul 2023 16:34:17 +0100 [thread overview]
Message-ID: <875y6iwnsr.fsf@suse.de> (raw)
In-Reply-To: 0afc75247f4312d78ce663382e7d89854c353e9b.1689564024.git.sweettea-kernel@dorminy.me
(Sorry for the send. Somehow, my MUA is failing to set the CC header.)
Sweet Tea Dorminy <sweettea-kernel@dorminy.me> writes:
> From: Omar Sandoval <osandov@osandov.com>
>
> In order to appropriately encrypt, create, open, rename, and various
> symlink operations must call fscrypt hooks. These determine whether the
> inode should be encrypted and do other preparatory actions. The
> superblock must have fscrypt operations registered, so implement the
> minimal set also, and introduce the new fscrypt.[ch] files to hold the
> fscrypt-specific functionality. Also add the key prefix for fscrypt v1
> keys.
>
> Signed-off-by: Omar Sandoval <osandov@osandov.com>
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> ---
> fs/btrfs/Makefile | 1 +
> fs/btrfs/btrfs_inode.h | 1 +
> fs/btrfs/file.c | 3 ++
> fs/btrfs/fscrypt.c | 8 ++++
> fs/btrfs/fscrypt.h | 10 +++++
> fs/btrfs/inode.c | 87 +++++++++++++++++++++++++++++++++++-------
> fs/btrfs/super.c | 2 +
> 7 files changed, 99 insertions(+), 13 deletions(-)
> create mode 100644 fs/btrfs/fscrypt.c
> create mode 100644 fs/btrfs/fscrypt.h
>
> diff --git a/fs/btrfs/Makefile b/fs/btrfs/Makefile
> index 90d53209755b..90d30b1e044a 100644
> --- a/fs/btrfs/Makefile
> +++ b/fs/btrfs/Makefile
> @@ -40,6 +40,7 @@ btrfs-$(CONFIG_BTRFS_FS_CHECK_INTEGRITY) += check-integrity.o
> btrfs-$(CONFIG_BTRFS_FS_REF_VERIFY) += ref-verify.o
> btrfs-$(CONFIG_BLK_DEV_ZONED) += zoned.o
> btrfs-$(CONFIG_FS_VERITY) += verity.o
> +btrfs-$(CONFIG_FS_ENCRYPTION) += fscrypt.o
>
> btrfs-$(CONFIG_BTRFS_FS_RUN_SANITY_TESTS) += tests/free-space-tests.o \
> tests/extent-buffer-tests.o tests/btrfs-tests.o \
> diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
> index d47a927b3504..ec4a06a78aff 100644
> --- a/fs/btrfs/btrfs_inode.h
> +++ b/fs/btrfs/btrfs_inode.h
> @@ -448,6 +448,7 @@ struct btrfs_new_inode_args {
> struct posix_acl *default_acl;
> struct posix_acl *acl;
> struct fscrypt_name fname;
> + bool encrypt;
> };
>
> int btrfs_new_inode_prepare(struct btrfs_new_inode_args *args,
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index fd03e689a6be..73038908876a 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -3698,6 +3698,9 @@ static int btrfs_file_open(struct inode *inode, struct file *filp)
>
> filp->f_mode |= FMODE_NOWAIT | FMODE_BUF_RASYNC | FMODE_BUF_WASYNC |
> FMODE_CAN_ODIRECT;
> + ret = fscrypt_file_open(inode, filp);
> + if (ret)
> + return ret;
>
> ret = fsverity_file_open(inode, filp);
> if (ret)
> diff --git a/fs/btrfs/fscrypt.c b/fs/btrfs/fscrypt.c
Nit: both ext4 and ubifs (and in a (hopefully!) near future, ceph) use
'crypto.c' for naming this file. I'd prefer consistency, but meh.
> new file mode 100644
> index 000000000000..3a53dc59c1e4
> --- /dev/null
> +++ b/fs/btrfs/fscrypt.c
> @@ -0,0 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include "ctree.h"
> +#include "fscrypt.h"
> +
> +const struct fscrypt_operations btrfs_fscrypt_ops = {
> + .key_prefix = "btrfs:"
> +};
> diff --git a/fs/btrfs/fscrypt.h b/fs/btrfs/fscrypt.h
> new file mode 100644
> index 000000000000..7f4e6888bd43
> --- /dev/null
> +++ b/fs/btrfs/fscrypt.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef BTRFS_FSCRYPT_H
> +#define BTRFS_FSCRYPT_H
> +
> +#include <linux/fscrypt.h>
> +
> +extern const struct fscrypt_operations btrfs_fscrypt_ops;
> +
> +#endif /* BTRFS_FSCRYPT_H */
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 6e622328f2b5..d8484752b905 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -5361,6 +5361,7 @@ void btrfs_evict_inode(struct inode *inode)
> trace_btrfs_inode_evict(inode);
>
> if (!root) {
> + fscrypt_put_encryption_info(inode);
> fsverity_cleanup_inode(inode);
> clear_inode(inode);
Small suggestion: maybe it's time to start thinking adding a new label in
this function and use a 'goto' here.
> return;
> @@ -5464,6 +5465,7 @@ void btrfs_evict_inode(struct inode *inode)
> * to retry these periodically in the future.
> */
> btrfs_remove_delayed_node(BTRFS_I(inode));
> + fscrypt_put_encryption_info(inode);
> fsverity_cleanup_inode(inode);
> clear_inode(inode);
> }
> @@ -6214,6 +6216,10 @@ int btrfs_new_inode_prepare(struct btrfs_new_inode_args *args,
> return ret;
> }
>
> + ret = fscrypt_prepare_new_inode(dir, inode, &args->encrypt);
> + if (ret)
We're leaking args->fname here.
Cheers,
--
Luís
> + return ret;
> +
> /* 1 to add inode item */
> *trans_num_items = 1;
> /* 1 to add compression property */
> @@ -6690,9 +6696,13 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir,
> if (inode->i_nlink >= BTRFS_LINK_MAX)
> return -EMLINK;
>
> + err = fscrypt_prepare_link(old_dentry, dir, dentry);
> + if (err)
> + return err;
> +
> err = fscrypt_setup_filename(dir, &dentry->d_name, 0, &fname);
> if (err)
> - goto fail;
> + return err;
>
> err = btrfs_set_inode_index(BTRFS_I(dir), &index);
> if (err)
> @@ -8636,6 +8646,7 @@ void btrfs_test_destroy_inode(struct inode *inode)
>
> void btrfs_free_inode(struct inode *inode)
> {
> + fscrypt_free_inode(inode);
> kmem_cache_free(btrfs_inode_cachep, BTRFS_I(inode));
> }
>
> @@ -8706,8 +8717,7 @@ int btrfs_drop_inode(struct inode *inode)
> /* the snap/subvol tree is on deleting */
> if (btrfs_root_refs(&root->root_item) == 0)
> return 1;
> - else
> - return generic_drop_inode(inode);
> + return generic_drop_inode(inode) || fscrypt_drop_inode(inode);
> }
>
> static void init_once(void *foo)
> @@ -9298,6 +9308,11 @@ static int btrfs_rename2(struct mnt_idmap *idmap, struct inode *old_dir,
> if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE | RENAME_WHITEOUT))
> return -EINVAL;
>
> + ret = fscrypt_prepare_rename(old_dir, old_dentry, new_dir, new_dentry,
> + flags);
> + if (ret)
> + return ret;
> +
> if (flags & RENAME_EXCHANGE)
> ret = btrfs_rename_exchange(old_dir, old_dentry, new_dir,
> new_dentry);
> @@ -9517,15 +9532,22 @@ static int btrfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
> };
> unsigned int trans_num_items;
> int err;
> - int name_len;
> int datasize;
> unsigned long ptr;
> struct btrfs_file_extent_item *ei;
> struct extent_buffer *leaf;
> + struct fscrypt_str disk_link;
> + u32 name_len = strlen(symname);
>
> - name_len = strlen(symname);
> - if (name_len > BTRFS_MAX_INLINE_DATA_SIZE(fs_info))
> - return -ENAMETOOLONG;
> + /*
> + * fscrypt sets disk_link.len to be len + 1, including a NUL terminator, but we
> + * don't store that '\0' character.
> + */
> + err = fscrypt_prepare_symlink(dir, symname, name_len,
> + BTRFS_MAX_INLINE_DATA_SIZE(fs_info) + 1,
> + &disk_link);
> + if (err)
> + return err;
>
> inode = new_inode(dir->i_sb);
> if (!inode)
> @@ -9534,8 +9556,8 @@ static int btrfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
> inode->i_op = &btrfs_symlink_inode_operations;
> inode_nohighmem(inode);
> inode->i_mapping->a_ops = &btrfs_aops;
> - btrfs_i_size_write(BTRFS_I(inode), name_len);
> - inode_set_bytes(inode, name_len);
> + btrfs_i_size_write(BTRFS_I(inode), disk_link.len - 1);
> + inode_set_bytes(inode, disk_link.len - 1);
>
> new_inode_args.inode = inode;
> err = btrfs_new_inode_prepare(&new_inode_args, &trans_num_items);
> @@ -9562,10 +9584,23 @@ static int btrfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
> inode = NULL;
> goto out;
> }
> +
> + if (IS_ENCRYPTED(inode)) {
> + err = fscrypt_encrypt_symlink(inode, symname, name_len,
> + &disk_link);
> + if (err) {
> + btrfs_abort_transaction(trans, err);
> + btrfs_free_path(path);
> + discard_new_inode(inode);
> + inode = NULL;
> + goto out;
> + }
> + }
> +
> key.objectid = btrfs_ino(BTRFS_I(inode));
> key.offset = 0;
> key.type = BTRFS_EXTENT_DATA_KEY;
> - datasize = btrfs_file_extent_calc_inline_size(name_len);
> + datasize = btrfs_file_extent_calc_inline_size(disk_link.len - 1);
> err = btrfs_insert_empty_item(trans, root, path, &key,
> datasize);
> if (err) {
> @@ -9584,10 +9619,10 @@ static int btrfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
> btrfs_set_file_extent_encryption(leaf, ei, 0);
> btrfs_set_file_extent_compression(leaf, ei, 0);
> btrfs_set_file_extent_other_encoding(leaf, ei, 0);
> - btrfs_set_file_extent_ram_bytes(leaf, ei, name_len);
> + btrfs_set_file_extent_ram_bytes(leaf, ei, disk_link.len - 1);
>
> ptr = btrfs_file_extent_inline_start(ei);
> - write_extent_buffer(leaf, symname, ptr, name_len);
> + write_extent_buffer(leaf, disk_link.name, ptr, disk_link.len - 1);
> btrfs_mark_buffer_dirty(leaf);
> btrfs_free_path(path);
>
> @@ -9604,6 +9639,29 @@ static int btrfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
> return err;
> }
>
> +static const char *btrfs_get_link(struct dentry *dentry, struct inode *inode,
> + struct delayed_call *done)
> +{
> + struct page *cpage;
> + const char *paddr;
> + struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> +
> + if (!IS_ENCRYPTED(inode))
> + return page_get_link(dentry, inode, done);
> +
> + if (!dentry)
> + return ERR_PTR(-ECHILD);
> +
> + cpage = read_mapping_page(inode->i_mapping, 0, NULL);
> + if (IS_ERR(cpage))
> + return ERR_CAST(cpage);
> +
> + paddr = fscrypt_get_symlink(inode, page_address(cpage),
> + BTRFS_MAX_INLINE_DATA_SIZE(fs_info), done);
> + put_page(cpage);
> + return paddr;
> +}
> +
> static struct btrfs_trans_handle *insert_prealloc_file_extent(
> struct btrfs_trans_handle *trans_in,
> struct btrfs_inode *inode,
> @@ -11082,7 +11140,7 @@ static const struct inode_operations btrfs_special_inode_operations = {
> .update_time = btrfs_update_time,
> };
> static const struct inode_operations btrfs_symlink_inode_operations = {
> - .get_link = page_get_link,
> + .get_link = btrfs_get_link,
> .getattr = btrfs_getattr,
> .setattr = btrfs_setattr,
> .permission = btrfs_permission,
> @@ -11092,4 +11150,7 @@ static const struct inode_operations btrfs_symlink_inode_operations = {
>
> const struct dentry_operations btrfs_dentry_operations = {
> .d_delete = btrfs_dentry_delete,
> +#ifdef CONFIG_FS_ENCRYPTION
> + .d_revalidate = fscrypt_d_revalidate,
> +#endif
> };
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index cffdd6f7f8e8..08b1e2ded5be 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -48,6 +48,7 @@
> #include "tests/btrfs-tests.h"
> #include "block-group.h"
> #include "discard.h"
> +#include "fscrypt.h"
> #include "qgroup.h"
> #include "raid56.h"
> #include "fs.h"
> @@ -1144,6 +1145,7 @@ static int btrfs_fill_super(struct super_block *sb,
> sb->s_vop = &btrfs_verityops;
> #endif
> sb->s_xattr = btrfs_xattr_handlers;
> + fscrypt_set_ops(sb, &btrfs_fscrypt_ops);
> sb->s_time_gran = 1;
> #ifdef CONFIG_BTRFS_FS_POSIX_ACL
> sb->s_flags |= SB_POSIXACL;
> --
>
> 2.40.1
>
next prev parent reply other threads:[~2023-07-17 15:38 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-17 3:52 [PATCH v2 00/17] btrfs: add encryption feature Sweet Tea Dorminy
2023-07-17 3:52 ` [PATCH v2 01/17] btrfs: disable various operations on encrypted inodes Sweet Tea Dorminy
2023-07-17 3:52 ` [PATCH v2 02/17] btrfs: disable verity " Sweet Tea Dorminy
2023-07-17 3:52 ` [PATCH v2 03/17] fscrypt: expose fscrypt_nokey_name Sweet Tea Dorminy
2023-07-17 3:52 ` [PATCH v2 04/17] btrfs: start using fscrypt hooks Sweet Tea Dorminy
2023-07-17 15:34 ` Luís Henriques [this message]
2023-07-17 17:28 ` David Sterba
2023-07-18 8:36 ` Luís Henriques
2023-07-17 3:52 ` [PATCH v2 05/17] btrfs: add inode encryption contexts Sweet Tea Dorminy
2023-07-17 15:41 ` Josef Bacik
2023-07-17 3:52 ` [PATCH v2 06/17] btrfs: add new FEATURE_INCOMPAT_ENCRYPT flag Sweet Tea Dorminy
2023-07-17 15:42 ` Josef Bacik
2023-07-17 3:52 ` [PATCH v2 07/17] btrfs: adapt readdir for encrypted and nokey names Sweet Tea Dorminy
2023-07-17 15:34 ` Luís Henriques
2023-07-17 17:46 ` Josef Bacik
2023-07-17 3:52 ` [PATCH v2 08/17] btrfs: use correct name hash for " Sweet Tea Dorminy
2023-07-17 3:52 ` [PATCH v2 09/17] btrfs: implement fscrypt ioctls Sweet Tea Dorminy
2023-07-17 3:52 ` [PATCH v2 10/17] btrfs: add encryption to CONFIG_BTRFS_DEBUG Sweet Tea Dorminy
2023-07-17 3:52 ` [PATCH v2 11/17] btrfs: add get_devices hook for fscrypt Sweet Tea Dorminy
2023-07-17 17:51 ` Josef Bacik
2023-07-17 3:52 ` [PATCH v2 12/17] btrfs: turn on inlinecrypt mount option for encrypt Sweet Tea Dorminy
2023-07-17 15:34 ` Luís Henriques
2023-07-17 17:55 ` Josef Bacik
2023-07-17 3:52 ` [PATCH v2 13/17] btrfs: turn on the encryption ioctls Sweet Tea Dorminy
2023-07-17 3:52 ` [PATCH v2 14/17] btrfs: create and free extent fscrypt_infos Sweet Tea Dorminy
2023-07-17 17:58 ` Josef Bacik
2023-07-17 3:52 ` [PATCH v2 15/17] btrfs: start tracking extent encryption context info Sweet Tea Dorminy
2023-07-17 18:11 ` Josef Bacik
2023-07-17 3:52 ` [PATCH v2 16/17] btrfs: explicitly track file extent length and encryption Sweet Tea Dorminy
2023-07-17 15:30 ` Josef Bacik
2023-07-17 18:12 ` Josef Bacik
2023-07-17 3:52 ` [PATCH v2 17/17] btrfs: save and load fscrypt extent contexts Sweet Tea Dorminy
2023-07-17 18:15 ` Josef Bacik
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=875y6iwnsr.fsf@suse.de \
--to=lhenriques@suse.de \
--cc=clm@fb.com \
--cc=dsterba@suse.com \
--cc=ebiggers@kernel.org \
--cc=jaegeuk@kernel.org \
--cc=josef@toxicpanda.com \
--cc=kernel-team@meta.com \
--cc=linux-btrfs@vger.kernel.org \
--cc=linux-fscrypt@vger.kernel.org \
--cc=osandov@osandov.com \
--cc=sweettea-kernel@dorminy.me \
--cc=tytso@mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox