Page MenuHomeFreeBSD

ICMP checksum test: Fix for big endian
ClosedPublic

Authored by renato.riolino_eldorado.org.br on Jan 15 2020, 4:53 PM.

Details

Summary

According to RFC 1071, ICMP checksum is byte order independent. Just had to swap bytes of input data on EB because in_cksum() casts them to u_short.

Test Plan

cd /usr/src/sbin/ping/tests
make clean
make
make install
kyua test -k /usr/tests/sbin/ping/Kyuafile

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Hello, thank you for the patch. I think I can see your point.

In my view, the tests in in_cksum_test.c should prove that the in_cksum() is byte order indepentent no matter for which endianness (big, little) they are compiled for and executed on. That means, that there should be both types of input data (big and little endian).

Maybe, there could be some issues with interpretation of the results of in_cksum() on different endianness architectures. I need to look into this a bit more.

Ok, now it makes sense. But then something is wrong with the test or with the in_cksum implementation, since it is failing on all tests on BE. For been independent of the endianess, I suppose the result checksum for {0x12, 0x34, 0x56, 0x78} and {0x34, 0x12, 0x78, 0x56} should be the same, right?

Original test wasn't working on BE. Just swaping the input data and testing on the same platform is not the same as testing the same input on a different platform. The best prove that in_cksum is enadiannes independent is that the same input outputs the same value on both platforms as long as you test the output byte-a-byte.

Just swaping the input data and testing on the same platform is not the same as testing the same input on a different platform.

I understand that those are two different strategies testing different properties of the algorithm or its implementation. We have to wait for a FreeBSD developer to choose an approach that will be accepted.

I agree with Renato. There's no way to test in_cksum on both endiannesses without using something complicated like qemu. The right approach is to checksum the same input on different platforms, and assert that the result is identical on all. That's what Renato's change does. @renato.riolino_eldorado.org.br I don't think you have a commit bit, right? If not, I can commit the change for you. But I don't have any BE hardware. Can you first confirm that with your change the tests pass on BE hardware on a newish build?

This revision is now accepted and ready to land.Mar 24 2020, 3:31 AM

I agree with Renato. There's no way to test in_cksum on both endiannesses without using something complicated like qemu. The right approach is to checksum the same input on different platforms, and assert that the result is identical on all. That's what Renato's change does. @renato.riolino_eldorado.org.br I don't think you have a commit bit, right? If not, I can commit the change for you. But I don't have any BE hardware. Can you first confirm that with your change the tests pass on BE hardware on a newish build?

Yes, this test is working on both endianness.

Thanks

This review was approved in March last year. It seems all issues have been solved. Could anyone commit this?

This review was approved in March last year. It seems all issues have been solved. Could anyone commit this?

Yeah. I'll try to rebase and retest tonight.

This revision was automatically updated to reflect the committed changes.