linux-kernel - Re: [PATCH 3/3] lib: Update the muldiv64 tests to verify the C on x86-64

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: <62qp434q-q2ps-r698-qs2n-43345rn4npn0@onlyvoer.pbz>
Date: Sat, 5 Apr 2025 22:26:30 -0400 (EDT)
From: Nicolas Pitre <npitre@...libre.com>
To: David Laight <david.laight.linux@...il.com>
cc: Andrew Morton <akpm@...ux-foundation.org>, linux-kernel@...r.kernel.org, 
    Uwe Kleine-König <u.kleine-koenig@...libre.com>, 
    Oleg Nesterov <oleg@...hat.com>, Peter Zijlstra <peterz@...radead.org>, 
    Biju Das <biju.das.jz@...renesas.com>
Subject: Re: [PATCH 3/3] lib: Update the muldiv64 tests to verify the C on
 x86-64

On Sat, 5 Apr 2025, David Laight wrote:

> div64.c contains a 128 by 64 division algorithm which x86-64 overrides
> it with an asm implementation.
> So running the muldiv64 tests only verifies the asm code.
> Since x86-64 is the most likely test system compile the default
> code into an x86-64 kernel (under a different name) when the tests
> are being built.
> Verify that both the asm and C functions generate the correct results.
> 
> Signed-off-by: David Laight <david.laight.linux@...il.com>
> ---
>  lib/math/div64.c                    | 18 ++++++++++++++++--
>  lib/math/test_mul_u64_u64_div_u64.c | 26 ++++++++++++++++++++------
>  2 files changed, 36 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/math/div64.c b/lib/math/div64.c
> index 50e025174495..38ee5c01c288 100644
> --- a/lib/math/div64.c
> +++ b/lib/math/div64.c
> @@ -25,6 +25,8 @@
>  #include <linux/minmax.h>
>  #include <linux/log2.h>
>  
> +#include <generated/autoconf.h>

Isn't this automatically included everywhere by the Makefile?

> @@ -183,10 +185,22 @@ u32 iter_div_u64_rem(u64 dividend, u32 divisor, u64 *remainder)
>  }
>  EXPORT_SYMBOL(iter_div_u64_rem);
>  
> -#if !defined(mul_u64_add_u64_div_u64)
> +/*
> + * If the architecture overrides the implementation below and the test module
> + * is being built then compile the default implementation with a different name
> + * so that it can be tested.
> + */
> +#if defined(mul_u64_add_u64_div_u64) && (defined(CONFIG_TEST_MULDIV64) || defined(CONFIG_TEST_MULDIV64_MODULE))

You could shorten this to:

#if defined(mul_u64_add_u64_div_u64) && IS_ENABLED(CONFIG_TEST_MULDIV64)

> +#define TEST_MULDIV64

Then I'd use IS_ENABLED(CONFIG_TEST_MULDIV64) in place of TEST_MULDIV64.
It is more self explanatory.

> +#undef mul_u64_add_u64_div_u64
> +#define mul_u64_add_u64_div_u64 mul_u64_add_u64_div_u64_test
> +u64 mul_u64_add_u64_div_u64_test(u64 a, u64 b, u64 c, u64 d);
> +#endif

Hmmm... I wish there could be a better way to do this, but other than 
the above suggestion I don't see one.

> +
> +#if !defined( mul_u64_add_u64_div_u64) || defined(TEST_MULDIV64)
>  u64 mul_u64_add_u64_div_u64(u64 a, u64 b, u64 c, u64 d)
>  {
> -#if defined(__SIZEOF_INT128__)
> +#if defined(__SIZEOF_INT128__) && !defined(TEST_MULDIV64)
>  
>  	/* native 64x64=128 bits multiplication */
>  	u128 prod = (u128)a * b + c;
> diff --git a/lib/math/test_mul_u64_u64_div_u64.c b/lib/math/test_mul_u64_u64_div_u64.c
> index 9548eb7458c7..e2289b412601 100644
> --- a/lib/math/test_mul_u64_u64_div_u64.c
> +++ b/lib/math/test_mul_u64_u64_div_u64.c
> @@ -73,6 +73,10 @@ done
>  
>   */
>  
> +#ifdef mul_u64_add_u64_div_u64
> +u64 mul_u64_add_u64_div_u64_test(u64 a, u64 b, u64 add, u64 c);
> +#endif
> +
>  static int __init test_init(void)
>  {
>  	int errors = 0;
> @@ -80,21 +84,31 @@ static int __init test_init(void)
>  
>  	pr_info("Starting mul_u64_u64_div_u64() test\n");
>  
> -	for (i = 0; i < ARRAY_SIZE(test_values); i++) {
> -		u64 a = test_values[i].a;
> -		u64 b = test_values[i].b;
> -		u64 c = test_values[i].c;
> -		u64 expected_result = test_values[i].result;
> +	for (i = 0; i < ARRAY_SIZE(test_values) * 2; i++) {
> +		u64 a = test_values[i / 2].a;
> +		u64 b = test_values[i / 2].b;
> +		u64 c = test_values[i / 2].c;
> +		u64 expected_result = test_values[i / 2].result;

I don't see the point of the loop doubling here.
If I understand it correctly, you'll test the default version twice and 
the _test version once for each test entry.


>  		u64 result = mul_u64_u64_div_u64(a, b, c);
>  		u64 result_up = mul_u64_u64_div_u64_roundup(a, b, c);
>  
> +#ifdef mul_u64_add_u64_div_u64
> +		if (i & 1) {
> +			/* Verify the generic C version */
> +			result = mul_u64_add_u64_div_u64_test(a, b, 0, c);
> +			result_up = mul_u64_add_u64_div_u64_test(a, b, c - 1, c);
> +		}
> +#else
> +		i++;
> +#endif
> +
>  		if (result != expected_result) {
>  			pr_err("ERROR: 0x%016llx * 0x%016llx / 0x%016llx\n", a, b, c);
>  			pr_err("ERROR: expected result: %016llx\n", expected_result);
>  			pr_err("ERROR: obtained result: %016llx\n", result);
>  			errors++;
>  		}
> -		expected_result += test_values[i].round_up;
> +		expected_result += test_values[i / 2].round_up;
>  		if (result_up != expected_result) {
>  			pr_err("ERROR: 0x%016llx * 0x%016llx +/ 0x%016llx\n", a, b, c);
>  			pr_err("ERROR: expected result: %016llx\n", expected_result);
> -- 
> 2.39.5
> 
>