Change the CXPLAT LARGE IO BUFFER SIZE to 0xFFE3 on unix platforms. #4556
Conversation
nibanks
added
OS: Ubuntu
external
|
Thanks so much for finding/fixing this bug! |
| @@ -33,8 +33,9 @@ CXPLAT_STATIC_ASSERT((SIZEOF_STRUCT_MEMBER(QUIC_BUFFER, Buffer) == sizeof(void*) | |||
|
|
|||
| // | |||
| // The maximum single buffer size for coalesced IO payloads. | |||
| // Payload size: 65535 - 8 (UDP header) - 20 (IP header) = 65507 bytes. | |||
There was a problem hiding this comment.
What happens for IPv6? Its header size is bigger. We can use this for the pool allocation size, but should we be returning a different max size to actually use when doing IPv6?
There was a problem hiding this comment.
Refer to this article
https://blog.cloudflare.com/virtual-networking-101-understanding-tap/
Payload size: 65507 bytes for IPv4(without options and padding field) and 65527 for IPv6(Payload length field no include IP header)
Perhaps a more rigorous modification is needed here.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4556 +/- ##
==========================================
- Coverage 87.20% 86.79% -0.41%
==========================================
Files 56 56
Lines 17354 17354
==========================================
- Hits 15134 15063 -71
- Misses 2220 2291 +71 ☔ View full report in Codecov by Sentry. |
Description
Sendmsg always returns its length parameter, indicating success. With messages greater than 65507 (0xFFE3) bytes, it returns a Message too long error, corresponding error code EOVERFLOW.
Payload size calculate: 65535 - 8 (UDP header) - 20 (IP header) = 65507 bytes.
Therefore the size of the IO Buffer should be less than or equal to 65507 bytes.
Online discussion of related issues
https://stackoverflow.com/questions/11809727/sendto-returns-values-greater-than-mtu-on-sock-dgram-udp-socket
Describe the purpose of and changes within this Pull Request.
Testing
Do any existing tests cover this change? Are new tests needed?
Documentation
Is there any documentation impact for this change?