| Message ID | 20180718165545.1622-4-colyli@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 19, 2018 at 12:55:45AM +0800, Coly Li wrote: > This patch adds a kernel module to test the consistency of multiple crc > calculation in Linux kernel. It is enabled with CONFIG_TEST_CRC enabled. > > The test results are printed into kernel message, which look like, > > test_crc: crc64_be: FAILED (0x03d4d0d85685d9a1, expected 0x3d4d0d85685d9a1f) > > kernel 0day system has framework to check kernel message, then the above > result can be handled by 0day system. If crc calculation inconsistency > happens, it can be detected quite soon. > > lib/test_crc.c is a testing frame work for many crc consistency > testings. For now, there is only one test caes for crc64_be(). Are you aware there's already a CRC-32 test module: CONFIG_CRC32_SELFTEST and lib/crc32test.c? Confusingly, your patch uses a different naming convention for the new CRC-64 one, and puts the Kconfig option in a different place, and makes it sound like it's a generic test for all CRC implementations rather than just the CRC-64 one. Please use the existing convention (i.e. add CONFIG_CRC64_SELFTEST and lib/crc64test.c) unless you have a strong argument for why it should be done differently. (And I don't think it makes sense to combine all CRC tests into one module, since you should be able to e.g. enable just CRC32 and CRC32_SELFTEST without also pulling in a dependency on all the other CRC variants.) > +/* Add your crc test cases here */ > +static void test_crc64_be(struct crc_test_record *rec) > +{ > + u64 crc; > + > + crc = crc64_be(rec->initval, rec->data, sizeof(rec->data)); > + chk_and_msg(rec->name, crc, rec->expval); > +} > + > +/* > + * Set up your crc test initial data here. > + * Do not change the existing items, they are hard coded with > + * pre-calculated values. > + */ > +static struct crc_test_record test_data[] = { > + { .name = "crc64_be", > + .data = { 0x42F0E1EBA9EA3693, 0x85E1C3D753D46D26, > + 0xC711223CFA3E5BB5, 0x493366450E42ECDF }, > + .initval = 0x61C8864680B583EB, > + .expval = 0xb2c863673f4292bf, > + .handler = test_crc64_be, > + }, > + {} > +}; This is incorrect; the test is checksumming data that has a CPU-specific endianness. So, it will fail on big-endian systems. The data needs to be declared as a byte or char array instead. See e.g. what crypto/testmgr.h does for crypto API algorithms. Also please mark the test data structures 'const'. - Eric
On 2018/7/24 12:44 PM, Eric Biggers wrote: > On Thu, Jul 19, 2018 at 12:55:45AM +0800, Coly Li wrote: >> This patch adds a kernel module to test the consistency of multiple crc >> calculation in Linux kernel. It is enabled with CONFIG_TEST_CRC enabled. >> >> The test results are printed into kernel message, which look like, >> >> test_crc: crc64_be: FAILED (0x03d4d0d85685d9a1, expected 0x3d4d0d85685d9a1f) >> >> kernel 0day system has framework to check kernel message, then the above >> result can be handled by 0day system. If crc calculation inconsistency >> happens, it can be detected quite soon. >> >> lib/test_crc.c is a testing frame work for many crc consistency >> testings. For now, there is only one test caes for crc64_be(). > > Are you aware there's already a CRC-32 test module: CONFIG_CRC32_SELFTEST and > lib/crc32test.c? Confusingly, your patch uses a different naming convention for > the new CRC-64 one, and puts the Kconfig option in a different place, and makes > it sound like it's a generic test for all CRC implementations rather than just > the CRC-64 one. Please use the existing convention (i.e. add > CONFIG_CRC64_SELFTEST and lib/crc64test.c) unless you have a strong argument for > why it should be done differently. > > (And I don't think it makes sense to combine all CRC tests into one module, > since you should be able to e.g. enable just CRC32 and CRC32_SELFTEST without > also pulling in a dependency on all the other CRC variants.) > Hi Eric, The purpose of test_crc is to provide a unified crc calculation consistency testing for 0day. So far it is only crc64, and I will add more test cases later. I see there is crc-32 test module, which does more testing then consistency check, and no unified format for 0day system to detect. This is why people suggested me to add this test framework. >> +/* Add your crc test cases here */ >> +static void test_crc64_be(struct crc_test_record *rec) >> +{ >> + u64 crc; >> + >> + crc = crc64_be(rec->initval, rec->data, sizeof(rec->data)); >> + chk_and_msg(rec->name, crc, rec->expval); >> +} >> + >> +/* >> + * Set up your crc test initial data here. >> + * Do not change the existing items, they are hard coded with >> + * pre-calculated values. >> + */ >> +static struct crc_test_record test_data[] = { >> + { .name = "crc64_be", >> + .data = { 0x42F0E1EBA9EA3693, 0x85E1C3D753D46D26, >> + 0xC711223CFA3E5BB5, 0x493366450E42ECDF }, >> + .initval = 0x61C8864680B583EB, >> + .expval = 0xb2c863673f4292bf, >> + .handler = test_crc64_be, >> + }, >> + {} >> +}; > > This is incorrect; the test is checksumming data that has a CPU-specific > endianness. So, it will fail on big-endian systems. The data needs to be > declared as a byte or char array instead. See e.g. what crypto/testmgr.h does > for crypto API algorithms. > > Also please mark the test data structures 'const'. Sure, I will send fix patches (not rebase the posted patches because they are in linux-next for now) soon. Thanks for your comments. Coly Li
On Wed, Jul 25, 2018 at 12:28:15AM +0800, Coly Li wrote: > On 2018/7/24 12:44 PM, Eric Biggers wrote: > > On Thu, Jul 19, 2018 at 12:55:45AM +0800, Coly Li wrote: > >> This patch adds a kernel module to test the consistency of multiple crc > >> calculation in Linux kernel. It is enabled with CONFIG_TEST_CRC enabled. > >> > >> The test results are printed into kernel message, which look like, > >> > >> test_crc: crc64_be: FAILED (0x03d4d0d85685d9a1, expected 0x3d4d0d85685d9a1f) > >> > >> kernel 0day system has framework to check kernel message, then the above > >> result can be handled by 0day system. If crc calculation inconsistency > >> happens, it can be detected quite soon. > >> > >> lib/test_crc.c is a testing frame work for many crc consistency > >> testings. For now, there is only one test caes for crc64_be(). > > > > Are you aware there's already a CRC-32 test module: CONFIG_CRC32_SELFTEST and > > lib/crc32test.c? Confusingly, your patch uses a different naming convention for > > the new CRC-64 one, and puts the Kconfig option in a different place, and makes > > it sound like it's a generic test for all CRC implementations rather than just > > the CRC-64 one. Please use the existing convention (i.e. add > > CONFIG_CRC64_SELFTEST and lib/crc64test.c) unless you have a strong argument for > > why it should be done differently. > > > > (And I don't think it makes sense to combine all CRC tests into one module, > > since you should be able to e.g. enable just CRC32 and CRC32_SELFTEST without > > also pulling in a dependency on all the other CRC variants.) > > > > Hi Eric, > > The purpose of test_crc is to provide a unified crc calculation > consistency testing for 0day. So far it is only crc64, and I will add > more test cases later. I see there is crc-32 test module, which does > more testing then consistency check, and no unified format for 0day > system to detect. This is why people suggested me to add this test > framework. > Actually the code in crc32test is nearly the same as what you're adding for CRC-64. The CRC-32 test is longer because it's testing two different polynomials "crc32" and "crc32c" as well as combining CRC's; neither of those is relevant for CRC-64 yet, as you've implemented just one polynomial and there is no function provided to combine CRC64's yet. The CRC-32 test also tests performance, but if you don't believe CRC performance should be tested, then you should remove the performance test from the existing module rather than implementing a brand new test module just to remove the performance test... I still don't understand why you decided to do things differently for CRC-64, when there were already CRC-32 tests that used a certain convention for the Kconfig option, filename, etc. It's inconsistent and confusing. Again, please use the existing convention unless you have a strong argument for why it should be done differently. (And if you do want to do things differently, the existing test should be converted first.) - Eric
On 2018/7/25 1:39 AM, Eric Biggers wrote: > On Wed, Jul 25, 2018 at 12:28:15AM +0800, Coly Li wrote: >> On 2018/7/24 12:44 PM, Eric Biggers wrote: >>> On Thu, Jul 19, 2018 at 12:55:45AM +0800, Coly Li wrote: >>>> This patch adds a kernel module to test the consistency of multiple crc >>>> calculation in Linux kernel. It is enabled with CONFIG_TEST_CRC enabled. >>>> >>>> The test results are printed into kernel message, which look like, >>>> >>>> test_crc: crc64_be: FAILED (0x03d4d0d85685d9a1, expected 0x3d4d0d85685d9a1f) >>>> >>>> kernel 0day system has framework to check kernel message, then the above >>>> result can be handled by 0day system. If crc calculation inconsistency >>>> happens, it can be detected quite soon. >>>> >>>> lib/test_crc.c is a testing frame work for many crc consistency >>>> testings. For now, there is only one test caes for crc64_be(). >>> >>> Are you aware there's already a CRC-32 test module: CONFIG_CRC32_SELFTEST and >>> lib/crc32test.c? Confusingly, your patch uses a different naming convention for >>> the new CRC-64 one, and puts the Kconfig option in a different place, and makes >>> it sound like it's a generic test for all CRC implementations rather than just >>> the CRC-64 one. Please use the existing convention (i.e. add >>> CONFIG_CRC64_SELFTEST and lib/crc64test.c) unless you have a strong argument for >>> why it should be done differently. >>> >>> (And I don't think it makes sense to combine all CRC tests into one module, >>> since you should be able to e.g. enable just CRC32 and CRC32_SELFTEST without >>> also pulling in a dependency on all the other CRC variants.) >>> >> >> Hi Eric, >> >> The purpose of test_crc is to provide a unified crc calculation >> consistency testing for 0day. So far it is only crc64, and I will add >> more test cases later. I see there is crc-32 test module, which does >> more testing then consistency check, and no unified format for 0day >> system to detect. This is why people suggested me to add this test >> framework. >> > > Actually the code in crc32test is nearly the same as what you're adding for > CRC-64. The CRC-32 test is longer because it's testing two different > polynomials "crc32" and "crc32c" as well as combining CRC's; neither of those is > relevant for CRC-64 yet, as you've implemented just one polynomial and there is > no function provided to combine CRC64's yet. The CRC-32 test also tests > performance, but if you don't believe CRC performance should be tested, then you > should remove the performance test from the existing module rather than > implementing a brand new test module just to remove the performance test... > > I still don't understand why you decided to do things differently for CRC-64, > when there were already CRC-32 tests that used a certain convention for the > Kconfig option, filename, etc. It's inconsistent and confusing. Again, please > use the existing convention unless you have a strong argument for why it should > be done differently. (And if you do want to do things differently, the existing > test should be converted first.) Hi Eric, So far only crc32 has selftesting code, it is hardly to be a convention IMHO. Anyway, considerate your comments and suggestion, I feel crc_test can be a separate patch from this series. Later I will post another series, which unify all crc test together into test_crc, including other crc calculations which don't have their testing code. Thanks. Coly Li
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 8838d1158d19..a9c1de0c2a7d 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -1911,6 +1911,16 @@ config TEST_SYSCTL If unsure, say N. +config TEST_CRC + tristate "CRC calculation test driver" + depends on CRC64 + help + This builds the "test_crc" module. This driver enables to test the + CRC calculation consistency to make sure new modification does not + break existing checksum calculation. + + if unsure, say N. + config TEST_UDELAY tristate "udelay test driver" default n diff --git a/lib/Makefile b/lib/Makefile index 40c215181687..224d047d026a 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -49,6 +49,7 @@ obj-$(CONFIG_FIND_BIT_BENCHMARK) += find_bit_benchmark.o obj-$(CONFIG_TEST_BPF) += test_bpf.o obj-$(CONFIG_TEST_FIRMWARE) += test_firmware.o obj-$(CONFIG_TEST_SYSCTL) += test_sysctl.o +obj-$(CONFIG_TEST_CRC) += test_crc.o obj-$(CONFIG_TEST_HASH) += test_hash.o test_siphash.o obj-$(CONFIG_TEST_KASAN) += test_kasan.o CFLAGS_test_kasan.o += -fno-builtin diff --git a/lib/test_crc.c b/lib/test_crc.c new file mode 100644 index 000000000000..324e8ad419d7 --- /dev/null +++ b/lib/test_crc.c @@ -0,0 +1,93 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * CRC test driver + * + * Copyright (C) 2018 Coly Li <colyli@suse.de> + * + * This module provides an simple framework to check the consistency of + * Linux kernel CRC calculation routines in lib/crc*.c. This driver + * requires CONFIG_CRC* items to be enabled if the associated routines are + * tested here. The test results will be printed to kernel message + * when this test driver is loaded. + * + * Current test routines are, + * - crc64_be() + */ + +#include <linux/module.h> +#include <linux/crc64.h> + +struct crc_test_record { + char *name; + u64 data[4]; + u64 initval; + u64 expval; + void (*handler)(struct crc_test_record *rec); +}; + +int failed_tests; +int total_tests; + +static void chk_and_msg(const char *name, u64 crc, u64 expval) +{ + total_tests++; + if (crc == expval) + return; + + pr_err("test_crc: %s: FAILED:(0x%016llx, expected 0x%016llx)\n", + name, crc, expval); + failed_tests++; +} + +/* Add your crc test cases here */ +static void test_crc64_be(struct crc_test_record *rec) +{ + u64 crc; + + crc = crc64_be(rec->initval, rec->data, sizeof(rec->data)); + chk_and_msg(rec->name, crc, rec->expval); +} + +/* + * Set up your crc test initial data here. + * Do not change the existing items, they are hard coded with + * pre-calculated values. + */ +static struct crc_test_record test_data[] = { + { .name = "crc64_be", + .data = { 0x42F0E1EBA9EA3693, 0x85E1C3D753D46D26, + 0xC711223CFA3E5BB5, 0x493366450E42ECDF }, + .initval = 0x61C8864680B583EB, + .expval = 0xb2c863673f4292bf, + .handler = test_crc64_be, + }, + {} +}; + +static int __init test_crc_init(void) +{ + int i; + + failed_tests = 0; + total_tests = 0; + + pr_info("Kernel CRC consistency testing:\n"); + for (i = 0; test_data[i].name; i++) + test_data[i].handler(&test_data[i]); + + if (failed_tests == 0) + pr_info("test_crc: all %d tests passed\n", i); + else + pr_err("test_crc: %d cases tested, %d passed, %d failed\n", + total_tests, total_tests - failed_tests, failed_tests); + + return (failed_tests == 0) ? 0 : -EINVAL; +} +late_initcall(test_crc_init); + +static void __exit test_crc_exit(void) { } +module_exit(test_crc_exit); + +MODULE_DESCRIPTION("CRC consistency testing driver"); +MODULE_AUTHOR("Coly Li <colyli@suse.de>"); +MODULE_LICENSE("GPL v2");