DSI Quantum optimisations, alignments and performance regression fixes#3196
DSI Quantum optimisations, alignments and performance regression fixes#3196andylemin wants to merge 1 commit into
Conversation
🤖 Augment PR SummarySummary: This PR tightens DSI quantum handling (spec-correct write semantics), eliminates extra buffered-write copying, and updates perf tooling/tests to reflect the corrected behavior. Changes:
Technical Notes: The main semantic change is that the configured/negotiated quantum constrains AFP write payload bytes, while request-block/frame overhead rides on top of that on the wire. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
6d88b3b to
b463fad
Compare
|
@rdmark Performance fully recovered, security patches maintained and one additional vector closed. This is not ready for merge - I am creating a new performance test rig which runs the speedtest and lantest tests;
Ie, the purpose is to provide a static network test environment, rather than the CPU bound loopback based one the performance runners currently use. This will show performance on different real networks much more consistently, and will show the number of packets exchanged, exposing any runt and alignment issues. We need to merge that PR before this one, so we have a before/after this PR for a true concrete comparison? |
|
impressive work, this turned out to be a quite big changeset! do you think it's feasible to backport the performance regression to the stable 4.5 branch? I think it's worth putting out another bugfix release for this. it makes sense to hold off on merging this while you introduce new testing capabilities to properly prove the improvement |
The key parts yes (byte accounting and the extra security fix). This includes several changes which do not need to be back-ported (such as the MSS alignment, which adjusts the quantum value to a figure which is divisible by the MSS, mitigating the runt packet at the end of every single DSI record).
Absolutely, This one needs to be grounded in evidence as there is a lot of hard theory work here. |
|
Performance BEFORE quantum security hardening; Performance AFTER quantum security hardening (more secure but performance regression); Performance AFTER quantum security hardening and AFTER this fix All test files sizes bellow 8-16MiB have significant performance loss, as every single test payload is a multiple of KiB/MiB, however the server DSI quantum became un-aligned during the security fixes, resulting in a runt DSI record for every single DSI record stream. |
|
@rdmark It looks bigger than it is due to the number of commits - mostly unittests due to the critical nature of the code. I added tests first (to ensure they failed) and then confirm they turned green after adding the respective fixes. Once it is squashed it will be smaller and easier to review. I have just modified the quantum alignment algorithm to scale up (instead of down) to the nearest value which is divisible by 4KiB and aligns the DSI record size with the connections negotiated MSS ensure no runt packets at the end of each record. So there is some churn which will vanish |
|
augment review |
8371c61 to
b23227e
Compare
Problems: dsi_stream_receive bounded the whole DSI frame by the server
quantum, so a spec-legal FPWrite carrying exactly quantum data bytes
(dsi_len = quantum + doff) tore down the session. To stay under that
bound the tools subtracted the frame overhead from every transfer, so
clients never used the full negotiated quantum in either direction --
a throughput loss on both writes and reads from incorrect header
accounting, not a real protocol limit. dsi_writeinit copied the
buffered payload prefix into the caller's buffer (a second userspace
copy of up to the read-ahead size per write) and silently truncated
when the prefix exceeded the caller's buffer. The test client sent
each write frame as two syscalls, leaving a runt TCP segment before
every payload; transfers also ended in a runt segment whenever the
quantum was not aligned to the path MSS.
Model: per the AFP spec (DSIOpenSession, Table 1: the request quantum
is the write data size 'not including the length of the DSI header and
the AFP command'), the server quantum counts FPWrite/FPWriteExt data
only; the 16-byte DSI header and the request block ride on top. The
session quantum is resolved once per session after accept(): the
smallest 4 KiB multiple at or above the configured value whose
FPWriteExt frame (data + 36 bytes) ends in a TCP segment at least 7/8
full, growing by at most 256 KiB, capped at 256 MiB; best fill wins
when the threshold is unreachable. Buffers, inbound checks, and the
advertised value all use the grown quantum, so advertised == capacity.
Fixes: writes are bounded by dsi_len - doff with doff validated to
{12, 20}; non-write commands keep the strict bound. This restores
full-quantum transfers -- a client may again send (and request) a
complete server_quantum of data per frame, recovering the throughput
the overhead-subtraction workarounds gave up. dsi_writeinit hands the
buffered payload back by reference (NULL drains it) -- one copy, no
truncation, and afp_addicon's error path drains and returns instead of
re-arming the transfer. The client sends header, request block, and
payload in a single writev; speedtest gains an FPWriteExt mode
(default for AFP >= 3.0, -E for legacy) and both tools size transfers
from the full advertised quantum with no overhead subtraction and no
zero-quantum division. dsireadbuf defaults to 32, capped so dsireadbuf
x quantum stays under 1 GiB per session; server quantum values above
the maximum clamp instead of falling back to the default.
dsi_writeinit's signature change bumps the soversion to 21.
Security: the DSI frame-parsing hardening from CVE-2026-62319 and
CVE-2026-62320 (commit cc0a9d1) stays fully in place and is
strengthened -- the write data offset is now validated against an
exact {12, 20} whitelist, so malformed offsets 13-19 are rejected
instead of parsed, and dsi_tcp_open's first-frame whole-frame bound is
unchanged. Those fixes correctly reject a frame whose total exceeds
the quantum; the tools had compensated with frame-overhead
subtraction (PR #3186, PR #3187), which shrank the usable quantum.
This supersedes those workarounds by fixing the server semantics they
compensated for -- restoring full-quantum throughput -- while
preserving their invariant that the client never emits a frame the
hardened parser rejects.
Additionally closes an out-of-bounds read in the FPAddIcon write path:
the old dsi_writeinit truncated the buffered prefix into the caller's
buffer via memmove(buf, start, MIN(buflen, bytes)) but returned the
untruncated byte count, and afp_addicon then wrote that count from a
64 KiB (DSI_DATASIZ) buffer -- reading past the buffer and emitting
stale heap bytes into the icon file whenever a buffered write prefix
exceeded 64 KiB against the 1 MiB default quantum. The pointer handoff
returns the exact buffered range, removing the over-read.
Tests: afpd unit tests pin full-quantum accept (both doffs),
over-quantum reject, doff validation, the strict non-write bound, the
no-duplicate-copy handoff, the alignment search (growth, page
alignment, caps, overflow at the top of the range), and the read-ahead
clamp. RED runs for the two server bugs are preserved in the PR's CI
history. The speedtest dashboard gains sweep-average columns so
small-file regressions are visible alongside peaks.
|
📊 Performance DashboardCommit: 🔥 Spectest (AFP 3.4) - FlameGraphNetatalk Code-time: 2.1% · Runtime: 68s · Stacks: 1143 🔥 Click the preview to open the interactive flamegraph (zoom + search). 🔝 Top 10 leaf functions
📈 Speedtest (AFP 3.4) - PerfGraph🔝 Throughputs per operation
⏱️ Lantest (AFP 3.4) - LatencyGraphAvg total runtime: 7373 ms · Avg time per AFP op: 0.116 ms 🐢 All operations (avg runtime, in test order)
|
The decimal KB/MB switch was a workaround for the runt-frame effect of the pre-fix server, which advertised a quantum just below the configured power of two so every binary-sized transfer ended in a near-empty frame. PR #3196 makes the quantum count write data only and grows it to MSS alignment, so binary sizes no longer trail a runt frame -- and binary KiB/MiB is what other AFP/SMB testing tools use. Revert the units in speedtest and lantest (file sizes, the -z sweep, axis labels, the man page, and the CI/perftest sweep lists) to binary. Keep the two non-unit changes from that commit: the -d single-size sentinel fix (SizeFlagSeen) and the shaped-network perftest harness.
|
@rdmark Ready for review. Can be merged in any order as I have already tested using 3203 before and after this - all validated and ready to go Interesting comparing the speedtests; "Performance BEFORE quantum security hardening", "Performance AFTER quantum security hardening, and the last Performance Dashboard 🙂 |
andylemin
changed the title
2 participants
The DSI server quantum governs how much file data a client moves per FPWrite/FPWriteExt request, and — because clients derive their read chunk size from it — per read as well. The recent DSI frame-parsing hardening (CVE-2026-62319 / CVE-2026-62320, commit
cc0a9d16) correctly rejects any frame whose total size exceeds the negotiated quantum. But the server was measuring the wrong thing: it bounded the whole DSI frame (header + request block + data) by the quantum, when the AFP spec defines the quantum as the data size only. A spec-legal write of exactlyquantumdata bytes therefore had a frame ofquantum + doff, which the hardened parser tore down as oversized.The tools papered over this by subtracting the frame overhead from every transfer (PR #3186, PR #3187), so clients never used the full negotiated quantum in either direction. That restored correctness but gave up throughput — a workaround for a server-side accounting bug, not a real protocol limit.
This PR fixes the accounting at the source, restores full-quantum transfers, aligns the advertised quantum to the connection's TCP MSS so no frame ends in a runt segment, and removes a redundant userspace copy on the write receive path. Along the way it closes an out-of-bounds read in the FPAddIcon write path. All of the CVE hardening stays in place and is tightened.
Root cause
Per the AFP specification, the server request quantum is the size of the write data alone.
DSIOpenSession, Table 1:and
DSIWrite:The server instead enforced the quantum against the entire frame. In
dsi_stream_receivethe checkif (dsi_len > dsi->server_quantum)ran before any WRITE/doffhandling, so for a write framedsi_lenwasdata + doffand a full-quantum write (data == quantum) was rejected. Because clients size their read requests from the advertised quantum too, the client-side workaround (subtracting frame overhead) degraded read throughput as well as write.Two further defects on the write path:
dsi_writeinitmemmoved the already-buffered payload prefix out of the read-ahead buffer into the caller's buffer (a second userspace copy of up to the read-ahead size per write), and itsmemmove(buf, start, MIN(buflen, bytes))paired with an untruncatedbytesreturn silently dropped data whenever the prefix exceeded the caller's buffer.dsi_stream_send, then the payload viadsi_stream_write— so withTCP_NODELAYa runt segment preceded every payload.Model
The server quantum counts FPWrite/FPWriteExt data only; the 16-byte DSI header and the request block (12 bytes FPWrite, 20 bytes FPWriteExt) ride on top of it on the wire.
The session quantum is resolved once per session, immediately after
accept()(before any buffer is allocated), from the configured value and the socket's measured MSS:Growing (rather than rounding down) means the client never gets less than the administrator configured. Page alignment keeps the data a 4 KiB multiple; the 7/8-fill threshold is used because exact dual alignment is impossible when
gcd(4096, mss)does not divide 36 (e.g. MSS 1448 or 8960). Buffers, inbound checks, and the advertised value all use the grown quantum, so advertised == capacity and every client frame fits the server's buffers exactly.MSS is read from
TCP_INFO(Linuxtcpi_snd_mss) orTCP_CONNECTION_INFO(macOStcpi_maxseg), falling back toTCP_MAXSEG; live values already account for active TCP options. Detection self-disables to page-alignment-only where no socket option is available.Fixes
Server receive bound (
libatalk/dsi/dsi_stream.c). ForDSIFUNC_WRITE, the data offset is validated against an exact{12, 20}whitelist, thendoff <= dsi_len, thendsi_len - doff > server_quantumbounds the data. Non-write commands keep the strictdsi_len > server_quantumbound. This restores full-quantum transfers — a client may again send, and request, a completeserver_quantumof data per frame.Zero-copy write handoff (
libatalk/dsi/dsi_write.c).dsi_writeinitnow hands the buffered payload back by reference (char **), or drains it when passedNULL. The prefix is consumed in place — one copy, no truncation.afp_addicon's error path drains and returns the AFP error instead of falling through and re-arming the transfer. Signature change ⇒ soversion 20 → 21.Single-writev client (
test/testsuite/afpclient.c).dsi_stream_send3sends header, request block, and payload in onewritev, so no runt segment precedes the payload.speedtestgains an FPWriteExt mode (default for AFP ≥ 3.0, matching macOS;-Eselects legacy FPWrite), and bothspeedtestandlantestsize transfers from the full advertised quantum — no overhead subtraction, and a guard against a zero (unnegotiated) quantum.Config (
libatalk/util/netatalk_conf.c).dsireadbufdefault 12 → 32, capped sodsireadbuf × quantumstays under 1 GiB per session (NETATALK_READBUF_LIMIT, re-clamped in the child after the quantum grows). Server-quantum values above the maximum now clamp toDSI_SERVQUANT_MAXinstead of silently falling back to the 1 MiB default.Security
The CVE-2026-62319 / CVE-2026-62320 DSI frame-parsing hardening (
cc0a9d16) stays fully in place and is strengthened:{12, 20}whitelist — malformed offsets 13–19 are rejected, not parsed into a nonsense request block.doff <= dsi_lenbound and the ASC-3.7 zero-doffworkaround are preserved.dsi_tcp_open's first-frame whole-frame check is unchanged: a hostile full-quantum WRITE as the first frame is still killed there (conforming clients always sendDSIOpenSessionfirst).This supersedes the frame-overhead-subtraction workarounds from PR #3186 / PR #3187 by fixing the server semantics they compensated for — restoring full-quantum throughput — while preserving their invariant that the client never emits a frame the hardened parser rejects.
Additional vector closed — FPAddIcon out-of-bounds read
The old
dsi_writeinittruncated the buffered prefix into the caller's buffer viamemmove(buf, start, MIN(buflen, bytes))but returned the untruncated byte count.afp_addiconthen wrote that count from a 64 KiB (DSI_DATASIZ) reply buffer — reading past the buffer and emitting stale heap bytes into the icon file whenever a buffered write prefix exceeded 64 KiB against the 1 MiB default quantum. The pointer handoff returns the exact buffered range, removing the over-read.Behaviour changes
dsi_tcp_open: MSS 32768, session quantum 1077248).server quantummaximum is 256 MiB (was 4 GiB); out-of-range configs clamp to the max (above) or the default (below the 32000 minimum).dsireadbufdefault is 32 (max 1024), with the per-session product capped at 1 GiB.DSIOpenSession.Testing
afpd unit tests (
test/afpd/test.c)utest_dsi_receive_accepts_full_quantum_write(doff 12 & 20)utest_dsi_receive_rejects_overquantum_write(doff 12 & 20)utest_dsi_receive_rejects_overquantum_cmdutest_dsi_receive_validates_doffdoffoutside{12, 20}rejected;doff==0maps to 12utest_dsi_writeinit_no_duplicate_copyutest_dsi_align_quantumutest_readbuf_clampdsireadbuf × quantumbounded, floored at 1The two server bugs landed as RED tests one commit ahead of their fixes; those failing CI runs are preserved in this PR's checks history as the RED evidence, before the branch was squashed.
Integration
afp_speedtestandafp_lantestcomplete against the built server in both FPWriteExt and legacy-Emodes.Not in this PR (follow-ups)
lantest(pre-existing, out of scope).Quantum/MSS scaling validation;
