linux-kernel - Re: [PATCH v6 03/10] crc32-replace-self-test.diff

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: <20110902165138.927100ce.akpm@linux-foundation.org>
Date:	Fri, 2 Sep 2011 16:51:38 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Bob Pearson <rpearson@...temfabricworks.com>
Cc:	linux-kernel@...r.kernel.org, fzago@...temfabricworks.com,
	Joakim Tjernlund <joakim.tjernlund@...nsmode.se>,
	George Spelvin <linux@...izon.com>
Subject: Re: [PATCH v6 03/10] crc32-replace-self-test.diff

On Wed, 31 Aug 2011 17:29:58 -0500
Bob Pearson <rpearson@...temfabricworks.com> wrote:

> Replaced the unit test provided in crc32.c, which doesn't have a
> makefile and doesn't compile with current headers, with a simpler
> self test routine that also gives a measure of performance and
> runs at module init time. The self test option can be enabled
> through a configuration option CONFIG_CRC32_SELFTEST.
> 
> The test stresses the pre and post loops and is thus not very
> realistic since actual uses will likely have addresses and lengths
> that are at least 4 byte aligned. However, the main loop is long
> enough so that the performance is dominated by that loop.
> 
> The expected values for crc32_le and crc32_be were generated
> with the original version of crc32.c using CRC_BITS_LE = 8 and
> CRC_BITS_BE = 8. These values were then used to check all the
> values of the BITS parameters in both the original and new versions.
> 
> The performance results show some variability from run to run
> in spite of attempts to both warm the cache and reduce the amount
> of OS noise by limiting interrutps during the test. To get comparable
> results and to analyse options wrt performance the best time
> reported over a small sample of runs has been taken.
> 

I don't object to a self-test which actually works, but it seems pretty
lame that the self-test exists in kernel mode when it is so simple to
prepare a much more useful and powerful correctness/performance test
harness in userspace.

> ...
>
> -static u32 test_step(u32 init, unsigned char *buf, size_t len)
> -{
> -	u32 crc1, crc2;
> -	size_t i;
> +		crc ^= crc32_be(test[i].crc, test_buf +
> +		    test[i].start, test[i].length);
> +	}
>  
> -	crc1 = crc32_be(init, buf, len);
> -	store_be(crc1, buf + len);
> -	crc2 = crc32_be(init, buf, len + 4);
> -	if (crc2)
> -		printf("\nCRC cancellation fail: 0x%08x should be 0\n",
> -		       crc2);
> -
> -	for (i = 0; i <= len + 4; i++) {
> -		crc2 = crc32_be(init, buf, i);
> -		crc2 = crc32_be(crc2, buf + i, len + 4 - i);
> -		if (crc2)
> -			printf("\nCRC split fail: 0x%08x\n", crc2);
> +	/* reduce OS noise */

This comment is useless.

> +	local_irq_save(flags);
> +	local_irq_disable();

local_irq_save() already does local_irq_disable().

local_irq_disable() doesn't protect against actions of other CPUs.  I'd
know if this was a bug if the comment wasn't useless :)

> +	getnstimeofday(&start);
> +	for (i = 0; i < 100; i++) {
> +		if (test[i].crc_le != crc32_le(test[i].crc, test_buf +
> +		    test[i].start, test[i].length))
> +			errors++;
> +
> +		if (test[i].crc_be != crc32_be(test[i].crc, test_buf +
> +		    test[i].start, test[i].length))
> +			errors++;
>  	}
> ...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/