Re: [PATCH] parisc: Fix ip_fast_csum

[Charlie Jenkins]

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@xxxxxxxxxxxx>
> Cc: Palmer Dabbelt <palmer@xxxxxxxxxxxx>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> ---
> Prerequisite for this patch is
> https://lore.kernel.org/lkml/20240207-fix_sparse_errors_checksum_tests-v6-0-4caa9629705b@xxxxxxxxxxxx/
> 
> 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@xxxxxxxxxxxx>
Reviewed-by: Charlie Jenkins <charlie@xxxxxxxxxxxx>