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
