| 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
| ||
|
Message-ID: <8eed6488-87c0-49a9-b030-d22b4a698356@csgroup.eu>
Date: Sat, 24 Feb 2024 07:40:41 +0000
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Charlie Jenkins <charlie@...osinc.com>
CC: "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
Andrew Morton <akpm@...ux-foundation.org>, Guenter Roeck
<linux@...ck-us.net>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "netdev@...r.kernel.org"
<netdev@...r.kernel.org>, Palmer Dabbelt <palmer@...belt.com>, Erhard Furtner
<erhard_f@...lbox.org>
Subject: Re: [PATCH net] kunit: Fix again checksum tests on big endian CPUs
Le 23/02/2024 à 21:05, Charlie Jenkins a écrit :
> On Fri, Feb 23, 2024 at 06:15:16PM +0000, Christophe Leroy wrote:
>>
>>
>> Le 23/02/2024 à 18:57, Charlie Jenkins a écrit :
>>> On Fri, Feb 23, 2024 at 11:41:52AM +0100, Christophe Leroy wrote:
>>>> Commit b38460bc463c ("kunit: Fix checksum tests on big endian CPUs")
>>>> fixed endianness issues with kunit checksum tests, but then
>>>> commit 6f4c45cbcb00 ("kunit: Add tests for csum_ipv6_magic and
>>>> ip_fast_csum") introduced new issues on big endian CPUs. Those issues
>>>> are once again reflected by the warnings reported by sparse.
>>>>
>>>> So, fix them with the same approach, perform proper conversion in
>>>> order to support both little and big endian CPUs. Once the conversions
>>>> are properly done and the right types used, the sparse warnings are
>>>> cleared as well.
>>>>
>>>> Reported-by: Erhard Furtner <erhard_f@...lbox.org>
>>>> Fixes: 6f4c45cbcb00 ("kunit: Add tests for csum_ipv6_magic and ip_fast_csum")
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@...roup.eu>
>>>> ---
>>>> lib/checksum_kunit.c | 17 +++++++++--------
>>>> 1 file changed, 9 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/lib/checksum_kunit.c b/lib/checksum_kunit.c
>>>> index 225bb7701460..bf70850035c7 100644
>>>> --- a/lib/checksum_kunit.c
>>>> +++ b/lib/checksum_kunit.c
>>>> @@ -215,7 +215,7 @@ static const u32 init_sums_no_overflow[] = {
>>>> 0xffff0000, 0xfffffffb,
>>>> };
>>>>
>>>> -static const __sum16 expected_csum_ipv6_magic[] = {
>>>> +static const u16 expected_csum_ipv6_magic[] = {
>>>> 0x18d4, 0x3085, 0x2e4b, 0xd9f4, 0xbdc8, 0x78f, 0x1034, 0x8422, 0x6fc0,
>>>> 0xd2f6, 0xbeb5, 0x9d3, 0x7e2a, 0x312e, 0x778e, 0xc1bb, 0x7cf2, 0x9d1e,
>>>> 0xca21, 0xf3ff, 0x7569, 0xb02e, 0xca86, 0x7e76, 0x4539, 0x45e3, 0xf28d,
>>>> @@ -241,7 +241,7 @@ static const __sum16 expected_csum_ipv6_magic[] = {
>>>> 0x3845, 0x1014
>>>> };
>>>>
>>>> -static const __sum16 expected_fast_csum[] = {
>>>> +static const u16 expected_fast_csum[] = {
>>>> 0xda83, 0x45da, 0x4f46, 0x4e4f, 0x34e, 0xe902, 0xa5e9, 0x87a5, 0x7187,
>>>> 0x5671, 0xf556, 0x6df5, 0x816d, 0x8f81, 0xbb8f, 0xfbba, 0x5afb, 0xbe5a,
>>>> 0xedbe, 0xabee, 0x6aac, 0xe6b, 0xea0d, 0x67ea, 0x7e68, 0x8a7e, 0x6f8a,
>>>> @@ -577,7 +577,8 @@ static void test_csum_no_carry_inputs(struct kunit *test)
>>>>
>>>> static void test_ip_fast_csum(struct kunit *test)
>>>> {
>>>> - __sum16 csum_result, expected;
>>>> + __sum16 csum_result;
>>>> + u16 expected;
>>>>
>>>> for (int len = IPv4_MIN_WORDS; len < IPv4_MAX_WORDS; len++) {
>>>> for (int index = 0; index < NUM_IP_FAST_CSUM_TESTS; index++) {
>>>> @@ -586,7 +587,7 @@ static void test_ip_fast_csum(struct kunit *test)
>>>> expected_fast_csum[(len - IPv4_MIN_WORDS) *
>>>> NUM_IP_FAST_CSUM_TESTS +
>>>> index];
>>>> - CHECK_EQ(expected, csum_result);
>>>> + CHECK_EQ(to_sum16(expected), csum_result);
>>>> }
>>>> }
>>>> }
>>>> @@ -598,7 +599,7 @@ static void test_csum_ipv6_magic(struct kunit *test)
>>>> const struct in6_addr *daddr;
>>>> unsigned int len;
>>>> unsigned char proto;
>>>> - unsigned int csum;
>>>> + __wsum csum;
>>>>
>>>> const int daddr_offset = sizeof(struct in6_addr);
>>>> const int len_offset = sizeof(struct in6_addr) + sizeof(struct in6_addr);
>>>> @@ -611,10 +612,10 @@ static void test_csum_ipv6_magic(struct kunit *test)
>>>> saddr = (const struct in6_addr *)(random_buf + i);
>>>> daddr = (const struct in6_addr *)(random_buf + i +
>>>> daddr_offset);
>>>> - len = *(unsigned int *)(random_buf + i + len_offset);
>>>> + len = le32_to_cpu(*(__le32 *)(random_buf + i + len_offset));
>>>> proto = *(random_buf + i + proto_offset);
>>>> - csum = *(unsigned int *)(random_buf + i + csum_offset);
>>>> - CHECK_EQ(expected_csum_ipv6_magic[i],
>>>> + csum = *(__wsum *)(random_buf + i + csum_offset);
>>>> + CHECK_EQ(to_sum16(expected_csum_ipv6_magic[i]),
>>>> csum_ipv6_magic(saddr, daddr, len, proto, csum));
>>>> }
>>>> #endif /* !CONFIG_NET */
>>>> --
>>>> 2.43.0
>>>>
>>>
>>> There is no need to duplicate efforts here. This has already been
>>> resolved by
>>> https://lore.kernel.org/lkml/20240221-fix_sparse_errors_checksum_tests-v9-2-bff4d73ab9d1@rivosinc.com/.
>>>
>>
>> The idea here is to provide a fix which is similar to the one done
>> previously and that uses the same approach and reuses the same helpers.
>>
>> This is to keep the code homogeneous.
>>
>> Christophe
>
> htons makes more sense here since this is networking code, but I don't
> care enough to argue the point. I tested it on big endian SPARC and on
> riscv. I'll base my alignment patch on this.
I agree with you that htons() would have made more sense, but the
expected values are in little endian and when I fixed the issue the
first time I wanted to keep it as a minimal fix and didn't want to churn
things too much and change all expected values.
If we go for htons() this time this means again changing expected values
to natural network byte order (big endian). Either we do it only for the
tests to be fixed now, and we'll have some tests for with the results
are in big endian and some that remains in little endian. That looks
inconsistant to me. Or we do the change for all tests. In both case it
would be a bulky change. As we are relatively late in 6.8 cycle my
preference goes for a minimal fix.
Why not then convert _all_ tests to network byte order expectation in a
follow-up patch.
>
> Tested-by: Charlie Jenkins <charlie@...osinc.com>
>
Thanks
Christophe
Powered by blists - more mailing lists