Avoid UDP cipher search if a NAT mapping exists #83
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change limits the UDP cipher search to the first packet on a NAT mapping. Subsequent packets use the cipher associated with the NAT mapping. This assumes that multiple keys do not share a client port, but that assumption was already in place for the downstream connection, which uses a fixed cipher for the duration of a NAT mapping. This makes UDP streams perform similarly to TCP. The BenchmarkUDPManyKeys test was reusing the same client port with multiple ciphers (and not attempting to decrypt the returned packets), so it had to be modified to avoid client port reuse. The new version of that test shows the effect of this optimization: Before: BenchmarkUDPManyKeys-4 1660 752446 ns/op 130564 B/op 1740 allocs/op After: BenchmarkUDPManyKeys-4 12822 90546 ns/op 6845 B/op 112 allocs/op This test is something of a worst-case scenario (100 keys on the same IP, interleaved), so real usage won't show such a large improvement. In real use, the main benefit is in avoiding copying of the cipher list on every packet. The new BenchmarkSnapshot test shows that this takes ~5 ns per key on my laptop. Decryption takes 7000 ns, so copying the list dominates the upstream CPU usage of UDP streams on servers with 1500+ keys.
|
@JonathanDCohen FYI, this corresponds to some discussions we've had. |
fortuna
approved these changes
Sep 14, 2020
shadowsocks/udp.go
Outdated
| ip := clientAddr.(*net.UDPAddr).IP | ||
| var cipher shadowaead.Cipher | ||
| unpackStart := time.Now() | ||
| textData, keyID, cipher, err = unpack(ip, textBuf, cipherData, s.ciphers) |
There was a problem hiding this comment.
Maybe rename this unpack so it's clearer it's doing the trial decryption?
There was a problem hiding this comment.
OK, renamed to findAccessKeyUDP to match findAccessKey in tcp.go.
fortuna
approved these changes
Sep 15, 2020
bemasc
pushed a commit
that referenced
this pull request
Nov 2, 2020
Currently, each valid shadowsocks packet causes a NAT mapping to be created (if there isn't a matching one already), even if that packet is rejected due to a malformed or disallowed destination address. This is a regression due to #83. The regression results in a significant memory leak, because unused mappings are never cleaned up.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change limits the UDP cipher search to the first packet on a NAT
mapping. Subsequent packets use the cipher associated with the NAT
mapping. This assumes that multiple keys do not share a client port,
but that assumption was already in place for the downstream connection,
which uses a fixed cipher for the duration of a NAT mapping.
This makes UDP streams perform similarly to TCP.
The BenchmarkUDPManyKeys test was reusing the same client port with
multiple ciphers (and not attempting to decrypt the returned packets),
so it had to be modified to avoid client port reuse. The new version of
that test shows the effect of this optimization:
Before:
BenchmarkUDPManyKeys-4 1660 752446 ns/op 130564 B/op 1740 allocs/op
After:
BenchmarkUDPManyKeys-4 12822 90546 ns/op 6845 B/op 112 allocs/op
This test is something of a worst-case scenario (100 keys on the same
IP, interleaved), so real usage won't show such a large improvement.
In real use, the main benefit is in avoiding copying of the cipher
list on every packet. The new BenchmarkSnapshot test shows that this
takes ~5 ns per key on my laptop. Decryption takes 7000 ns, so copying
the list dominates the upstream CPU usage of UDP streams on servers with
1500+ keys.