On 27.05.19 г. 21:39 ч., Eric Biggers wrote:
> Hi Nikolay,
>
> On Mon, May 27, 2019 at 05:28:10PM +0300, Nikolay Borisov wrote:
>> xxhash is currently implemented as a self-contained module in /lib.
>> This patch enables that module to be used as part of the generic kernel
>> crypto framework. It adds a simple wrapper to the 64bit version. I've
>> also added a couple of simplistic test vectors to ensure I haven't
>> screwed up anything doing the plumbing.
>
> What is this planned to be used for?
Possibly as a replacement hash of crc32c in btrfs.
>
> Please also run the crypto self-tests (i.e. boot a kernel with
> CONFIG_CRYPTO_MANAGER_DISABLE_TESTS unset) for all crypto API patches.
> When that's done this code immediately crashes, see output below.
Not for me, I tested the vectors I've added for correct results. I
wouldn't have sent the patch otherwise. Strange why I didn't observe
this crash, I wonder if it's due to an unaligned buffer or digest not
initialising the state.
>
> More comments below.
>
> [ 0.305235] BUG: unable to handle page fault for address: ffff88817c1966fe
> [ 0.306613] #PF: supervisor write access in kernel mode
> [ 0.307653] #PF: error_code(0x0002) - not-present page
> [ 0.308503] PGD 2a01067 P4D 2a01067 PUD 0
> [ 0.308503] Oops: 0002 [#1] SMP
> [ 0.308503] CPU: 3 PID: 59 Comm: cryptomgr_test Not tainted 5.2.0-rc2-00001-g4fcf4df09e23d #3
> [ 0.308503] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-20181126_142135-anatol 04/01/2014
> [ 0.308503] RIP: 0010:__memcpy+0x12/0x20
> [ 0.308503] Code: 8b 43 60 48 2b 43 50 88 43 4e 5b 5d c3 c3 90 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 48 89 f8 48 89 d1 48 c1 e9 4
> [ 0.308503] RSP: 0018:ffffc900006b7ab0 EFLAGS: 00010202
> [ 0.308503] RAX: ffff88817c1966fe RBX: ffff88807d1a67d8 RCX: 0000000000202024
> [ 0.308503] RDX: 0000000000000002 RSI: ffff88807c996000 RDI: ffff88817c1966fe
> [ 0.308503] RBP: ffffc900006b7ad8 R08: ffffffff81c81bf0 R09: 0000000000000000
> [ 0.308503] R10: ffff88807c996000 R11: ffffffff81a33fb0 R12: 0000000000000007
> [ 0.308503] R13: ffff88807c996000 R14: 0000000000000020 R15: ffffffff81a373c8
> [ 0.308503] FS: 0000000000000000(0000) GS:ffff88807fd80000(0000) knlGS:0000000000000000
> [ 0.308503] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 0.308503] CR2: ffff88817c1966fe CR3: 0000000001c0f000 CR4: 00000000003406e0
> [ 0.308503] Call Trace:
> [ 0.308503] ? xxh64_update+0x51/0x1e0
> [ 0.308503] xxhash64_finup+0x18/0x30
> [ 0.308503] xxhash64_digest+0x9/0x10
> [ 0.308503] crypto_shash_digest+0x24/0x40
> [ 0.308503] shash_ahash_digest+0x9a/0xf0
> [ 0.308503] ? shash_ahash_digest+0xf0/0xf0
> [ 0.308503] shash_async_digest+0x19/0x20
> [ 0.308503] crypto_ahash_op+0x24/0x60
> [ 0.308503] crypto_ahash_digest+0x16/0x20
> [ 0.308503] do_ahash_op.constprop.12+0x10/0x40
> [ 0.308503] test_hash_vec_cfg+0x205/0x610
> [ 0.308503] ? _raw_spin_unlock+0x11/0x30
> [ 0.308503] ? sprintf+0x56/0x70
> [ 0.308503] __alg_test_hash.isra.8+0x115/0x1d0
> [ 0.308503] alg_test_hash+0x7b/0x100
> [ 0.308503] alg_test+0xb6/0x375
> [ 0.308503] ? __kthread_parkme+0x5c/0x90
> [ 0.308503] ? lockdep_hardirqs_on+0xf6/0x190
> [ 0.308503] ? _raw_spin_unlock_irqrestore+0x44/0x50
> [ 0.308503] ? trace_hardirqs_on+0x22/0xf0
> [ 0.308503] ? __kthread_parkme+0x2a/0x90
> [ 0.308503] cryptomgr_test+0x26/0x40
> [ 0.308503] kthread+0x124/0x140
> [ 0.308503] ? cryptomgr_probe+0xd0/0xd0
> [ 0.308503] ? __kthread_create_on_node+0x1c0/0x1c0
> [ 0.308503] ret_from_fork+0x24/0x30
> [ 0.308503] CR2: ffff88817c1966fe
> [ 0.308503] ---[ end trace 3ee93ad10b0b79d0 ]---
>
>> diff --git a/crypto/xxhash_generic.c b/crypto/xxhash_generic.c
>> new file mode 100644
>> index 000000000000..aedaabe55d45
>> --- /dev/null
>> +++ b/crypto/xxhash_generic.c
>> @@ -0,0 +1,112 @@
>> +#include <crypto/internal/hash.h>
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/xxhash.h>
>> +
>> +#define XXHASH_BLOCK_SIZE 1
>
> This should be set to 32 bytes (the "stripe size" of xxhash64), to match the
> size of the chunks in which the algorithm processes data. I.e., if you pass 31
> bytes to xxhash64 it just buffers them, without doing any real work yet.
>
> A "block size" of 1 is only appropriate for algorithms like CRC whose simplest
> mathematical description operates directly on bytes.
>
>> +#define XXHASH64_DIGEST_SIZE 8
>> +
>> +struct xxhash64_crypto_ctx {
>> + u64 seed;
>> +};
>
> To make it clearer what kind of "context" this is, please name this
> "xxhash64_tfm_ctx", not "xxhash64_crypto_ctx".
>
> Similarly, name the variables "tctx" instead of "cctx".
ACK
>
>> +
>> +struct xxhash64_desc_ctx {
>> + struct xxh64_state xxhstate;
>> + u64 seed;
>> +};
>> +
>> +static int xxhash64_init(struct shash_desc *desc)
>> +{
>> + struct xxhash64_crypto_ctx *cctx = crypto_shash_ctx(desc->tfm);
>> + struct xxhash64_desc_ctx *dctx = shash_desc_ctx(desc);
>> +
>> + dctx->seed = cctx->seed;
>> + xxh64_reset(&dctx->xxhstate, dctx->seed);
>> +
>> + return 0;
>> +}
>
> What's the point of storing 'seed' in the desc_ctx, given that it's already
> represented in the initialized 'xxhstate'?
Good point will remove seed in next iteration.
>
>> +
>> +static int xxhash64_setkey(struct crypto_shash *tfm, const u8 *key,
>> + unsigned int keylen)
>> +{
>> + struct xxhash64_crypto_ctx *ctx = crypto_shash_ctx(tfm);
>> +
>> + if (keylen != sizeof(ctx->seed)) {
>> + crypto_shash_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
>> + return -EINVAL;
>> + }
>> + ctx->seed = *(u64 *)key;
>> + return 0;
>> +}
>
> The crypto API takes the "key" as a byte array which might be unaligned. Also,
> the sequence of "key" bytes is conventionally interpreted the same way on all
> architectures, e.g. it's not endian-dependent. So, I suggest:
>
> ctx->seed = get_unaligned_le64(key);
Can it actually be unaligned though, looking at
crypto_shash_setkey->shash_setkey_unaligned it seems that the generic
layer aligns the buffer and passes the aligned one to ->setkey.
>
> Note that without this fix, your test vectors are wrong on big endian CPUs.
>
>> +
>> +static int xxhash64_update(struct shash_desc *desc, const u8 *data,
>> + unsigned int length)
>> +{
>> + struct xxhash64_desc_ctx *ctx = shash_desc_ctx(desc);
>> +
>> + xxh64_update(&ctx->xxhstate, data, length);
>> +
>> + return 0;
>> +}
>> +
>> +static int xxhash64_final(struct shash_desc *desc, u8 *out)
>> +{
>> + struct xxhash64_desc_ctx *ctx = shash_desc_ctx(desc);
>> +
>> + *(u64 *)out = xxh64_digest(&ctx->xxhstate);
>> +
>> + return 0;
>> +}
>
> Similarly, the 'out' array might be misaligned. And conventionally the same
> bytes are output on all architectures. So I suggest:
>
> put_unaligned_u64(xxh64_digest(&ctx->xxhstate), out);
Fair enough, will fix it in next version.
>
>> +
>> +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)
>> +{
>> + return xxhash64_finup(desc, data, length, out);
>> +}
>> +
>
> It seems that xxhash64_digest() is forgetting to initialize the hash state.
>
>> +static struct shash_alg alg = {
>> + .digestsize = XXHASH64_DIGEST_SIZE,
>> + .setkey = xxhash64_setkey,
>
> Please use consistent indentation.
This was copy/pasted from crc32c struct but yeah, I admit it looks ugly,
will fix.
>
>> + .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 = XXHASH_BLOCK_SIZE,
>> + .cra_ctxsize = sizeof(struct xxhash64_crypto_ctx),
>> + .cra_module = THIS_MODULE,
>> + }
>> +};
>> +
>> +static int __init xxhash_mod_init(void)
>> +{
>> + return crypto_register_shash(&alg);
>> +}
>> +
>> +static void __exit xxhash_mod_fini(void)
>> +{
>> + crypto_unregister_shash(&alg);
>> +}
>> +
>> +module_init(xxhash_mod_init);
>> +module_exit(xxhash_mod_fini);
>> +
>> +MODULE_AUTHOR("Nikolay Borisov <nborisov@xxxxxxxx>");
>> +MODULE_DESCRIPTION("xxhash calculations wrapper for lib/xxhash.c");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS_CRYPTO("xxhash-generic");
>
> The purpose of MODULE_ALIAS_CRYPTO() is to allow the module to be dynamically
> loaded when someone requests the algorithm by name. So, putting a string here
> which doesn't match the algorithm name is wrong. It should be:
>
> MODULE_ALIAS_CRYPTO("xxhash64");
> MODULE_ALIAS_CRYPTO("xxhash64-generic");
>
> - Eric
>
