| Message ID | 20240210175526.3710522-1-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | parisc: Fix ip_fast_csum | expand |
On Sat, Feb 10, 2024 at 09:55:26AM -0800, Guenter Roeck wrote: > IP checksum unit tests report the following error when run on hppa/hppa64. > > # test_ip_fast_csum: ASSERTION FAILED at lib/checksum_kunit.c:463 > Expected ( u64)csum_result == ( u64)expected, but > ( u64)csum_result == 33754 (0x83da) > ( u64)expected == 10946 (0x2ac2) > not ok 4 test_ip_fast_csum > > 0x83da is the expected result if the IP header length is 20 bytes. 0x2ac2 > is the expected result if the IP header length is 24 bytes. The test fails > with an IP header length of 24 bytes. It appears that ip_fast_csum() > always returns the checksum for a 20-byte header, no matter how long > the header actually is. > > Code analysis shows a suspicious assembler sequence in ip_fast_csum(). > > " addc %0, %3, %0\n" > "1: ldws,ma 4(%1), %3\n" > " addib,< 0, %2, 1b\n" <--- > > While my understanding of HPPA assembler is limited, it does not seem > to make much sense to subtract 0 from a register and to expect the result > to ever be negative. Subtracting 1 from the length parameter makes more > sense. On top of that, the operation should be repeated if and only if > the result is still > 0, so change the suspicious instruction to > " addib,> -1, %2, 1b\n" > > The IP checksum unit test passes after this change. > > Cc: Charlie Jenkins <charlie@rivosinc.com> > Cc: Palmer Dabbelt <palmer@rivosinc.com> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > Prerequisite for this patch is > https://lore.kernel.org/lkml/20240207-fix_sparse_errors_checksum_tests-v6-0-4caa9629705b@rivosinc.com/ > > No idea how that was not detected before. Maybe I am missing something. > > Note that test_csum_ipv6_magic still fails on 32-bit parisc systems > after this patch has been applied. That is a different problem. > > arch/parisc/include/asm/checksum.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/parisc/include/asm/checksum.h b/arch/parisc/include/asm/checksum.h > index 3c43baca7b39..f705e5dd1074 100644 > --- a/arch/parisc/include/asm/checksum.h > +++ b/arch/parisc/include/asm/checksum.h > @@ -40,7 +40,7 @@ static inline __sum16 ip_fast_csum(const void *iph, unsigned int ihl) > " addc %0, %5, %0\n" > " addc %0, %3, %0\n" > "1: ldws,ma 4(%1), %3\n" > -" addib,< 0, %2, 1b\n" > +" addib,> -1, %2, 1b\n" > " addc %0, %3, %0\n" > "\n" > " extru %0, 31, 16, %4\n" > -- > 2.39.2 > I got my parisc setup working and this does indeed fix the issue. Tested-by: Charlie Jenkins <charlie@rivosinc.com> Reviewed-by: Charlie Jenkins <charlie@rivosinc.com>
diff --git a/arch/parisc/include/asm/checksum.h b/arch/parisc/include/asm/checksum.h index 3c43baca7b39..f705e5dd1074 100644 --- a/arch/parisc/include/asm/checksum.h +++ b/arch/parisc/include/asm/checksum.h @@ -40,7 +40,7 @@ static inline __sum16 ip_fast_csum(const void *iph, unsigned int ihl) " addc %0, %5, %0\n" " addc %0, %3, %0\n" "1: ldws,ma 4(%1), %3\n" -" addib,< 0, %2, 1b\n" +" addib,> -1, %2, 1b\n" " addc %0, %3, %0\n" "\n" " extru %0, 31, 16, %4\n"
IP checksum unit tests report the following error when run on hppa/hppa64. # test_ip_fast_csum: ASSERTION FAILED at lib/checksum_kunit.c:463 Expected ( u64)csum_result == ( u64)expected, but ( u64)csum_result == 33754 (0x83da) ( u64)expected == 10946 (0x2ac2) not ok 4 test_ip_fast_csum 0x83da is the expected result if the IP header length is 20 bytes. 0x2ac2 is the expected result if the IP header length is 24 bytes. The test fails with an IP header length of 24 bytes. It appears that ip_fast_csum() always returns the checksum for a 20-byte header, no matter how long the header actually is. Code analysis shows a suspicious assembler sequence in ip_fast_csum(). " addc %0, %3, %0\n" "1: ldws,ma 4(%1), %3\n" " addib,< 0, %2, 1b\n" <--- While my understanding of HPPA assembler is limited, it does not seem to make much sense to subtract 0 from a register and to expect the result to ever be negative. Subtracting 1 from the length parameter makes more sense. On top of that, the operation should be repeated if and only if the result is still > 0, so change the suspicious instruction to " addib,> -1, %2, 1b\n" The IP checksum unit test passes after this change. Cc: Charlie Jenkins <charlie@rivosinc.com> Cc: Palmer Dabbelt <palmer@rivosinc.com> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- Prerequisite for this patch is https://lore.kernel.org/lkml/20240207-fix_sparse_errors_checksum_tests-v6-0-4caa9629705b@rivosinc.com/ No idea how that was not detected before. Maybe I am missing something. Note that test_csum_ipv6_magic still fails on 32-bit parisc systems after this patch has been applied. That is a different problem. arch/parisc/include/asm/checksum.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)