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

Fix UDPv6 checksum #3693

Merged
merged 2 commits into from
Jun 12, 2023
Merged

Fix UDPv6 checksum #3693

merged 2 commits into from
Jun 12, 2023

Conversation

csujedihy
Copy link
Contributor

@csujedihy csujedihy commented Jun 12, 2023

Description

UDPv6 requires checksum to be non-zero.

Testing

CI

@csujedihy csujedihy added the Area: Lola Related to low latency work label Jun 12, 2023
@csujedihy csujedihy requested a review from a team as a code owner June 12, 2023 01:21
@nibanks
Copy link
Member

nibanks commented Jun 12, 2023

Also, UDPv4 can be skipped.

Why?

@csujedihy
Copy link
Contributor Author

Also, UDPv4 can be skipped.

Why?

UDPv4 checksum is optional. UDP checksum is required only with IPv6.

@nibanks
Copy link
Member

nibanks commented Jun 12, 2023

Also, UDPv4 can be skipped.

Why?

UDPv4 checksum is optional. UDP checksum is required only with IPv6.

I assume you haven't validated this in a real environment (i.e. Azure VM)? I will sign off, and leave it up to you if you're sure enough to merge without further validation.

nibanks
nibanks previously approved these changes Jun 12, 2023
@csujedihy
Copy link
Contributor Author

Also, UDPv4 can be skipped.

Why?

UDPv4 checksum is optional. UDP checksum is required only with IPv6.

I assume you haven't validated this in a real environment (i.e. Azure VM)? I will sign off, and leave it up to you if you're sure enough to merge without further validation.

Some middleboxes might not like zero UDPv4 checksum. Though, I am pretty sure it works fine over azure with linux and windows. I am gonna revert this change to be safe. We could add a knob to disable UDPv4 checksum in the future even for regular QUIC. We have UDP_NOCHECKSUM for windows UDP sockets to disable it, and UDPv4 checksum is default on.

@csujedihy csujedihy changed the title Make UDPv4 xsum optional and fix UDPv6 checksum Fix UDPv6 checksum Jun 12, 2023
@mtfriesen
Copy link
Contributor

Does this handle the UDPv6 case that we recently fixed in XDP? That is, the UDPv6 checksum must not be zero?

@mtfriesen
Copy link
Contributor

Does this handle the UDPv6 case that we recently fixed in XDP? That is, the UDPv6 checksum must not be zero?

Yes it does. I was looking at the wrong commit diff.

@csujedihy csujedihy merged commit 15c45b7 into main Jun 12, 2023
354 of 361 checks passed
@csujedihy csujedihy deleted the huanyi/udp-xsum-fix-and-opt branch June 12, 2023 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Lola Related to low latency work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants