| Message ID | 0561e8a33f991fa15053054b7b089d176fde6523.1660596577.git.boris@bur.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] btrfs: send: add support for fs-verity | expand |
Hi Boris,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on kdave/for-next]
[also build test WARNING on linus/master v6.0-rc1 next-20220816]
[cannot apply to fscrypt/fsverity]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Boris-Burkov/btrfs-send-add-support-for-fs-verity/20220816-130051
base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: sparc-randconfig-c003-20220815 (https://download.01.org/0day-ci/archive/20220816/202208161755.1c2WmPnn-lkp@intel.com/config)
compiler: sparc-linux-gcc (GCC) 12.1.0
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
cocci warnings: (new ones prefixed by >>)
>> fs/btrfs/send.c:6874:5-8: Unneeded variable: "ret". Return "0" on line 6880
On Mon, Aug 15, 2022 at 01:54:28PM -0700, Boris Burkov wrote: > diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c > index e7671afcee4f..9e8679848d54 100644 > --- a/fs/btrfs/send.c > +++ b/fs/btrfs/send.c > @@ -3,6 +3,7 @@ > * Copyright (C) 2012 Alexander Block. All rights reserved. > */ > > +#include "linux/compiler_attributes.h" I don't understand the purpose of this include. And why is it in quotes? Otherwise this patch looks good to me. - Eric
On Mon, Aug 15, 2022 at 01:54:28PM -0700, Boris Burkov wrote: > Preserve the fs-verity status of a btrfs file across send/recv. > > There is no facility for installing the Merkle tree contents directly on > the receiving filesystem, so we package up the parameters used to enable > verity found in the verity descriptor. This gives the receive side > enough information to properly enable verity again. Note that this means > that receive will have to re-compute the whole Merkle tree, similar to > how compression worked before encoded_write. > > Since the file becomes read-only after verity is enabled, it is > important that verity is added to the send stream after any file writes. > Therefore, when we process a verity item, merely note that it happened, > then actually create the command in the send stream during > 'finish_inode_if_needed'. > > This also creates V3 of the send stream format, without any format > changes besides adding the new commands and attributes. > > Signed-off-by: Boris Burkov <boris@bur.io> As for the merge target, a realistic one seems to be 6.2, we have too many pending patches everywhere else. There's a todo list for v3 that I'd really like to get done. To be able to test things incrementally until then we can add v3 support under debug config. > -- > Changes in v4: > - Use btrfs_get_verity_descriptor instead of verity ops get descriptor. > Move that definition to ctree.h for conditional building. This cleaned > up most of the conditional building issues, in my opinion. Yes, that way it's ok. > - Rename process_new_verity to process_verity. > - Use le-to-cpu conversion for all fsverity descriptor fields. > - Don't check NULL for kvfree of the send descriptor. > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > --- a/fs/btrfs/send.c > +++ b/fs/btrfs/send.c > @@ -3,6 +3,7 @@ > * Copyright (C) 2012 Alexander Block. All rights reserved. > */ > > +#include "linux/compiler_attributes.h" As Eric pointed out, this is not necessary, I'll delete the line, no need to resend just for that.
On Wed, Aug 17, 2022 at 09:57:22PM -0700, Eric Biggers wrote: > On Mon, Aug 15, 2022 at 01:54:28PM -0700, Boris Burkov wrote: > > diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c > > index e7671afcee4f..9e8679848d54 100644 > > --- a/fs/btrfs/send.c > > +++ b/fs/btrfs/send.c > > @@ -3,6 +3,7 @@ > > * Copyright (C) 2012 Alexander Block. All rights reserved. > > */ > > > > +#include "linux/compiler_attributes.h" > > I don't understand the purpose of this include. And why is it in quotes? It compiles without it so I've deleted it. > > Otherwise this patch looks good to me. I assume it's acked-by/reviewed-by namely for the fs-verity changes, right?
On Thu, Aug 18, 2022 at 07:40:10PM +0200, David Sterba wrote: > On Wed, Aug 17, 2022 at 09:57:22PM -0700, Eric Biggers wrote: > > On Mon, Aug 15, 2022 at 01:54:28PM -0700, Boris Burkov wrote: > > > diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c > > > index e7671afcee4f..9e8679848d54 100644 > > > --- a/fs/btrfs/send.c > > > +++ b/fs/btrfs/send.c > > > @@ -3,6 +3,7 @@ > > > * Copyright (C) 2012 Alexander Block. All rights reserved. > > > */ > > > > > > +#include "linux/compiler_attributes.h" > > > > I don't understand the purpose of this include. And why is it in quotes? > > It compiles without it so I've deleted it. > > > > > Otherwise this patch looks good to me. > > I assume it's acked-by/reviewed-by namely for the fs-verity changes, > right? Yes if you're just fixing that without a new version, you can add: Acked-by: Eric Biggers <ebiggers@google.com> (By the way, please fix your email client to not generate a Mail-Followup-To header, as it causes replies to move everyone to "To". If you're using mutt, you need 'set followup_to = no' in your muttrc.) - Eric
On Wed, Aug 17, 2022 at 09:57:22PM -0700, Eric Biggers wrote: > On Mon, Aug 15, 2022 at 01:54:28PM -0700, Boris Burkov wrote: > > diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c > > index e7671afcee4f..9e8679848d54 100644 > > --- a/fs/btrfs/send.c > > +++ b/fs/btrfs/send.c > > @@ -3,6 +3,7 @@ > > * Copyright (C) 2012 Alexander Block. All rights reserved. > > */ > > > > +#include "linux/compiler_attributes.h" > > I don't understand the purpose of this include. And why is it in quotes? Sorry about that. I have this weird feeling that vim-lsp is "helpfully" adding includes for me sometimes. I'll look out for it next time I send a patch. Thanks for the review. > > Otherwise this patch looks good to me. > > - Eric
On Thu, Aug 18, 2022 at 07:32:54PM +0200, David Sterba wrote: > On Mon, Aug 15, 2022 at 01:54:28PM -0700, Boris Burkov wrote: > > Preserve the fs-verity status of a btrfs file across send/recv. > > > > There is no facility for installing the Merkle tree contents directly on > > the receiving filesystem, so we package up the parameters used to enable > > verity found in the verity descriptor. This gives the receive side > > enough information to properly enable verity again. Note that this means > > that receive will have to re-compute the whole Merkle tree, similar to > > how compression worked before encoded_write. > > > > Since the file becomes read-only after verity is enabled, it is > > important that verity is added to the send stream after any file writes. > > Therefore, when we process a verity item, merely note that it happened, > > then actually create the command in the send stream during > > 'finish_inode_if_needed'. > > > > This also creates V3 of the send stream format, without any format > > changes besides adding the new commands and attributes. > > > > Signed-off-by: Boris Burkov <boris@bur.io> > > As for the merge target, a realistic one seems to be 6.2, we have too > many pending patches everywhere else. There's a todo list for v3 that > I'd really like to get done. > > To be able to test things incrementally until then we can add v3 support > under debug config. That all sounds good and reasonable to me. Would you like me to re-send with gating V3 behind debug, or will you do it as part of taking it? Also, this just popped in my head, but could we acheive what we want with the "--proto" feature of the send CLI, and having a notion of a provisional version that is not yet hardened and properly named/fixed for future compatibility? For extra fanciness, we can do sub-versions or hashes of the commands or something. Maybe proto=(u64)-1 means experimental. Anyway, I'm totally fine with putting it behind debug till it's done, but figured I'd share that thought. > > > -- > > Changes in v4: > > - Use btrfs_get_verity_descriptor instead of verity ops get descriptor. > > Move that definition to ctree.h for conditional building. This cleaned > > up most of the conditional building issues, in my opinion. > > Yes, that way it's ok. > > > - Rename process_new_verity to process_verity. > > - Use le-to-cpu conversion for all fsverity descriptor fields. > > - Don't check NULL for kvfree of the send descriptor. > > > --- a/fs/btrfs/ctree.h > > +++ b/fs/btrfs/ctree.h > > --- a/fs/btrfs/send.c > > +++ b/fs/btrfs/send.c > > @@ -3,6 +3,7 @@ > > * Copyright (C) 2012 Alexander Block. All rights reserved. > > */ > > > > +#include "linux/compiler_attributes.h" > > As Eric pointed out, this is not necessary, I'll delete the line, no > need to resend just for that.
On Thu, Aug 18, 2022 at 11:49:33AM -0700, Boris Burkov wrote: > On Thu, Aug 18, 2022 at 07:32:54PM +0200, David Sterba wrote: > > On Mon, Aug 15, 2022 at 01:54:28PM -0700, Boris Burkov wrote: > > > Preserve the fs-verity status of a btrfs file across send/recv. > > > > > > There is no facility for installing the Merkle tree contents directly on > > > the receiving filesystem, so we package up the parameters used to enable > > > verity found in the verity descriptor. This gives the receive side > > > enough information to properly enable verity again. Note that this means > > > that receive will have to re-compute the whole Merkle tree, similar to > > > how compression worked before encoded_write. > > > > > > Since the file becomes read-only after verity is enabled, it is > > > important that verity is added to the send stream after any file writes. > > > Therefore, when we process a verity item, merely note that it happened, > > > then actually create the command in the send stream during > > > 'finish_inode_if_needed'. > > > > > > This also creates V3 of the send stream format, without any format > > > changes besides adding the new commands and attributes. > > > > > > Signed-off-by: Boris Burkov <boris@bur.io> > > > > As for the merge target, a realistic one seems to be 6.2, we have too > > many pending patches everywhere else. There's a todo list for v3 that > > I'd really like to get done. > > > > To be able to test things incrementally until then we can add v3 support > > under debug config. > > That all sounds good and reasonable to me. Would you like me to re-send > with gating V3 behind debug, or will you do it as part of taking it? Please send a separate patch for that. > Also, this just popped in my head, but could we acheive what we want > with the "--proto" feature of the send CLI, and having a notion of a > provisional version that is not yet hardened and properly named/fixed > for future compatibility? For extra fanciness, we can do sub-versions > or hashes of the commands or something. Maybe proto=(u64)-1 means > experimental. This could be usefor for some cases but I think not for the protocol, the version is linear and we want to batch the changes into one version. Othewise it creates dependency and configuration complications, and users don't always listen to the "this is still experimental" should it be meant to be outside of the debug build.
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index 51c480439263..7c55af4aa048 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -4106,6 +4106,7 @@ static inline int btrfs_defrag_cancelled(struct btrfs_fs_info *fs_info) extern const struct fsverity_operations btrfs_verityops; int btrfs_drop_verity_items(struct btrfs_inode *inode); +int btrfs_get_verity_descriptor(struct inode *inode, void *buf, size_t buf_size); BTRFS_SETGET_FUNCS(verity_descriptor_encryption, struct btrfs_verity_descriptor_item, encryption, 8); @@ -4123,6 +4124,11 @@ static inline int btrfs_drop_verity_items(struct btrfs_inode *inode) return 0; } +static inline int btrfs_get_verity_descriptor(struct inode *inode, void *buf, size_t buf_size) +{ + return -EPERM; +} + #endif /* Sanity test specific functions */ diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c index e7671afcee4f..9e8679848d54 100644 --- a/fs/btrfs/send.c +++ b/fs/btrfs/send.c @@ -3,6 +3,7 @@ * Copyright (C) 2012 Alexander Block. All rights reserved. */ +#include "linux/compiler_attributes.h" #include <linux/bsearch.h> #include <linux/fs.h> #include <linux/file.h> @@ -15,6 +16,7 @@ #include <linux/string.h> #include <linux/compat.h> #include <linux/crc32c.h> +#include <linux/fsverity.h> #include "send.h" #include "ctree.h" @@ -127,6 +129,8 @@ struct send_ctx { bool cur_inode_new_gen; bool cur_inode_deleted; bool ignore_cur_inode; + bool cur_inode_needs_verity; + void *verity_descriptor; u64 send_progress; @@ -624,6 +628,7 @@ static int tlv_put(struct send_ctx *sctx, u16 attr, const void *data, int len) return tlv_put(sctx, attr, &__tmp, sizeof(__tmp)); \ } +TLV_PUT_DEFINE_INT(8) TLV_PUT_DEFINE_INT(32) TLV_PUT_DEFINE_INT(64) @@ -4886,6 +4891,79 @@ static int process_all_new_xattrs(struct send_ctx *sctx) return ret; } +static int send_verity(struct send_ctx *sctx, struct fs_path *path, + struct fsverity_descriptor *desc) +{ + int ret; + + ret = begin_cmd(sctx, BTRFS_SEND_C_ENABLE_VERITY); + if (ret < 0) + goto out; + + TLV_PUT_PATH(sctx, BTRFS_SEND_A_PATH, path); + TLV_PUT_U8(sctx, BTRFS_SEND_A_VERITY_ALGORITHM, le8_to_cpu(desc->hash_algorithm)); + TLV_PUT_U32(sctx, BTRFS_SEND_A_VERITY_BLOCK_SIZE, 1U << le8_to_cpu(desc->log_blocksize)); + TLV_PUT(sctx, BTRFS_SEND_A_VERITY_SALT_DATA, desc->salt, le8_to_cpu(desc->salt_size)); + TLV_PUT(sctx, BTRFS_SEND_A_VERITY_SIG_DATA, desc->signature, le32_to_cpu(desc->sig_size)); + + ret = send_cmd(sctx); + +tlv_put_failure: +out: + return ret; +} + +static int process_verity(struct send_ctx *sctx) +{ + int ret = 0; + struct btrfs_fs_info *fs_info = sctx->send_root->fs_info; + struct inode *inode; + struct fs_path *p; + + inode = btrfs_iget(fs_info->sb, sctx->cur_ino, sctx->send_root); + if (IS_ERR(inode)) + return PTR_ERR(inode); + + ret = btrfs_get_verity_descriptor(inode, NULL, 0); + if (ret < 0) + goto iput; + + if (ret > FS_VERITY_MAX_DESCRIPTOR_SIZE) { + ret = -EMSGSIZE; + goto iput; + } + if (!sctx->verity_descriptor) { + sctx->verity_descriptor = kvmalloc(FS_VERITY_MAX_DESCRIPTOR_SIZE, GFP_KERNEL); + if (!sctx->verity_descriptor) { + ret = -ENOMEM; + goto iput; + } + } + + ret = btrfs_get_verity_descriptor(inode, sctx->verity_descriptor, ret); + if (ret < 0) + goto iput; + + p = fs_path_alloc(); + if (!p) { + ret = -ENOMEM; + goto iput; + } + ret = get_cur_path(sctx, sctx->cur_ino, sctx->cur_inode_gen, p); + if (ret < 0) + goto free_path; + + ret = send_verity(sctx, p, sctx->verity_descriptor); + if (ret < 0) + goto free_path; + +free_path: + fs_path_free(p); +iput: + iput(inode); + return ret; +} + static inline u64 max_send_read_size(const struct send_ctx *sctx) { return sctx->send_max_size - SZ_16K; @@ -6377,6 +6455,11 @@ static int finish_inode_if_needed(struct send_ctx *sctx, int at_end) if (ret < 0) goto out; } + if (sctx->cur_inode_needs_verity) { + ret = process_verity(sctx); + if (ret < 0) + goto out; + } ret = send_capabilities(sctx); if (ret < 0) @@ -6785,6 +6868,18 @@ static int changed_extent(struct send_ctx *sctx, return ret; } +static int changed_verity(struct send_ctx *sctx, + enum btrfs_compare_tree_result result) +{ + int ret = 0; + + if (!sctx->cur_inode_new_gen && !sctx->cur_inode_deleted) { + if (result == BTRFS_COMPARE_TREE_NEW) + sctx->cur_inode_needs_verity = true; + } + return ret; +} + static int dir_changed(struct send_ctx *sctx, u64 dir) { u64 orig_gen, new_gen; @@ -6939,6 +7034,9 @@ static int changed_cb(struct btrfs_path *left_path, ret = changed_xattr(sctx, result); else if (key->type == BTRFS_EXTENT_DATA_KEY) ret = changed_extent(sctx, result); + else if (key->type == BTRFS_VERITY_DESC_ITEM_KEY && + key->offset == 0) + ret = changed_verity(sctx, result); } out: @@ -8036,6 +8134,7 @@ long btrfs_ioctl_send(struct inode *inode, struct btrfs_ioctl_send_args *arg) kvfree(sctx->clone_roots); kfree(sctx->send_buf_pages); kvfree(sctx->send_buf); + kvfree(sctx->verity_descriptor); name_cache_free(sctx); diff --git a/fs/btrfs/send.h b/fs/btrfs/send.h index 4bb4e6a638cb..0a4537775e0c 100644 --- a/fs/btrfs/send.h +++ b/fs/btrfs/send.h @@ -92,8 +92,11 @@ enum btrfs_send_cmd { BTRFS_SEND_C_ENCODED_WRITE = 25, BTRFS_SEND_C_MAX_V2 = 25, + /* Version 3 */ + BTRFS_SEND_C_ENABLE_VERITY = 26, + BTRFS_SEND_C_MAX_V3 = 26, /* End */ - BTRFS_SEND_C_MAX = 25, + BTRFS_SEND_C_MAX = 26, }; /* attributes in send stream */ @@ -160,8 +163,14 @@ enum { BTRFS_SEND_A_ENCRYPTION = 31, BTRFS_SEND_A_MAX_V2 = 31, - /* End */ - BTRFS_SEND_A_MAX = 31, + /* Version 3 */ + BTRFS_SEND_A_VERITY_ALGORITHM = 32, + BTRFS_SEND_A_VERITY_BLOCK_SIZE = 33, + BTRFS_SEND_A_VERITY_SALT_DATA = 34, + BTRFS_SEND_A_VERITY_SIG_DATA = 35, + BTRFS_SEND_A_MAX_V3 = 35, + + __BTRFS_SEND_A_MAX = 35, }; long btrfs_ioctl_send(struct inode *inode, struct btrfs_ioctl_send_args *arg); diff --git a/fs/btrfs/verity.c b/fs/btrfs/verity.c index 90eb5c2830a9..ee00e33c309e 100644 --- a/fs/btrfs/verity.c +++ b/fs/btrfs/verity.c @@ -659,8 +659,7 @@ static int btrfs_end_enable_verity(struct file *filp, const void *desc, * * Returns the size on success or a negative error code on failure. */ -static int btrfs_get_verity_descriptor(struct inode *inode, void *buf, - size_t buf_size) +int btrfs_get_verity_descriptor(struct inode *inode, void *buf, size_t buf_size) { u64 true_size; int ret = 0; diff --git a/fs/verity/fsverity_private.h b/fs/verity/fsverity_private.h index 629785c95007..dbe1ce5b450a 100644 --- a/fs/verity/fsverity_private.h +++ b/fs/verity/fsverity_private.h @@ -70,8 +70,6 @@ struct fsverity_info { const struct inode *inode; }; -/* Arbitrary limit to bound the kmalloc() size. Can be changed. */ -#define FS_VERITY_MAX_DESCRIPTOR_SIZE 16384 #define FS_VERITY_MAX_SIGNATURE_SIZE (FS_VERITY_MAX_DESCRIPTOR_SIZE - \ sizeof(struct fsverity_descriptor)) diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h index 7af030fa3c36..40f14e5fed9d 100644 --- a/include/linux/fsverity.h +++ b/include/linux/fsverity.h @@ -22,6 +22,9 @@ */ #define FS_VERITY_MAX_DIGEST_SIZE SHA512_DIGEST_SIZE +/* Arbitrary limit to bound the kmalloc() size. Can be changed. */ +#define FS_VERITY_MAX_DESCRIPTOR_SIZE 16384 + /* Verity operations for filesystems */ struct fsverity_operations {
Preserve the fs-verity status of a btrfs file across send/recv. There is no facility for installing the Merkle tree contents directly on the receiving filesystem, so we package up the parameters used to enable verity found in the verity descriptor. This gives the receive side enough information to properly enable verity again. Note that this means that receive will have to re-compute the whole Merkle tree, similar to how compression worked before encoded_write. Since the file becomes read-only after verity is enabled, it is important that verity is added to the send stream after any file writes. Therefore, when we process a verity item, merely note that it happened, then actually create the command in the send stream during 'finish_inode_if_needed'. This also creates V3 of the send stream format, without any format changes besides adding the new commands and attributes. Signed-off-by: Boris Burkov <boris@bur.io> -- Changes in v4: - Use btrfs_get_verity_descriptor instead of verity ops get descriptor. Move that definition to ctree.h for conditional building. This cleaned up most of the conditional building issues, in my opinion. - Rename process_new_verity to process_verity. - Use le-to-cpu conversion for all fsverity descriptor fields. - Don't check NULL for kvfree of the send descriptor. Changes in v3: - Fixed build failure when CONFIG_FS_VERITY was not set. This required a kludge to avoid a build warning as well. Changes in v2: - Allocate 16K with kvmalloc and keep it around till the end of send instead of re-allocating on each file with fs-verity. - Use unsigned literal for bitshift. --- fs/btrfs/ctree.h | 6 +++ fs/btrfs/send.c | 99 ++++++++++++++++++++++++++++++++++++ fs/btrfs/send.h | 15 ++++-- fs/btrfs/verity.c | 3 +- fs/verity/fsverity_private.h | 2 - include/linux/fsverity.h | 3 ++ 6 files changed, 121 insertions(+), 7 deletions(-)