Re: [PATCH] btrfs: send: add support for fs-verity

[Boris Burkov <boris@bur.io>]

On Thu, Jul 28, 2022 at 01:43:32PM +0200, David Sterba wrote:
> On Wed, Jul 27, 2022 at 04:46:22PM -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>
> > ---
> > @@ -4886,6 +4889,80 @@ 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, desc->hash_algorithm);
> > +	TLV_PUT_U32(sctx, BTRFS_SEND_A_VERITY_BLOCK_SIZE, 1 << desc->log_blocksize);
> 
> bitshifts should be done on unsigned types
> 
> 	1U << desc->log_blocksize
> 
> > +	TLV_PUT(sctx, BTRFS_SEND_A_VERITY_SALT_DATA, desc->salt, desc->salt_size);
> > +	TLV_PUT(sctx, BTRFS_SEND_A_VERITY_SIG_DATA, desc->signature, desc->sig_size);
> > +
> > +	ret = send_cmd(sctx);
> > +
> > +tlv_put_failure:
> > +out:
> > +	return ret;
> > +}
> > +
> > +static int process_new_verity(struct send_ctx *sctx)
> > +{
> > +	int ret = 0;
> > +	struct btrfs_fs_info *fs_info = sctx->send_root->fs_info;
> > +	struct inode *inode;
> > +	struct fsverity_descriptor *desc;
> > +	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 = fs_info->sb->s_vop->get_verity_descriptor(inode, NULL, 0);
> > +	if (ret < 0)
> > +		goto iput;
> > +
> > +	if (ret > FS_VERITY_MAX_DESCRIPTOR_SIZE) {
> > +		ret = -EMSGSIZE;
> > +		goto iput;
> > +	}
> > +	desc = kmalloc(ret, GFP_KERNEL);
> 
> I think that once there's a file with verity record there will be more
> so it would make sense to cache the allocated memory, which means to
> allocate the full size.
> 
> FS_VERITY_MAX_DESCRIPTOR_SIZE is 16K so this should be allocated by
> kvmalloc, like we have for other data during send.
> 
> > +	if (!desc) {
> > +		ret = -ENOMEM;
> > +		goto iput;
> > +	}
> > +
> > +	ret = fs_info->sb->s_vop->get_verity_descriptor(inode, desc, ret);
> > +	if (ret < 0)
> > +		goto free_desc;
> > +
> > +	p = fs_path_alloc();
> > +	if (!p) {
> > +		ret = -ENOMEM;
> > +		goto free_desc;
> > +	}
> > +	ret = get_cur_path(sctx, sctx->cur_ino, sctx->cur_inode_gen, p);
> > +	if (ret < 0)
> > +		goto free_path;
> > +
> > +	ret = send_verity(sctx, p, desc);
> > +	if (ret < 0)
> > +		goto free_path;
> > +
> > +free_path:
> > +	fs_path_free(p);
> > +free_desc:
> > +	kfree(desc);
> > +iput:
> > +	iput(inode);
> > +	return ret;
> > +}
> > +
> >  static inline u64 max_send_read_size(const struct send_ctx *sctx)
> >  {
> >  	return sctx->send_max_size - SZ_16K;
> > --- 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,
> 
> I find the name confusing, it sounds like enabling it for the whole
> filesystem, but it affects only one file.

Your point about fs-wide vs file specific makes sense,  but on the other
hand, that is the name of the ioctl and the cli command.

Would you prefer VERITY_ENABLE? or just VERITY? I am having trouble
thinking of a better name, but I also don't mind if you think of one
and rename it.

> 
> > +	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,
> >  };