feature: TCP MSS clamping support (bidirectional)#123
Conversation
|
I am a little hesitant to merge a feature like this - the Operating System itself wil have a mss clamp feature that can be enabled on the virtual ethernet device. Thus this would expand the code that needs to be supported and at the same time the best implementation would not be this one. Can you explain your reasons for needing to add this feature into n3n instead of just using the OS clamping? |
|
Additionally, please ensure you have run |
|
Thanks for the feedback. To save your time, I'll get straight to the point—I feel an internal implementation is better for two reasons:
I’m open to refactoring the code if you have a better implementation in mind. Looking forward to your thoughts. |
bb0ef2d to
d638df6
Compare
hamishcoleman
left a comment
There was a problem hiding this comment.
While I understand the desire to have an all-in-one solution, it does bring maintenance overheads and dilutes the aims to be a simple ethernet packet switch that this project embodies.
By setting the MTU of the interface correctly, everything except for routing/bridging is already handled. I would suggest that anyone doing routing and bridging is already in a more complex environment and has a number of external settings that need to be changed (setting forwarding to enabled or creating a bridging setup both need external scripts or tools to make them work already)
The existing UDP packet fragmentation already handles the transport of data as well - which means that the large benefit of clamping is going to be speed, and if speed is your main aim, an in-kernel VPN system would be faster.
The in-kernel firewall MSS clamp features would also be faster and contain less bugs. (And the kernel features also work with bridge networks)
I'm happy to keep discussing, but I'm still not immediately convinced
| /* TCP MSS clamping */ | ||
| uint16_t tcp_csum_update (uint16_t old_csum, uint16_t old_val, uint16_t new_val); | ||
| void clamp_mss (struct n3n_runtime_data *eee, uint8_t *tap_pkt, size_t len); |
There was a problem hiding this comment.
As these are internal functions that we do not wish to export to external users, they should not be declared in the public header files.
If the functions are simply placed in the edge_utils.c file before they are called, then there is no need to have any forward definition at all.
There was a problem hiding this comment.
This was because I had prepared unit tests that weren't included in the initial PR, but I have already added them in the subsequent commits.
| * Supports IPv4/IPv6, VLAN/QinQ, IP-in-IP, and IPv6 extension headers. | ||
| * Uses byte-shift reads for alignment safety on ARM/MIPS. | ||
| */ | ||
| void clamp_mss (struct n3n_runtime_data *eee, uint8_t *tap_pkt, size_t len) { |
There was a problem hiding this comment.
There is quite a lot of tricky logic and corner cases in this function. Did this code come from somewhere? Please attribute it so that the provenance can be checked and the appropriate licensing can be reviewed. Also, in the future, fixes from upstream could be cherry picked.
There was a problem hiding this comment.
The foundational workflow is inspired by tinc's clamp_mss, which I have expanded for full protocol support using Qwen Code.
The handling of IPv4/IPv6, VLAN/QinQ, IP-in-IP, and IPv6 ext headers is based on my domain expertise, with Wikipedia used to cross-reference specifications. While AI assisted in the coding, I personally directed the logic and verified the implementation through manual and Gemini AI review.
Add clamp_mss configuration option to clamp TCP MSS values in SYN packets to the TAP interface MTU, preventing fragmentation and connection timeouts caused by MTU mismatches. - tcp_csum_update(): incremental TCP checksum per RFC 1624 - clamp_mss(): handles IPv4/IPv6, 802.1Q VLAN, IP-in-IP, IPv6 ext headers - Called in both edge_send_packet2net() (TAP->VPN) and handle_PACKET() (VPN->TAP) for bidirectional clamping - All 16-bit fields use byte-shift reads for ARM/MIPS alignment safety - IPv4 fragmentation detection skips fragmented packets - IPv6 Fragment/Auth extension headers abort to avoid data corruption
- Move clamp_mss field after tuntap_ip_mode to avoid struct hole - Move clamp_mss config option from connection to tuntap section, sorted alphabetically between address_mode and macaddr - Move clamp_mss bool check to caller sites, remove from function - Add provenance comment (inspired by tinc, RFC 1624) - Update test expected output for new config section placement
1af9269 to
f442e0d
Compare
- 32 tests covering IPv4/IPv6, VLAN/QinQ, IP-in-IP, IPv6 ext headers - Edge cases: mtu=0, MSS=0, boundary values, malformed options - Incremental checksum verification (RFC 1624) - Build integration: tests-mss added to tools/Makefile TESTS
f442e0d to
b8dcbdc
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #123 +/- ##
==========================================
- Coverage 48.86% 48.43% -0.44%
==========================================
Files 54 55 +1
Lines 10570 9112 -1458
==========================================
- Hits 5165 4413 -752
+ Misses 5405 4699 -706 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add clamp_mss configuration option to clamp TCP MSS values in SYN packets to the TAP interface MTU, preventing fragmentation and connection timeouts caused by MTU mismatches.