Re: [PATCH 14/17] btrfs: directly call into crypto framework for checsumming

[Chris Mason <clm@fb.com>]

On 10 May 2019, at 7:15, Johannes Thumshirn wrote:

> Currently btrfs_csum_data() relied on the crc32c() wrapper around the 
> crypto
> framework for calculating the CRCs.
>
> As we have our own crypto_shash structure in the fs_info now, we can
> directly call into the crypto framework without going trough the 
> wrapper.

It has been a while since I dug through the cryptoapi internals.  I have 
one vague and somewhat unfounded concern, where we're defining the disk 
format to be whatever is returned from looking up sha256 or crc32c in 
the crypto tables.  I'm not really sure how to resolve this since we 
obviously don't want our own sha256 implementation.

I'm a little concerned about about btrfs_csum_data() and 
btrfs_csum_final() below.  We're using two different 
SHASH_DESC_ON_STACK() and then overwriting internals (shash_desc_ctx()) 
with the assumption that whatever we're doing is going to be the same as 
using the same shash_desc struct for both the update and final calls.  I 
think we should be either using or adding a helper to the crypto api for 
this.  We're digging too deep into cryptoapi private structs with the 
current usage.

Otherwise, thanks for doing this, it looks great overall.

-chris

>
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  fs/btrfs/disk-io.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index fb0b06b7b9f6..0c9ac7b45ce8 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -253,12 +253,36 @@ struct extent_map *btree_get_extent(struct 
> btrfs_inode *inode,
>  u32 btrfs_csum_data(struct btrfs_fs_info *fs_info, const char *data,
>  		    u32 seed, size_t len)
>  {
> -	return crc32c(seed, data, len);
> +	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
> +	u32 *ctx = (u32 *)shash_desc_ctx(shash);
> +	u32 retval;
> +	int err;
> +
> +	shash->tfm = fs_info->csum_shash;
> +	shash->flags = 0;
> +	*ctx = seed;
> +
> +	err = crypto_shash_update(shash, data, len);
> +	ASSERT(err);
> +
> +	retval = *ctx;
> +	barrier_data(ctx);
> +
> +	return retval;
>  }
>
>  void btrfs_csum_final(struct btrfs_fs_info *fs_info, u32 crc, u8 
> *result)
>  {
> -	put_unaligned_le32(~crc, result);
> +	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
> +	u32 *ctx = (u32 *)shash_desc_ctx(shash);
> +	int err;
> +
> +	shash->tfm = fs_info->csum_shash;
> +	shash->flags = 0;
> +	*ctx = crc;
> +
> +	err = crypto_shash_final(shash, result);
> +	ASSERT(err);
>  }
>
>  /*
> -- 
> 2.16.4