On 29.05.19 г. 22:55 ч., Eric Biggers wrote:
> 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()...)
Good suggestion, however they stretch beyond what I'm currently
comfortable doing. I will do a v4 which deals with the minor
inconsistencies in the crypto module (variable naming, removing finup,
implementing digest via xxh64), however I will definitely won't be doing
any changes in lib/xxhash.c now. I don't think any of the things you
mentioned in that regard are a blockers towards merging crypto layer
support for xxhash.
>
> Thanks,
>
> - Eric
>
