feat: In-place crypto #2385
feat: In-place crypto #2385
Conversation
Only in-place encryption so far, and only for the main data path. Fixes mozilla#2246 (eventually)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2385 +/- ##
========================================
Coverage 95.32% 95.33%
========================================
Files 114 114
Lines 36868 36968 +100
Branches 36868 36968 +100
========================================
+ Hits 35146 35243 +97
- Misses 1718 1719 +1
- Partials 4 6 +2 ☔ View full report in Codecov by Sentry. |
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to 108fb8d. neqo-latest as client
neqo-latest as server
All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
|
Benchmark resultsPerformance differences relative to 379722c. decode 4096 bytes, mask ff: No change in performance detected. time: [11.840 µs 11.876 µs 11.918 µs]
change: [-0.9427% -0.2009% +0.5321%] (p = 0.64 > 0.05)
decode 1048576 bytes, mask ff: Change within noise threshold. time: [2.8846 ms 2.8924 ms 2.9017 ms]
change: [-1.8770% -1.4110% -0.9559%] (p = 0.00 < 0.05)
decode 4096 bytes, mask 7f: No change in performance detected. time: [19.815 µs 19.884 µs 19.958 µs]
change: [-0.4636% +3.8130% +12.051%] (p = 0.54 > 0.05)
decode 1048576 bytes, mask 7f: Change within noise threshold. time: [5.0703 ms 5.0832 ms 5.0967 ms]
change: [+0.0402% +0.4171% +0.7929%] (p = 0.03 < 0.05)
decode 4096 bytes, mask 3f: No change in performance detected. time: [6.9116 µs 6.9545 µs 7.0079 µs]
change: [-1.3035% +0.0195% +1.1017%] (p = 0.98 > 0.05)
decode 1048576 bytes, mask 3f: No change in performance detected. time: [1.4152 ms 1.4208 ms 1.4277 ms]
change: [-0.4010% +0.1870% +0.7749%] (p = 0.55 > 0.05)
coalesce_acked_from_zero 1+1 entries: Change within noise threshold. time: [98.872 ns 99.075 ns 99.280 ns]
change: [+0.1926% +0.5738% +0.9919%] (p = 0.00 < 0.05)
coalesce_acked_from_zero 3+1 entries: No change in performance detected. time: [117.25 ns 117.61 ns 117.99 ns]
change: [-1.1860% -0.1557% +0.5782%] (p = 0.79 > 0.05)
coalesce_acked_from_zero 10+1 entries: No change in performance detected. time: [117.01 ns 117.56 ns 118.17 ns]
change: [-0.3171% +0.2601% +0.7536%] (p = 0.36 > 0.05)
coalesce_acked_from_zero 1000+1 entries: No change in performance detected. time: [97.589 ns 97.718 ns 97.868 ns]
change: [-1.0319% +0.0511% +1.0381%] (p = 0.93 > 0.05)
RxStreamOrderer::inbound_frame(): No change in performance detected. time: [111.21 ms 111.27 ms 111.33 ms]
change: [-0.1102% -0.0334% +0.0452%] (p = 0.41 > 0.05)
SentPackets::take_ranges: No change in performance detected. time: [5.4988 µs 5.6582 µs 5.8251 µs]
change: [-14.582% -3.1118% +4.8905%] (p = 0.74 > 0.05)
transfer/pacing-false/varying-seeds: 💚 Performance has improved. time: [39.778 ms 39.856 ms 39.939 ms]
change: [-8.4091% -8.1545% -7.9079%] (p = 0.00 < 0.05)
transfer/pacing-true/varying-seeds: 💚 Performance has improved. time: [40.172 ms 40.260 ms 40.356 ms]
change: [-7.2430% -6.9256% -6.5994%] (p = 0.00 < 0.05)
transfer/pacing-false/same-seed: 💚 Performance has improved. time: [40.091 ms 40.175 ms 40.265 ms]
change: [-7.3297% -7.0459% -6.7661%] (p = 0.00 < 0.05)
transfer/pacing-true/same-seed: 💚 Performance has improved. time: [40.165 ms 40.237 ms 40.319 ms]
change: [-8.2108% -7.9660% -7.7219%] (p = 0.00 < 0.05)
1-conn/1-100mb-resp/mtu-1504 (aka. Download)/client: 💚 Performance has improved. time: [877.81 ms 888.75 ms 899.85 ms]
thrpt: [111.13 MiB/s 112.52 MiB/s 113.92 MiB/s]
change:
time: [-5.2321% -3.7353% -2.2598%] (p = 0.00 < 0.05)
thrpt: [+2.3121% +3.8802% +5.5210%]
1-conn/10_000-parallel-1b-resp/mtu-1504 (aka. RPS)/client: Change within noise threshold. time: [313.16 ms 315.40 ms 317.72 ms]
thrpt: [31.474 Kelem/s 31.706 Kelem/s 31.933 Kelem/s]
change:
time: [-2.6314% -1.7000% -0.7082%] (p = 0.00 < 0.05)
thrpt: [+0.7133% +1.7294% +2.7025%]
1-conn/1-1b-resp/mtu-1504 (aka. HPS)/client: No change in performance detected. time: [34.044 ms 34.215 ms 34.401 ms]
thrpt: [29.069 elem/s 29.227 elem/s 29.374 elem/s]
change:
time: [-1.3788% -0.6632% +0.0845%] (p = 0.06 > 0.05)
thrpt: [-0.0844% +0.6676% +1.3981%]
1-conn/1-100mb-resp/mtu-1504 (aka. Upload)/client: 💚 Performance has improved. time: [1.5393 s 1.5551 s 1.5713 s]
thrpt: [63.644 MiB/s 64.304 MiB/s 64.965 MiB/s]
change:
time: [-10.431% -9.0563% -7.6859%] (p = 0.00 < 0.05)
thrpt: [+8.3258% +9.9582% +11.646%]
Client/server transfer resultsTransfer of 33554432 bytes over loopback.
|
|
@mxinden when you have a moment, would you take a look at the borrow-checker issue in |
|
Took a quick look. let dcid = Self::opt(dcid_decoder.decode_cid(&mut decoder))?;
if decoder.remaining() < SAMPLE_OFFSET + SAMPLE_SIZE {
return Err(Error::InvalidPacket);
}
let header_len = decoder.offset();
return Ok((
Self {
packet_type: PacketType::Short,
dcid,
scid: None,
token: &[],
header_len,
version: None,
data,
},
&[],
&mut [],
));
I can take a deeper look and try to fix it. |
|
Thanks for the analysis! Wonder if we can make |
|
Ah, never seen this before. That would be error prone as the bytes within the range in |
|
The above described issue, namely that of diff --git a/neqo-transport/src/packet/mod.rs b/neqo-transport/src/packet/mod.rs
index 73b47bcc..779ca72b 100644
--- a/neqo-transport/src/packet/mod.rs
+++ b/neqo-transport/src/packet/mod.rs
@@ -563,7 +563,7 @@ pub struct PublicPacket<'a> {
/// The packet type.
packet_type: PacketType,
/// The recovered destination connection ID.
- dcid: ConnectionIdRef<'a>,
+ dcid: ConnectionId,
/// The source connection ID, if this is a long header packet.
scid: Option<ConnectionIdRef<'a>>,
/// Any token that is included in the packet (Retry always has a token; Initial sometimesThat leaves us with another issue, namely rustc not being able to infer that early returns of |
|
Okay, I got it. Let's take a look at /// `PublicPacket` holds information from packets that is public only. This allows for
/// processing of packets prior to decryption.
pub struct PublicPacket<'a> {
/// The packet type.
packet_type: PacketType,
/// The recovered destination connection ID.
dcid: ConnectionIdRef<'a>,
/// The source connection ID, if this is a long header packet.
scid: Option<ConnectionIdRef<'a>>,
/// Any token that is included in the packet (Retry always has a token; Initial sometimes
/// does). This is empty when there is no token.
token: &'a [u8],
/// The size of the header, not including the packet number.
header_len: usize,
/// Protocol version, if present in header.
version: Option<WireVersion>,
/// A reference to the entire packet, including the header.
data: &'a [u8],
}
This pull request introduces the following change: @@ -564,7 +574,7 @@ pub struct PublicPacket<'a> {
/// Protocol version, if present in header.
version: Option<WireVersion>,
/// A reference to the entire packet, including the header.
- data: &'a [u8],
+ data: &'a mut [u8],
}While An easy fix would be to make diff --git a/neqo-transport/src/packet/mod.rs b/neqo-transport/src/packet/mod.rs
index 73b47bcc..dc85bbd0 100644
--- a/neqo-transport/src/packet/mod.rs
+++ b/neqo-transport/src/packet/mod.rs
@@ -563,12 +563,12 @@ pub struct PublicPacket<'a> {
/// The packet type.
packet_type: PacketType,
/// The recovered destination connection ID.
- dcid: ConnectionIdRef<'a>,
+ dcid: ConnectionId,
/// The source connection ID, if this is a long header packet.
- scid: Option<ConnectionIdRef<'a>>,
+ scid: Option<ConnectionId>,
/// Any token that is included in the packet (Retry always has a token; Initial sometimes
/// does). This is empty when there is no token.
- token: &'a [u8],
+ token: Vec<u8>,
/// The size of the header, not including the packet number.
header_len: usize,
/// Protocol version, if present in header.The above, plus a couple of smaller lifetime changes resolve the borrow checker issues. I will propose a commit with my local changes. |
|
@larseggert let me know what you think of larseggert#34. Note that it only addresses the borrow-checker issues in Happy to look at the |
| )) | ||
| } | ||
|
|
||
| /// # Errors | ||
| /// | ||
| /// This will return an error if the packet cannot be decrypted. | ||
| pub fn decrypt(&self, crypto: &mut CryptoStates, release_at: Instant) -> Res<DecryptedPacket> { | ||
| pub fn decrypt( |
There was a problem hiding this comment.
I'm looking at the calling code and I think that we can change this to take self instead of &mut self. There is some use of the public packet after decryption, but it seems like the necessary pieces can be moved to the decrypted packet.
There was a problem hiding this comment.
I'd suggest a separate PR for this.
|
FWIW, I looked at the |
larseggert
force-pushed
the
feat-inplace-crypto
branch
from
39c0445 to
5c9bc3e
Compare
I actually think they might? Our API after the last round of changes is very similar. (Or I'm missing something.) |
Fixes #2246 (eventually)