Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gk: fix checksum issue #425

Merged
merged 1 commit into from
Aug 31, 2020
Merged

Conversation

mengxiang0811
Copy link
Collaborator

Some examples that implement the RFC1624 [Eqn. 4] use the network order:
https://elixir.bootlin.com/linux/latest/source/net/ipv4/netfilter/ipt_ECN.c#L38

This patch removes the byte-order convertion for checksum.

@AltraMayor
Copy link
Owner

Notice that the given example actually implements equation 3 of RFC1624. Moreover, one's complement sum and one's complement (i.e. C's operator ~) preserve endianness, that is, if all inputs are little-endian, the result is little-endian. The same is true for big-endian. This endianness preservation is even independent of the endianness of the host! That is why the given example works.

We have assumed that the two's complement subtraction, which is currently being used, is the same as "subtracting complements with borrow" under one's complement as required in RFC1624. While these two operations often come up with the same result, they are often not equal too. And this is the bug we are facing. In addition, the two's complement subtraction is not endianness preserving!

The solution is to fix the current code using the following functions:

/*
 * One's complement sum.
 *
 * Notice that if @a and @b are little-endian, the result is also
 * little-endian. The same is true for big-endian. In order words,
 * this function preserves endianness.
 *
 * The endianness preservation is independent of the endianness of the host.
 */
static inline uint16_t
onec_add(uint16_t a, uint16_t b)
{
	uint16_t res = a + b;
	return res + (res < b);
}

/*
 * The result has the same endianness of the inputs as long as
 * all inputs have the same endianness.
 * The endianness preservation is independent of the endianness of the host.
 */
static inline uint16_t
new_ip_csum(uint16_t old_csum, uint16_t old16, uint16_t new16)
{
	/* According to RFC1624 [Eqn. 3]. */
	return ~onec_add(onec_add(~old_csum, ~old16), new16);
}

Don't forget to update the description of the patch with a better text, for example, explain when the bug happens.

@mengxiang0811
Copy link
Collaborator Author

Thanks so much @AltraMayor ! This rebase is ready for another review.

@mengxiang0811 mengxiang0811 force-pushed the checksum branch 2 times, most recently from e746d6d to 6edb33d Compare August 29, 2020 02:55
@AltraMayor
Copy link
Owner

The code is ready for merge, but please drop the whole parenthesis in this section of the patch comment: the two's complement (i.e. C's operator ~). The operator ~ produces one's complements.

The checksum bug discussed in AltraMayor#417 (comment) is because we have assumed that
the two's complement subtraction, which is currently being used, is the same as
"subtracting complements with borrow" under one's complement as required in
RFC1624. While these two operations often come up with the same result, they
are often not equal too. In addition, the two's complement subtraction is not
endianness preserving!

To solve this issue, we followed the example implmentation of RFC1624 [Eqn. 3]
in Linux kernel:
https://elixir.bootlin.com/linux/latest/source/net/ipv4/netfilter/ipt_ECN.c#L38
@mengxiang0811
Copy link
Collaborator Author

This rebase is ready for another review.

@AltraMayor AltraMayor merged commit 1bdfd1d into AltraMayor:master Aug 31, 2020
@mengxiang0811 mengxiang0811 deleted the checksum branch September 1, 2020 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants