linux-kernel - Re: [PATCH v7 2/2] lib: checksum: Use aligned accesses for ip_fast_csum and csum_ipv6

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]

Date: Mon, 12 Feb 2024 22:57:47 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Charlie Jenkins <charlie@...osinc.com>
Cc: David Laight <David.Laight@...lab.com>,
 Palmer Dabbelt <palmer@...belt.com>,
 Andrew Morton <akpm@...ux-foundation.org>, Al Viro
 <viro@...iv.linux.org.uk>, linux-kernel@...r.kernel.org,
 Helge Deller <deller@....de>,
 "James E.J. Bottomley" <James.Bottomley@...senPartnership.com>,
 Parisc List <linux-parisc@...r.kernel.org>
Subject: Re: [PATCH v7 2/2] lib: checksum: Use aligned accesses for
 ip_fast_csum and csum_ipv6_magic tests [issues with parisc64]

On 2/12/24 16:32, Charlie Jenkins wrote:
> On Mon, Feb 12, 2024 at 04:14:49PM -0800, Guenter Roeck wrote:
>> On 2/12/24 12:33, Charlie Jenkins wrote:
>>> The test cases for ip_fast_csum and csum_ipv6_magic were failing on a
>>> variety of architectures that are big endian or do not support
>>> misalgined accesses. Both of these test cases are changed to support big
>>> and little endian architectures.
>>>
>>> The test for ip_fast_csum is changed to align the data along (14 +
>>> NET_IP_ALIGN) bytes which is the alignment of an IP header. The test for
>>> csum_ipv6_magic aligns the data using a struct. An extra padding field
>>> is added to the struct to ensure that the size of the struct is the same
>>> on all architectures (44 bytes).
>>>
>>> Fixes: 6f4c45cbcb00 ("kunit: Add tests for csum_ipv6_magic and ip_fast_csum")
>>> Signed-off-by: Charlie Jenkins <charlie@...osinc.com>
>>
>> This thing really wants to annoy me. Now I get:
>>
>>       # test_csum_ipv6_magic: ASSERTION FAILED at lib/checksum_kunit.c:494
>>       Expected ( u64)csum_result == ( u64)expected, but
>>           ( u64)csum_result == 46543 (0xb5cf)
>>           ( u64)expected == 46544 (0xb5d0)
>>       not ok 5 test_csum_ipv6_magic
>>
>> with the parisc64 tests. All other architectures / platforms work fine
>> after applying the various pending fixes. It looks like a carry gets
>> lost somewhere, but I have not been able to figure out where exactly
>> that happens. This only happens with the 64-bit hppa assembler code.
>>

It turns out that hppa64 triggers an unaligned instruction trap if an
access is unaligned (where unaligned means not aligned to 64 bit).
It appears that this trap either wrongly drops or adds a carry flag
under some circumstances, and that this is causing the problem to be seen.
I can confirm this with:

diff --git a/arch/parisc/include/asm/checksum.h b/arch/parisc/include/asm/checksum.h
index e619e67440db..9bddf3231a28 100644
--- a/arch/parisc/include/asm/checksum.h
+++ b/arch/parisc/include/asm/checksum.h
@@ -128,9 +128,9 @@ static __inline__ __sum16 csum_ipv6_magic(const struct in6_addr *saddr,

  "      ldd,ma          8(%1), %4\n"    /* get 1st saddr word */
  "      ldd,ma          8(%2), %5\n"    /* get 1st daddr word */
-"      add             %4, %0, %0\n"
  "      ldd,ma          8(%1), %6\n"    /* 2nd saddr */
  "      ldd,ma          8(%2), %7\n"    /* 2nd daddr */
+"      add             %4, %0, %0\n"
  "      add,dc          %5, %0, %0\n"
  "      add,dc          %6, %0, %0\n"
  "      add,dc          %7, %0, %0\n"

or, simpler,

diff --git a/arch/parisc/include/asm/checksum.h b/arch/parisc/include/asm/checksum.h
index e619e67440db..7e2bb797dfe0 100644
--- a/arch/parisc/include/asm/checksum.h
+++ b/arch/parisc/include/asm/checksum.h
@@ -131,7 +131,7 @@ static __inline__ __sum16 csum_ipv6_magic(const struct in6_addr *saddr,
  "      add             %4, %0, %0\n"
  "      ldd,ma          8(%1), %6\n"    /* 2nd saddr */
  "      ldd,ma          8(%2), %7\n"    /* 2nd daddr */
-"      add,dc          %5, %0, %0\n"
+"      add             %5, %0, %0\n"
  "      add,dc          %6, %0, %0\n"
  "      add,dc          %7, %0, %0\n"
  "      add,dc          %3, %0, %0\n"  /* fold in proto+len | carry bit */

both of which make the problem disappear. Unless I am missing something, this
means that the unaligned access trap adds an additional carry flag.

Question now is if this is a bug in the qemu emulation or a bug in the unaligned
trap handler. FWIW, both changes above should be possible workarounds since the
first add operation should never overflow (it adds two 32 bit numbers into a 64
bit register). I'd rather fix the root cause, though.

Any thoughts ? I am adding the parisc maintainers and its e-mail list for
additional feedback.

Thanks,
Guenter