| Message ID | 20240720063937.2311958-2-davidgow@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] x86: checksum: Fix unaligned checksums on < i686 | expand |
On Sat, Jul 20, 2024 at 2:40 PM David Gow <davidgow@google.com> wrote: > > The checksum_32 code was originally written to only handle 2-byte > aligned buffers, but was later extended to support arbitrary alignment. > However, the non-PPro variant doesn't apply the carry before jumping to > the 2- or 4-byte aligned versions, which clear CF. > > This causes the new checksum_kunit test to fail, as it runs with a large > number of different possible alignments and both with and without > carries. > > For example: > ./tools/testing/kunit/kunit.py run --arch i386 --kconfig_add CONFIG_M486=y checksum > Gives: > KTAP version 1 > # Subtest: checksum > 1..3 > ok 1 test_csum_fixed_random_inputs > # test_csum_all_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:267 > Expected result == expec, but > result == 65281 (0xff01) > expec == 65280 (0xff00) > not ok 2 test_csum_all_carry_inputs > # test_csum_no_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:314 > Expected result == expec, but > result == 65535 (0xffff) > expec == 65534 (0xfffe) > not ok 3 test_csum_no_carry_inputs > > With this patch, it passes. > KTAP version 1 > # Subtest: checksum > 1..3 > ok 1 test_csum_fixed_random_inputs > ok 2 test_csum_all_carry_inputs > ok 3 test_csum_no_carry_inputs > > I also tested it on a real 486DX2, with the same results. > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Signed-off-by: David Gow <davidgow@google.com> > --- > > Re-sending this from [1]. While there's an argument that the whole > 32-bit checksum code could do with rewriting, it's: > (a) worth fixing before someone takes the time to rewrite it, and > (b) worth any future rewrite starting from a point where the tests pass > > I don't think there should be any downside to this fix: it only affects > ancient computers, and adds a single instruction which isn't in a loop. > > Cheers, > -- David > > [1]: https://lore.kernel.org/lkml/20230704083206.693155-2-davidgow@google.com/ > > --- > arch/x86/lib/checksum_32.S | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/x86/lib/checksum_32.S b/arch/x86/lib/checksum_32.S > index 68f7fa3e1322..a5123b29b403 100644 > --- a/arch/x86/lib/checksum_32.S > +++ b/arch/x86/lib/checksum_32.S > @@ -62,6 +62,7 @@ SYM_FUNC_START(csum_partial) > jl 8f > movzbl (%esi), %ebx > adcl %ebx, %eax > + adcl $0, %eax > roll $8, %eax > inc %esi > testl $2, %esi > -- > 2.45.2.1089.g2a221341d9-goog > I'm not maintainer but LGTM. Reviewed-by: Noah Goldstein <goldstein.w.n@gmail.com>
diff --git a/arch/x86/lib/checksum_32.S b/arch/x86/lib/checksum_32.S index 68f7fa3e1322..a5123b29b403 100644 --- a/arch/x86/lib/checksum_32.S +++ b/arch/x86/lib/checksum_32.S @@ -62,6 +62,7 @@ SYM_FUNC_START(csum_partial) jl 8f movzbl (%esi), %ebx adcl %ebx, %eax + adcl $0, %eax roll $8, %eax inc %esi testl $2, %esi
The checksum_32 code was originally written to only handle 2-byte aligned buffers, but was later extended to support arbitrary alignment. However, the non-PPro variant doesn't apply the carry before jumping to the 2- or 4-byte aligned versions, which clear CF. This causes the new checksum_kunit test to fail, as it runs with a large number of different possible alignments and both with and without carries. For example: ./tools/testing/kunit/kunit.py run --arch i386 --kconfig_add CONFIG_M486=y checksum Gives: KTAP version 1 # Subtest: checksum 1..3 ok 1 test_csum_fixed_random_inputs # test_csum_all_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:267 Expected result == expec, but result == 65281 (0xff01) expec == 65280 (0xff00) not ok 2 test_csum_all_carry_inputs # test_csum_no_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:314 Expected result == expec, but result == 65535 (0xffff) expec == 65534 (0xfffe) not ok 3 test_csum_no_carry_inputs With this patch, it passes. KTAP version 1 # Subtest: checksum 1..3 ok 1 test_csum_fixed_random_inputs ok 2 test_csum_all_carry_inputs ok 3 test_csum_no_carry_inputs I also tested it on a real 486DX2, with the same results. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Signed-off-by: David Gow <davidgow@google.com> --- Re-sending this from [1]. While there's an argument that the whole 32-bit checksum code could do with rewriting, it's: (a) worth fixing before someone takes the time to rewrite it, and (b) worth any future rewrite starting from a point where the tests pass I don't think there should be any downside to this fix: it only affects ancient computers, and adds a single instruction which isn't in a loop. Cheers, -- David [1]: https://lore.kernel.org/lkml/20230704083206.693155-2-davidgow@google.com/ --- arch/x86/lib/checksum_32.S | 1 + 1 file changed, 1 insertion(+)