linux-kernel - Re: [PATCH v3 3/3] lib/test_crc: Add test cases for crc calculation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VcXDbDTebddnepeari8_ruynPudbtHGybFEQ=3Cz8Ddvg@mail.gmail.com>
Date:   Tue, 17 Jul 2018 20:11:49 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Coly Li <colyli@...e.de>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-bcache@...r.kernel.org, linux-block@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Kate Stewart <kstewart@...uxfoundation.org>
Subject: Re: [PATCH v3 3/3] lib/test_crc: Add test cases for crc calculation

On Tue, Jul 17, 2018 at 5:55 PM, Coly Li <colyli@...e.de> 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 <colyli@...e.de>
> Reviewed-by: Hannes Reinecke <hare@...e.de>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: Linus Torvalds <torvalds@...ux-foundation.org>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Kate Stewart <kstewart@...uxfoundation.org>

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;
> +}

-- 
With Best Regards,
Andy Shevchenko