[v3,3/3] lib/test_crc: Add test cases for crc calculation

Commit Message

Coly Li July 17, 2018, 2:55 p.m. UTC

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: PASSED (0x4e6b1ff972fa8c55, expected 0x4e6b1ff972fa8c55)
test_crc: crc64_bch: PASSED (0x0e4f1391d7a4a62e, expected 0x0e4f1391d7a4a62e)
test_crc: crc64_update: 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 are only test caes for 3 crc routines,
- crc64()
- crc64_bch()
- crc64_update()

Changelog:
v3: Add test cases passed/failed statistic
    More fixes for review comments of v2
v2: Fixes for review comments of v1
v1: Initial version.

Signed-off-by: Coly Li <[e-mail]>
Reviewed-by: Hannes Reinecke <[e-mail]>
Cc: Greg Kroah-Hartman <[e-mail]>
Cc: Linus Torvalds <[e-mail]>
Cc: Thomas Gleixner <[e-mail]>
Cc: Kate Stewart <[e-mail]>
---
 lib/Kconfig.debug |  10 ++++
 lib/Makefile      |   1 +
 lib/test_crc.c    | 138 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 149 insertions(+)
 create mode 100644 lib/test_crc.c

Comments

Randy Dunlap July 17, 2018, 4:57 p.m. UTC | #1

Hi,

On 07/17/2018 07:55 AM, Coly Li wrote:

> diff --git a/lib/test_crc.c b/lib/test_crc.c
> new file mode 100644
> index 000000000000..441bf835fbd3
> --- /dev/null
> +++ b/lib/test_crc.c
> @@ -0,0 +1,138 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CRC test driver
> + *
> + * Copyright (C) 2018 Coly Li <[e-mail]>
> + *

> +
> +static int __init test_crc_init(void)
> +{
> +	int i;
> +	int v, err = 0;
> +
> +	pr_info("Kernel CRC consitency testing:\n");

	                    consistency

> +	for (i = 0; test_data[i].name; i++) {
> +		v = test_data[i].handler(&test_data[i]);
> +		if (v < 0)
> +			err++;
> +	}
> +
> +	if (err == 0)
> +		pr_info("test_crc: all %d tests passed\n", i);
> +	else
> +		pr_err("test_crc: %d cases tested, %d passed, %d failed\n",
> +		       i, i - err, err);
> +
> +	return (err == 0) ? 0 : -EINVAL;
> +}

Andy Shevchenko July 17, 2018, 5:11 p.m. UTC | #2

On Tue, Jul 17, 2018 at 5:55 PM, Coly Li <[e-mail]> 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: PASSED (0x4e6b1ff972fa8c55, expected 0x4e6b1ff972fa8c55)
> test_crc: crc64_bch: PASSED (0x0e4f1391d7a4a62e, expected 0x0e4f1391d7a4a62e)
> test_crc: crc64_update: 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 are only test caes for 3 crc routines,
> - crc64()
> - crc64_bch()
> - crc64_update()

Thanks for an update. My comments below.

> Changelog:
> v3: Add test cases passed/failed statistic
>     More fixes for review comments of v2
> v2: Fixes for review comments of v1
> v1: Initial version.

Usually this part goes after --- line below.

> Signed-off-by: Coly Li <[e-mail]>
> Reviewed-by: Hannes Reinecke <[e-mail]>
> Cc: Greg Kroah-Hartman <[e-mail]>
> Cc: Linus Torvalds <[e-mail]>
> Cc: Thomas Gleixner <[e-mail]>
> Cc: Kate Stewart <[e-mail]>

Please, Cc me as well this one in next version (use my Intel address).

> +#include <linux/async.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/printk.h>
> +#include <linux/miscdevice.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +#include <linux/vmalloc.h>
> +#include <linux/crc64.h>

Do we need all of them?

> +static int chk_and_msg(const char *name, u64 crc, u64 expval)
> +{

> +       int ret = 0;
> +
> +       if (crc == expval) {

> +               pr_info("test_crc: %s: PASSED:(0x%016llx, expected 0x%016llx)\n",
> +                       name, crc, expval);

This doesn't bring anything useful.

> +       } else {
> +               pr_err("test_crc: %s: FAILED:(0x%016llx, expected 0x%016llx)\n",
> +                       name, crc, expval);
> +               ret = -EINVAL;
> +       }
> +
> +       return ret;

I would rewrite entire function as follows:

static void ...(...)
{
  total_tests++;
  if (crc == expval)
    return;

  pr_err(...);
  failed_tests++;
}


> +}

> +static int __init test_crc_init(void)
> +{
> +       int i;
> +       int v, err = 0;
> +
> +       pr_info("Kernel CRC consitency testing:\n");

> +       for (i = 0; test_data[i].name; i++) {
> +               v = test_data[i].handler(&test_data[i]);
> +               if (v < 0)
> +                       err++;
> +       }

...and correct this to simple
for (...)
 test_data[i].handler(...);

> +       if (err == 0)
> +               pr_info("test_crc: all %d tests passed\n", i);
> +       else
> +               pr_err("test_crc: %d cases tested, %d passed, %d failed\n",
> +                      i, i - err, err);

...and this accordingly.

Note, that in the future someone can add more test cases one or more
of which could not map 1:1 to i here.
That's why the rationale to have two global variables for test statistics.
Also it allows (as you see above) to get rid of return code from all
of those test. We don't interested in them I believe.

> +
> +       return (err == 0) ? 0 : -EINVAL;
> +}

Noah Massey July 17, 2018, 6:51 p.m. UTC | #3

On Tue, Jul 17, 2018 at 10:56 AM Coly Li <[e-mail]> 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: PASSED (0x4e6b1ff972fa8c55, expected 0x4e6b1ff972fa8c55)
> test_crc: crc64_bch: PASSED (0x0e4f1391d7a4a62e, expected 0x0e4f1391d7a4a62e)
> test_crc: crc64_update: 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 are only test caes for 3 crc routines,
> - crc64()
> - crc64_bch()
> - crc64_update()
>
> Changelog:
> v3: Add test cases passed/failed statistic
>     More fixes for review comments of v2
> v2: Fixes for review comments of v1
> v1: Initial version.
>
> Signed-off-by: Coly Li <[e-mail]>
> Reviewed-by: Hannes Reinecke <[e-mail]>
> Cc: Greg Kroah-Hartman <[e-mail]>
> Cc: Linus Torvalds <[e-mail]>
> Cc: Thomas Gleixner <[e-mail]>
> Cc: Kate Stewart <[e-mail]>
> ---
>  lib/Kconfig.debug |  10 ++++
>  lib/Makefile      |   1 +
>  lib/test_crc.c    | 138 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 149 insertions(+)
>  create mode 100644 lib/test_crc.c
>
> 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..441bf835fbd3
> --- /dev/null
> +++ b/lib/test_crc.c
> @@ -0,0 +1,138 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CRC test driver
> + *
> + * Copyright (C) 2018 Coly Li <[e-mail]>
> + *
> + * 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()
> + * - crc64_bch()
> + * - crc64_update()
> + *
> + */
> +
> +#include <linux/async.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/printk.h>
> +#include <linux/miscdevice.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +#include <linux/vmalloc.h>
> +#include <linux/crc64.h>
> +
> +struct crc_test_record {
> +       char    *name;
> +       u64     data[4];
> +       u64     initval;
> +       u64     expval;
> +       int     (*handler)(struct crc_test_record *rec);
> +};
> +
> +static int chk_and_msg(const char *name, u64 crc, u64 expval)
> +{
> +       int ret = 0;
> +
> +       if (crc == expval) {
> +               pr_info("test_crc: %s: PASSED:(0x%016llx, expected 0x%016llx)\n",
> +                       name, crc, expval);

I don't think we should have specific kernel output for passed tests.
If a new test is added which follows this pattern, the 0-day will fail
because the kernel output would change. Along the lines of "silence is
golden", if no test hit the error output, we're good.

> +       } else {
> +               pr_err("test_crc: %s: FAILED:(0x%016llx, expected 0x%016llx)\n",
> +                       name, crc, expval);
> +               ret = -EINVAL;
> +       }
> +
> +       return ret;
> +}
> +
> +/* Add your crc test cases here */
> +static int test_crc64(struct crc_test_record *rec)
> +{
> +       u64 crc;
> +
> +       crc = crc64(rec->data, sizeof(rec->data));
> +       return chk_and_msg(rec->name, crc, rec->expval);
> +}
> +
> +static int test_crc64_bch(struct crc_test_record *rec)
> +{
> +       u64 crc;
> +
> +       crc = crc64_bch(rec->data, sizeof(rec->data));
> +       return chk_and_msg(rec->name, crc, rec->expval);
> +}
> +
> +static int test_crc64_update(struct crc_test_record *rec)
> +{
> +       u64 crc = rec->initval;
> +
> +       crc = crc64_update(crc, rec->data, sizeof(rec->data));
> +       return 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",
> +         .data         = { 0x42F0E1EBA9EA3693, 0x85E1C3D753D46D26,
> +                           0xC711223CFA3E5BB5, 0x493366450E42ECDF },
> +         .initval      = 0,
> +         .expval       = 0xe2b9911e7b997201,
> +         .handler      = test_crc64,
> +       },
> +       { .name         = "crc64_bch",
> +         .data         = { 0x42F0E1EBA9EA3693, 0x85E1C3D753D46D26,
> +                           0xC711223CFA3E5BB5, 0x493366450E42ECDF },
> +         .initval      = 0,
> +         .expval       = 0xd2753a20fd862892,
> +         .handler      = test_crc64_bch,
> +       },
> +       { .name         = "crc64_update",
> +         .data         = { 0x42F0E1EBA9EA3693, 0x85E1C3D753D46D26,
> +                           0xC711223CFA3E5BB5, 0x493366450E42ECDF },
> +         .initval      = 0x61C8864680B583EB,
> +         .expval       = 0xb2c863673f4292bf,
> +         .handler      = test_crc64_update,
> +       },
> +       {}
> +};
> +
> +static int __init test_crc_init(void)
> +{
> +       int i;
> +       int v, err = 0;
> +
> +       pr_info("Kernel CRC consitency testing:\n");
> +       for (i = 0; test_data[i].name; i++) {
> +               v = test_data[i].handler(&test_data[i]);
> +               if (v < 0)
> +                       err++;
> +       }
> +
> +       if (err == 0)
> +               pr_info("test_crc: all %d tests passed\n", i);

Similar to previous comment: we should not report the number of passed
tests, since adding a test would invalidate previous golden output.
Also, consider the situation where some tests are conditionally
executed depending on kconfig.

> +       else
> +               pr_err("test_crc: %d cases tested, %d passed, %d failed\n",
> +                      i, i - err, err);
> +
> +       return (err == 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 <[e-mail]>");
> +MODULE_LICENSE("GPL v2");
> --
> 2.17.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andy Shevchenko July 17, 2018, 8:59 p.m. UTC | #4

On Tue, Jul 17, 2018 at 9:51 PM, Noah Massey <[e-mail]> wrote:
> On Tue, Jul 17, 2018 at 10:56 AM Coly Li <[e-mail]> wrote:

>> +               pr_info("test_crc: %s: PASSED:(0x%016llx, expected 0x%016llx)\n",
>> +                       name, crc, expval);
>
> I don't think we should have specific kernel output for passed tests.
> If a new test is added which follows this pattern, the 0-day will fail
> because the kernel output would change. Along the lines of "silence is
> golden", if no test hit the error output, we're good.

Agree.

>> +       if (err == 0)
>> +               pr_info("test_crc: all %d tests passed\n", i);
>
> Similar to previous comment: we should not report the number of passed
> tests, since adding a test would invalidate previous golden output.
> Also, consider the situation where some tests are conditionally
> executed depending on kconfig.

We do similar in many test modules and I know at least two that had
been changed in order to get new test cases.
Are you proposing to change 'em all?

Coly Li July 18, 2018, 2:53 p.m. UTC | #5

On 2018/7/18 12:57 AM, Randy Dunlap wrote:
> Hi,
> 
> On 07/17/2018 07:55 AM, Coly Li wrote:
> 
>> diff --git a/lib/test_crc.c b/lib/test_crc.c
>> new file mode 100644
>> index 000000000000..441bf835fbd3
>> --- /dev/null
>> +++ b/lib/test_crc.c
>> @@ -0,0 +1,138 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * CRC test driver
>> + *
>> + * Copyright (C) 2018 Coly Li <[e-mail]>
>> + *
> 
>> +
>> +static int __init test_crc_init(void)
>> +{
>> +	int i;
>> +	int v, err = 0;
>> +
>> +	pr_info("Kernel CRC consitency testing:\n");
> 
> 	                    consistency
> 

Fixed in v4 series. Thanks.

Coly Li


[snipped]

Coly Li July 18, 2018, 3:28 p.m. UTC | #6

On 2018/7/18 2:51 AM, Noah Massey wrote:
> On Tue, Jul 17, 2018 at 10:56 AM Coly Li <[e-mail]> 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: PASSED (0x4e6b1ff972fa8c55, expected 0x4e6b1ff972fa8c55)
>> test_crc: crc64_bch: PASSED (0x0e4f1391d7a4a62e, expected 0x0e4f1391d7a4a62e)
>> test_crc: crc64_update: 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 are only test caes for 3 crc routines,
>> - crc64()
>> - crc64_bch()
>> - crc64_update()
>>
>> Changelog:
>> v3: Add test cases passed/failed statistic
>>     More fixes for review comments of v2
>> v2: Fixes for review comments of v1
>> v1: Initial version.
>>
>> Signed-off-by: Coly Li <[e-mail]>
>> Reviewed-by: Hannes Reinecke <[e-mail]>
>> Cc: Greg Kroah-Hartman <[e-mail]>
>> Cc: Linus Torvalds <[e-mail]>
>> Cc: Thomas Gleixner <[e-mail]>
>> Cc: Kate Stewart <[e-mail]>
>> ---
>>  lib/Kconfig.debug |  10 ++++
>>  lib/Makefile      |   1 +
>>  lib/test_crc.c    | 138 ++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 149 insertions(+)
>>  create mode 100644 lib/test_crc.c
>>
>> 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..441bf835fbd3
>> --- /dev/null
>> +++ b/lib/test_crc.c
>> @@ -0,0 +1,138 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * CRC test driver
>> + *
>> + * Copyright (C) 2018 Coly Li <[e-mail]>
>> + *
>> + * 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()
>> + * - crc64_bch()
>> + * - crc64_update()
>> + *
>> + */
>> +
>> +#include <linux/async.h>
>> +#include <linux/delay.h>
>> +#include <linux/fs.h>
>> +#include <linux/list.h>
>> +#include <linux/module.h>
>> +#include <linux/printk.h>
>> +#include <linux/miscdevice.h>
>> +#include <linux/slab.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/vmalloc.h>
>> +#include <linux/crc64.h>
>> +
>> +struct crc_test_record {
>> +       char    *name;
>> +       u64     data[4];
>> +       u64     initval;
>> +       u64     expval;
>> +       int     (*handler)(struct crc_test_record *rec);
>> +};
>> +
>> +static int chk_and_msg(const char *name, u64 crc, u64 expval)
>> +{
>> +       int ret = 0;
>> +
>> +       if (crc == expval) {
>> +               pr_info("test_crc: %s: PASSED:(0x%016llx, expected 0x%016llx)\n",
>> +                       name, crc, expval);
> 
> I don't think we should have specific kernel output for passed tests.
> If a new test is added which follows this pattern, the 0-day will fail
> because the kernel output would change. Along the lines of "silence is
> golden", if no test hit the error output, we're good.
> 
>> +       } else {
>> +               pr_err("test_crc: %s: FAILED:(0x%016llx, expected 0x%016llx)\n",
>> +                       name, crc, expval);
>> +               ret = -EINVAL;
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +/* Add your crc test cases here */
>> +static int test_crc64(struct crc_test_record *rec)
>> +{
>> +       u64 crc;
>> +
>> +       crc = crc64(rec->data, sizeof(rec->data));
>> +       return chk_and_msg(rec->name, crc, rec->expval);
>> +}
>> +
>> +static int test_crc64_bch(struct crc_test_record *rec)
>> +{
>> +       u64 crc;
>> +
>> +       crc = crc64_bch(rec->data, sizeof(rec->data));
>> +       return chk_and_msg(rec->name, crc, rec->expval);
>> +}
>> +
>> +static int test_crc64_update(struct crc_test_record *rec)
>> +{
>> +       u64 crc = rec->initval;
>> +
>> +       crc = crc64_update(crc, rec->data, sizeof(rec->data));
>> +       return 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",
>> +         .data         = { 0x42F0E1EBA9EA3693, 0x85E1C3D753D46D26,
>> +                           0xC711223CFA3E5BB5, 0x493366450E42ECDF },
>> +         .initval      = 0,
>> +         .expval       = 0xe2b9911e7b997201,
>> +         .handler      = test_crc64,
>> +       },
>> +       { .name         = "crc64_bch",
>> +         .data         = { 0x42F0E1EBA9EA3693, 0x85E1C3D753D46D26,
>> +                           0xC711223CFA3E5BB5, 0x493366450E42ECDF },
>> +         .initval      = 0,
>> +         .expval       = 0xd2753a20fd862892,
>> +         .handler      = test_crc64_bch,
>> +       },
>> +       { .name         = "crc64_update",
>> +         .data         = { 0x42F0E1EBA9EA3693, 0x85E1C3D753D46D26,
>> +                           0xC711223CFA3E5BB5, 0x493366450E42ECDF },
>> +         .initval      = 0x61C8864680B583EB,
>> +         .expval       = 0xb2c863673f4292bf,
>> +         .handler      = test_crc64_update,
>> +       },
>> +       {}
>> +};
>> +
>> +static int __init test_crc_init(void)
>> +{
>> +       int i;
>> +       int v, err = 0;
>> +
>> +       pr_info("Kernel CRC consitency testing:\n");
>> +       for (i = 0; test_data[i].name; i++) {
>> +               v = test_data[i].handler(&test_data[i]);
>> +               if (v < 0)
>> +                       err++;
>> +       }
>> +
>> +       if (err == 0)
>> +               pr_info("test_crc: all %d tests passed\n", i);
> 
> Similar to previous comment: we should not report the number of passed
> tests, since adding a test would invalidate previous golden output.
> Also, consider the situation where some tests are conditionally
> executed depending on kconfig.

Sure, I fix this in v4 series. Thanks.

Coly Li

[snipped]

Coly Li July 18, 2018, 3:28 p.m. UTC | #7

On 2018/7/18 1:11 AM, Andy Shevchenko wrote:
> On Tue, Jul 17, 2018 at 5:55 PM, Coly Li <[e-mail]> 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: PASSED (0x4e6b1ff972fa8c55, expected 0x4e6b1ff972fa8c55)
>> test_crc: crc64_bch: PASSED (0x0e4f1391d7a4a62e, expected 0x0e4f1391d7a4a62e)
>> test_crc: crc64_update: 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 are only test caes for 3 crc routines,
>> - crc64()
>> - crc64_bch()
>> - crc64_update()
> 
> Thanks for an update. My comments below.
> 
>> Changelog:
>> v3: Add test cases passed/failed statistic
>>     More fixes for review comments of v2
>> v2: Fixes for review comments of v1
>> v1: Initial version.
> 
> Usually this part goes after --- line below.
> 

OK, I change all the patches in this way.

>> Signed-off-by: Coly Li <[e-mail]>
>> Reviewed-by: Hannes Reinecke <[e-mail]>
>> Cc: Greg Kroah-Hartman <[e-mail]>
>> Cc: Linus Torvalds <[e-mail]>
>> Cc: Thomas Gleixner <[e-mail]>
>> Cc: Kate Stewart <[e-mail]>
> 
> Please, Cc me as well this one in next version (use my Intel address).
> 

Added :-)

>> +#include <linux/async.h>
>> +#include <linux/delay.h>
>> +#include <linux/fs.h>
>> +#include <linux/list.h>
>> +#include <linux/module.h>
>> +#include <linux/printk.h>
>> +#include <linux/miscdevice.h>
>> +#include <linux/slab.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/vmalloc.h>
>> +#include <linux/crc64.h>
> 
> Do we need all of them?
> 

I remove most of them, only keep linux/module.h and linux/crc64.h in v4
series.

>> +static int chk_and_msg(const char *name, u64 crc, u64 expval)
>> +{
> 
>> +       int ret = 0;
>> +
>> +       if (crc == expval) {
> 
>> +               pr_info("test_crc: %s: PASSED:(0x%016llx, expected 0x%016llx)\n",
>> +                       name, crc, expval);
> 
> This doesn't bring anything useful.
> 
>> +       } else {
>> +               pr_err("test_crc: %s: FAILED:(0x%016llx, expected 0x%016llx)\n",
>> +                       name, crc, expval);
>> +               ret = -EINVAL;
>> +       }
>> +
>> +       return ret;
> 
> I would rewrite entire function as follows:
> 
> static void ...(...)
> {
>   total_tests++;
>   if (crc == expval)
>     return;
> 
>   pr_err(...);
>   failed_tests++;
> }
> 
> 
>> +}
> 
>> +static int __init test_crc_init(void)
>> +{
>> +       int i;
>> +       int v, err = 0;
>> +
>> +       pr_info("Kernel CRC consitency testing:\n");
> 
>> +       for (i = 0; test_data[i].name; i++) {
>> +               v = test_data[i].handler(&test_data[i]);
>> +               if (v < 0)
>> +                       err++;
>> +       }
> 
> ...and correct this to simple
> for (...)
>  test_data[i].handler(...);
> 
>> +       if (err == 0)
>> +               pr_info("test_crc: all %d tests passed\n", i);
>> +       else
>> +               pr_err("test_crc: %d cases tested, %d passed, %d failed\n",
>> +                      i, i - err, err);
> 
> ...and this accordingly.
> 
> Note, that in the future someone can add more test cases one or more
> of which could not map 1:1 to i here.
> That's why the rationale to have two global variables for test statistics.
> Also it allows (as you see above) to get rid of return code from all
> of those test. We don't interested in them I believe.

Yes, your code is simpler and more elegant IMHO. I change the code as
you suggested in v4 series.

Thanks.

Coly Li

Noah Massey July 18, 2018, 6:30 p.m. UTC | #8

On Tue, Jul 17, 2018 at 4:59 PM Andy Shevchenko
<[e-mail]> wrote:
> On Tue, Jul 17, 2018 at 9:51 PM, Noah Massey <[e-mail]> wrote:
> > On Tue, Jul 17, 2018 at 10:56 AM Coly Li <[e-mail]> wrote:
>
> >> +       if (err == 0)
> >> +               pr_info("test_crc: all %d tests passed\n", i);
> >
> > Similar to previous comment: we should not report the number of passed
> > tests, since adding a test would invalidate previous golden output.
> > Also, consider the situation where some tests are conditionally
> > executed depending on kconfig.
>
> We do similar in many test modules and I know at least two that had
> been changed in order to get new test cases.
> Are you proposing to change 'em all?
>

I was proposing that the message should be "test_crc: all tests
passed\n", since that would maintain a static expected output. Upon
further review, parsing minor variations in the messages is simple
enough so if the automated test tools already handle it keeping the
test count in the output is better.

Sorry for the noise,

~ Noah

Patch

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..441bf835fbd3
--- /dev/null
+++ b/lib/test_crc.c
@@ -0,0 +1,138 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CRC test driver
+ *
+ * Copyright (C) 2018 Coly Li <[e-mail]>
+ *
+ * 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()
+ * - crc64_bch()
+ * - crc64_update()
+ *
+ */
+
+#include <linux/async.h>
+#include <linux/delay.h>
+#include <linux/fs.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/printk.h>
+#include <linux/miscdevice.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/vmalloc.h>
+#include <linux/crc64.h>
+
+struct crc_test_record {
+	char	*name;
+	u64	data[4];
+	u64	initval;
+	u64	expval;
+	int	(*handler)(struct crc_test_record *rec);
+};
+
+static int chk_and_msg(const char *name, u64 crc, u64 expval)
+{
+	int ret = 0;
+
+	if (crc == expval) {
+		pr_info("test_crc: %s: PASSED:(0x%016llx, expected 0x%016llx)\n",
+			name, crc, expval);
+	} else {
+		pr_err("test_crc: %s: FAILED:(0x%016llx, expected 0x%016llx)\n",
+			name, crc, expval);
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+/* Add your crc test cases here */
+static int test_crc64(struct crc_test_record *rec)
+{
+	u64 crc;
+
+	crc = crc64(rec->data, sizeof(rec->data));
+	return chk_and_msg(rec->name, crc, rec->expval);
+}
+
+static int test_crc64_bch(struct crc_test_record *rec)
+{
+	u64 crc;
+
+	crc = crc64_bch(rec->data, sizeof(rec->data));
+	return chk_and_msg(rec->name, crc, rec->expval);
+}
+
+static int test_crc64_update(struct crc_test_record *rec)
+{
+	u64 crc = rec->initval;
+
+	crc = crc64_update(crc, rec->data, sizeof(rec->data));
+	return 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",
+	  .data		= { 0x42F0E1EBA9EA3693, 0x85E1C3D753D46D26,
+			    0xC711223CFA3E5BB5, 0x493366450E42ECDF },
+	  .initval	= 0,
+	  .expval	= 0xe2b9911e7b997201,
+	  .handler	= test_crc64,
+	},
+	{ .name		= "crc64_bch",
+	  .data		= { 0x42F0E1EBA9EA3693, 0x85E1C3D753D46D26,
+			    0xC711223CFA3E5BB5, 0x493366450E42ECDF },
+	  .initval	= 0,
+	  .expval	= 0xd2753a20fd862892,
+	  .handler	= test_crc64_bch,
+	},
+	{ .name		= "crc64_update",
+	  .data		= { 0x42F0E1EBA9EA3693, 0x85E1C3D753D46D26,
+			    0xC711223CFA3E5BB5, 0x493366450E42ECDF },
+	  .initval	= 0x61C8864680B583EB,
+	  .expval	= 0xb2c863673f4292bf,
+	  .handler	= test_crc64_update,
+	},
+	{}
+};
+
+static int __init test_crc_init(void)
+{
+	int i;
+	int v, err = 0;
+
+	pr_info("Kernel CRC consitency testing:\n");
+	for (i = 0; test_data[i].name; i++) {
+		v = test_data[i].handler(&test_data[i]);
+		if (v < 0)
+			err++;
+	}
+
+	if (err == 0)
+		pr_info("test_crc: all %d tests passed\n", i);
+	else
+		pr_err("test_crc: %d cases tested, %d passed, %d failed\n",
+		       i, i - err, err);
+
+	return (err == 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 <[e-mail]>");
+MODULE_LICENSE("GPL v2");