| 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: <e548f697-650e-4333-9f39-19a472b7d90a@roeck-us.net>
Date: Mon, 22 Jan 2024 17:06:09 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Charlie Jenkins <charlie@...osinc.com>,
Palmer Dabbelt <palmer@...belt.com>
Cc: David.Laight@...lab.com, Conor Dooley <conor@...nel.org>,
samuel.holland@...ive.com, xiao.w.wang@...el.com,
Evan Green <evan@...osinc.com>, guoren@...nel.org,
linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-arch@...r.kernel.org, Paul Walmsley <paul.walmsley@...ive.com>,
aou@...s.berkeley.edu, Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH v15 5/5] kunit: Add tests for csum_ipv6_magic and
ip_fast_csum
On 1/22/24 15:55, Charlie Jenkins wrote:
> On Mon, Jan 22, 2024 at 03:39:16PM -0800, Palmer Dabbelt wrote:
>> On Mon, 22 Jan 2024 13:41:48 PST (-0800), David.Laight@...LAB.COM wrote:
>>> From: Guenter Roeck
>>>> Sent: 22 January 2024 17:16
>>>>
>>>> On 1/22/24 08:52, David Laight wrote:
>>>>> From: Guenter Roeck
>>>>>> Sent: 22 January 2024 16:40
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On Mon, Jan 08, 2024 at 03:57:06PM -0800, Charlie Jenkins wrote:
>>>>>>> Supplement existing checksum tests with tests for csum_ipv6_magic and
>>>>>>> ip_fast_csum.
>>>>>>>
>>>>>>> Signed-off-by: Charlie Jenkins <charlie@...osinc.com>
>>>>>>> ---
>>>>>>
>>>>>> With this patch in the tree, the arm:mps2-an385 qemu emulation gets a bad hiccup.
>>>>>>
>>>>>> [ 1.839556] Unhandled exception: IPSR = 00000006 LR = fffffff1
>>>>>> [ 1.839804] CPU: 0 PID: 164 Comm: kunit_try_catch Tainted: G N 6.8.0-rc1 #1
>>>>>> [ 1.839948] Hardware name: Generic DT based system
>>>>>> [ 1.840062] PC is at __csum_ipv6_magic+0x8/0xb4
>>>>>> [ 1.840408] LR is at test_csum_ipv6_magic+0x3d/0xa4
>>>>>> [ 1.840493] pc : [<21212f34>] lr : [<21117fd5>] psr: 0100020b
>>>>>> [ 1.840586] sp : 2180bebc ip : 46c7f0d2 fp : 21275b38
>>>>>> [ 1.840664] r10: 21276b60 r9 : 21275b28 r8 : 21465cfc
>>>>>> [ 1.840751] r7 : 00003085 r6 : 21275b4e r5 : 2138702c r4 : 00000001
>>>>>> [ 1.840847] r3 : 2c000000 r2 : 1ac7f0d2 r1 : 21275b39 r0 : 21275b29
>>>>>> [ 1.840942] xPSR: 0100020b
>>>>>>
>>>>>> This translates to:
>>>>>>
>>>>>> PC is at __csum_ipv6_magic (arch/arm/lib/csumipv6.S:15)
>>>>>> LR is at test_csum_ipv6_magic (./arch/arm/include/asm/checksum.h:60
>>>>>> ./arch/arm/include/asm/checksum.h:163 lib/checksum_kunit.c:617)
>>>>>>
>>>>>> Obviously I can not say if this is a problem with qemu or a problem with
>>>>>> the Linux kernel. Given that, and the presumably low interest in
>>>>>> running mps2-an385 with Linux, I'll simply disable that test. Just take
>>>>>> it as a heads up that there _may_ be a problem with this on arm
>>>>>> nommu systems.
>>>>>
>>>>> Can you drop in a disassembly of __csum_ipv6_magic ?
>>>>> Actually I think it is:
>>>>
>>>> It is, as per the PC pointer above. I don't know anything about arm assembler,
>>>> much less about its behavior with THUMB code.
>>>
>>> Doesn't look like thumb to me (offset 8 is two 4-byte instructions) and
>>> the code I found looks like arm to me.
>>> (I haven't written any arm asm since before they invented thumb!)
>>>
>>>>> ENTRY(__csum_ipv6_magic)
>>>>> str lr, [sp, #-4]!
>>>>> adds ip, r2, r3
>>>>> ldmia r1, {r1 - r3, lr}
>>>>>
>>>>> So the fault is (probably) a misaligned ldmia ?
>>>>> Are they ever supported?
>>>>>
>>>>
>>>> Good question. My primary guess is that this never worked. As I said,
>>>> this was just intended to be informational, (probably) no reason to bother.
>>>>
>>>> Of course one might ask if it makes sense to even keep the arm nommu code
>>>> in the kernel, but that is of course a different question. I do wonder though
>>>> if anyone but me is running it.
>>>
>>> If it is an alignment fault it isn't a 'nommu' bug.
>>>
>>> And traditionally arm didn't support misaligned transfers (well not
>>> in anyway any other cpu did!).
>>> It might be that the kernel assumes that all ethernet packets are
>>> aligned, but the test suite isn't aligning the buffer.
>>> Which would make it a test suite bug.
>>
>> From talking to Evan and Vineet, I think you're right and this is a test
>> suite bug: specifically the tests weren't respecting NET_IP_ALIGN. That
>> didn't crop up for ip_fast_csum() as it just uses ldr which supports
>> misaligned accesses on the M3 (at least as far as I can tell).
>>
>> So I think the right fix is something like
>>
>> diff --git a/lib/checksum_kunit.c b/lib/checksum_kunit.c
>> index 225bb7701460..2dd282e27dd4 100644
>> --- a/lib/checksum_kunit.c
>> +++ b/lib/checksum_kunit.c
>> @@ -5,6 +5,7 @@
>> #include <kunit/test.h>
>> #include <asm/checksum.h>
>> +#include <asm/checksum.h>
>> #include <net/ip6_checksum.h>
>> #define MAX_LEN 512
>> @@ -15,6 +16,7 @@
>> #define IPv4_MAX_WORDS 15
>> #define NUM_IPv6_TESTS 200
>> #define NUM_IP_FAST_CSUM_TESTS 181
>> +#define SUPPORTED_ALIGNMENT (1 << NET_IP_ALIGN)
>> /* Values for a little endian CPU. Byte swap each half on big endian CPU. */
>> static const u32 random_init_sum = 0x2847aab;
>> @@ -486,7 +488,7 @@ static void test_csum_fixed_random_inputs(struct kunit *test)
>> __sum16 result, expec;
>> assert_setup_correct(test);
>> - for (align = 0; align < TEST_BUFLEN; ++align) {
>> + for (align = 0; align < TEST_BUFLEN; align += SUPPORTED_ALIGNMENT) {
>> memcpy(&tmp_buf[align], random_buf,
>> min(MAX_LEN, TEST_BUFLEN - align));
>> for (len = 0; len < MAX_LEN && (align + len) < TEST_BUFLEN;
>> @@ -513,7 +515,7 @@ static void test_csum_all_carry_inputs(struct kunit *test)
>> assert_setup_correct(test);
>> memset(tmp_buf, 0xff, TEST_BUFLEN);
>> - for (align = 0; align < TEST_BUFLEN; ++align) {
>> + for (align = 0; align < TEST_BUFLEN; align += SUPPORTED_ALIGNMENT) {
>> for (len = 0; len < MAX_LEN && (align + len) < TEST_BUFLEN;
>> ++len) {
>> /*
>> @@ -553,7 +555,7 @@ static void test_csum_no_carry_inputs(struct kunit *test)
>> assert_setup_correct(test);
>> memset(tmp_buf, 0x4, TEST_BUFLEN);
>> - for (align = 0; align < TEST_BUFLEN; ++align) {
>> + for (align = 0; align < TEST_BUFLEN; align += SUPPORTED_ALIGNMENT) {
>> for (len = 0; len < MAX_LEN && (align + len) < TEST_BUFLEN;
>> ++len) {
>> /*
>>
>> but I haven't even build tested it...
>
> This doesn't fix the test_csum_ipv6_magic test case that was causing the
> initial problem, but the same trick can be done in that test.
>
The above didn't (and still doesn't) fail for me. The following fixes the problem.
So, yes, I guess the problem has to do with alignment. I don't know if NET_IP_ALIGN
would do the trick, though - it works, but it seems to me that the definition of
NET_IP_ALIGN is supposed to address potential performance issues, not mandatory
IP header alignment.
Thanks,
Guenter
---
diff --git a/lib/checksum_kunit.c b/lib/checksum_kunit.c
index 225bb7701460..c8730af2a474 100644
--- a/lib/checksum_kunit.c
+++ b/lib/checksum_kunit.c
@@ -591,6 +591,8 @@ static void test_ip_fast_csum(struct kunit *test)
}
}
+#define SUPPORTED_ALIGNMENT (1 << NET_IP_ALIGN)
+
static void test_csum_ipv6_magic(struct kunit *test)
{
#if defined(CONFIG_NET)
@@ -607,7 +609,7 @@ static void test_csum_ipv6_magic(struct kunit *test)
const int csum_offset = sizeof(struct in6_addr) + sizeof(struct in6_addr) +
sizeof(int) + sizeof(char);
- for (int i = 0; i < NUM_IPv6_TESTS; i++) {
+ for (int i = 0; i < NUM_IPv6_TESTS; i+=SUPPORTED_ALIGNMENT) {
saddr = (const struct in6_addr *)(random_buf + i);
daddr = (const struct in6_addr *)(random_buf + i +
daddr_offset);
Powered by blists - more mailing lists