Re: [PATCH v3] crypto: xxhash - Implement xxhash support

[Eric Biggers <ebiggers@kernel.org>]

Hi Nikolay, some more comments from another read through.  Sorry for not
noticing these in v2.

On Wed, May 29, 2019 at 06:48:26PM +0300, Nikolay Borisov wrote:
> +static int xxhash64_update(struct shash_desc *desc, const u8 *data,
> +			 unsigned int length)
> +{
> +	struct xxhash64_desc_ctx *tctx = shash_desc_ctx(desc);

This variable should be named 'dctx' (for desc_ctx), not 'tctx' (for tfm_ctx).

> +
> +	xxh64_update(&tctx->xxhstate, data, length);
> +
> +	return 0;
> +}

xxh64_update() has a return value (0 or -errno) which is not being checked,
which at first glance seems to be a bug.

However, it only returns an error in this case:

        if (input == NULL)
                return -EINVAL;

But data=NULL, length=0 are valid parameters to xxhash64_update(), so if you did
check the return value, xxhash64_update() would break.  So it's fine as-is.

However, if anyone changed xxh64_update() to an error in any other case,
xxhash64_update() would break since it ignores the error.

I suggest avoiding this complexity around error codes by changing xxh64_update()
to return void.  It can be a separate patch.

> +
> +static int xxhash64_final(struct shash_desc *desc, u8 *out)
> +{
> +	struct xxhash64_desc_ctx *ctx = shash_desc_ctx(desc);
> +

For consistency it should be 'dctx' here too.

> +	put_unaligned_le64(xxh64_digest(&ctx->xxhstate), out);
> +
> +	return 0;
> +}
> +

> +static int xxhash64_finup(struct shash_desc *desc, const u8 *data,
> +			unsigned int len, u8 *out)
> +{
> +	xxhash64_update(desc, data, len);
> +	xxhash64_final(desc, out);
> +
> +	return 0;
> +}
> +
> +static int xxhash64_digest(struct shash_desc *desc, const u8 *data,
> +			 unsigned int length, u8 *out)
> +{
> +	xxhash64_init(desc);
> +	return xxhash64_finup(desc, data, length, out);
> +}
> +

The purpose of the ->finup() and ->digest() methods is to allow the algorithm to
work more efficiently than it could if ->init(), ->update(), and ->final() were
called separately.  So, implementing them on top of xxhash64_init(),
xxhash64_update(), and xxhash64_final() is mostly pointless.

lib/xxhash.c already provides a function xxh64() which does a digest in one
step, so that should be used to implement xxhash64_digest():

static int xxhash64_digest(struct shash_desc *desc, const u8 *data,
			 unsigned int length, u8 *out)
{
	struct xxhash64_tfm_ctx *tctx = crypto_shash_ctx(desc->tfm);

	put_unaligned_le64(xxh64(data, length, tctx->seed), out);

	return 0;
}

I suggest just deleting xxhash64_finup().

> +static struct shash_alg alg = {
> +	.digestsize	= XXHASH64_DIGEST_SIZE,
> +	.setkey		= xxhash64_setkey,
> +	.init		= xxhash64_init,
> +	.update		= xxhash64_update,
> +	.final		= xxhash64_final,
> +	.finup		= xxhash64_finup,
> +	.digest		= xxhash64_digest,
> +	.descsize	= sizeof(struct xxhash64_desc_ctx),
> +	.base		= {
> +		.cra_name	 = "xxhash64",
> +		.cra_driver_name = "xxhash64-generic",
> +		.cra_priority	 = 100,
> +		.cra_flags	 = CRYPTO_ALG_OPTIONAL_KEY,
> +		.cra_blocksize	 = XXHASH64_BLOCK_SIZE,
> +		.cra_ctxsize	 = sizeof(struct xxhash64_tfm_ctx),
> +		.cra_module	 = THIS_MODULE,
> +	}
> +};

Note that because .export() and .import() aren't set, if someone calls
crypto_shash_export() and crypto_shash_import() on an xxhash64 descriptor, the
crypto API will export and import the state by memcpy().

That's perfectly fine, but it also means that the xxh64_copy_state() function
won't be called.  Since it exists, one might assume that all state copies go
through that function.  But it's actually pointless as it just does a memcpy, so
I suggest removing it.  (As a separate patch, which you don't necessarily have
to do now.  BTW, another thing that should be cleaned up is the random
unnecessary local variable in xxh32_reset() and xxh64_reset()...)

Thanks,

- Eric