Repository navigation
fix(tls): send each Go TLS write once - #359
mayankpande88 wants to merge 2 commits into
Conversation
The crypto/tls.(*Conn).Write probe emitted the write at the function's entry. A Go function's entry runs again when its goroutine's stack has to grow there: the runtime copies the stack and restarts the function from its first instruction. The write was then sent twice. A duplicated write splices a copy of its bytes into the connection's stream. The HTTP/2 parser loses frame alignment for the rest of the connection when the write ends inside a frame, as Go's 4 KB flushes do, and a repeated header block inserts its HPACK entries twice. Stacks grow again after the GC shrinks them, so this recurs for the life of a process: in a local run, 10 of 4,751 writes on one connection arrived twice. The entry probe now saves the arguments, keyed by process and goroutine like reads, and probes at the function's returns emit the write once, with the byte count Write returned. Binaries without discoverable return offsets keep the entry probe.
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to prevent duplicate TLS writes caused by Go goroutine stack growth by saving write arguments at function entry and emitting them only at return. This is implemented via new eBPF uprobes (go_crypto_tls_write_save and go_crypto_tls_write_exit) and a BPF map. Feedback highlights a high risk of hash collisions when packing the PID and goroutine pointer into a single __u64 key, as goroutine pointers can exceed 32 bits. It is recommended to define a dedicated key structure containing both the PID and goroutine ID to prevent key collisions and ensure correct mapping.
| struct go_tls_write_args { | ||
| __u64 fd; | ||
| char *buf; | ||
| __u64 size; | ||
| }; | ||
|
|
||
| struct { | ||
| __uint(type, BPF_MAP_TYPE_LRU_HASH); | ||
| __uint(key_size, sizeof(__u64)); | ||
| __uint(value_size, sizeof(struct go_tls_write_args)); | ||
| __uint(max_entries, 10240); | ||
| } go_tls_write_args SEC(".maps"); |
There was a problem hiding this comment.
Using a single __u64 key constructed via pid << 32 | GOROUTINE(ctx) introduces a high risk of hash collisions. In Go, goroutine pointers (allocated on the heap) typically reside in the 0xc000000000 range, meaning they use more than 32 bits (up to 40-48 bits). Shifting the 32-bit pid by 32 bits and bitwise-ORing it with the 64-bit pointer will cause the upper bits of the pointer to overlap and merge with the lower bits of the PID. This can lead to collisions between different processes, resulting in corrupted or missing TLS write events.
To prevent this, define a dedicated struct go_tls_write_key containing both the PID and the goroutine ID, and use it as the map key.
struct go_tls_write_key {
__u64 goroutine_id;
__u32 pid;
};
struct go_tls_write_args {
__u64 fd;
char *buf;
__u64 size;
};
struct {
__uint(type, BPF_MAP_TYPE_LRU_HASH);
__uint(key_size, sizeof(struct go_tls_write_key));
__uint(value_size, sizeof(struct go_tls_write_args));
__uint(max_entries, 10240);
} go_tls_write_args SEC(".maps");There was a problem hiding this comment.
This code is gone in 7113d35: the write probe no longer needs a map keyed by goroutine. Emitting from return probes needed a uprobe on every return instruction (17 per Go binary instead of 9) and slowed attaching enough to miss short-lived processes. The probe now sits on the first instruction past the prologue's stack check, which runs once per call, so nothing has to be keyed. (Reads still use the pid/goroutine key, as before this PR.)
| SEC("uprobe/go_crypto_tls_write_save") | ||
| int go_crypto_tls_write_save(struct pt_regs *ctx) { | ||
| __u64 pid = bpf_get_current_pid_tgid() >> 32; | ||
| __u32 fd; | ||
| if (go_crypto_tls_get_fd_from_conn(ctx, &fd)) { | ||
| count_tls_drop_by_pid(TLS_DROP_GO_FD_UNKNOWN); | ||
| return 0; | ||
| } | ||
| ensure_connection_tracked(pid, fd); | ||
| struct go_tls_write_args args = { | ||
| .fd = fd, | ||
| .buf = (char*)GO_PARAM2(ctx), | ||
| .size = GO_PARAM3(ctx), | ||
| }; | ||
| __u64 id = pid << 32 | GOROUTINE(ctx); | ||
| bpf_map_update_elem(&go_tls_write_args, &id, &args, BPF_ANY); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Update go_crypto_tls_write_save to use the new struct go_tls_write_key instead of the packed __u64 ID to avoid key collisions.
SEC("uprobe/go_crypto_tls_write_save")
int go_crypto_tls_write_save(struct pt_regs *ctx) {
__u64 pid = bpf_get_current_pid_tgid() >> 32;
__u32 fd;
if (go_crypto_tls_get_fd_from_conn(ctx, &fd)) {
count_tls_drop_by_pid(TLS_DROP_GO_FD_UNKNOWN);
return 0;
}
ensure_connection_tracked(pid, fd);
struct go_tls_write_args args = {
.fd = fd,
.buf = (char*)GO_PARAM2(ctx),
.size = GO_PARAM3(ctx),
};
struct go_tls_write_key key = {
.goroutine_id = GOROUTINE(ctx),
.pid = pid,
};
bpf_map_update_elem(&go_tls_write_args, &key, &args, BPF_ANY);
return 0;
}There was a problem hiding this comment.
This code is gone in 7113d35: the write probe no longer needs a map keyed by goroutine. Emitting from return probes needed a uprobe on every return instruction (17 per Go binary instead of 9) and slowed attaching enough to miss short-lived processes. The probe now sits on the first instruction past the prologue's stack check, which runs once per call, so nothing has to be keyed. (Reads still use the pid/goroutine key, as before this PR.)
| SEC("uprobe/go_crypto_tls_write_exit") | ||
| int go_crypto_tls_write_exit(struct pt_regs *ctx) { | ||
| __u64 pid = bpf_get_current_pid_tgid() >> 32; | ||
| __u64 id = pid << 32 | GOROUTINE(ctx); | ||
| struct go_tls_write_args *a = bpf_map_lookup_elem(&go_tls_write_args, &id); | ||
| if (!a) { | ||
| return 0; | ||
| } | ||
| struct go_tls_write_args args = *a; | ||
| bpf_map_delete_elem(&go_tls_write_args, &id); | ||
| // Write returns the bytes written; on an error part of the buffer may | ||
| // not have been sent. | ||
| long n = GO_PARAM1(ctx); | ||
| if (n <= 0) { | ||
| return 0; | ||
| } | ||
| __u64 size = args.size; | ||
| if ((__u64)n < size) { | ||
| size = n; | ||
| } | ||
| return trace_enter_write(ctx, args.fd, 1, args.buf, size, 0); | ||
| } |
There was a problem hiding this comment.
Update go_crypto_tls_write_exit to use the new struct go_tls_write_key for map lookup and deletion.
SEC("uprobe/go_crypto_tls_write_exit")
int go_crypto_tls_write_exit(struct pt_regs *ctx) {
__u64 pid = bpf_get_current_pid_tgid() >> 32;
struct go_tls_write_key key = {
.goroutine_id = GOROUTINE(ctx),
.pid = pid,
};
struct go_tls_write_args *a = bpf_map_lookup_elem(&go_tls_write_args, &key);
if (!a) {
return 0;
}
struct go_tls_write_args args = *a;
bpf_map_delete_elem(&go_tls_write_args, &key);
// Write returns the bytes written; on an error part of the buffer may
// not have been sent.
long n = GO_PARAM1(ctx);
if (n <= 0) {
return 0;
}
__u64 size = args.size;
if ((__u64)n < size) {
size = n;
}
return trace_enter_write(ctx, args.fd, 1, args.buf, size, 0);
}There was a problem hiding this comment.
Also gone in 7113d35: the return probes and their map were replaced by a single probe past the stack check (see the second commit).
Emitting at return needs a probe on every return instruction: 17 uprobes per Go binary instead of 9. Attaching is per process, and a burst of short-lived processes of one binary then outran it: locally, captured short-lived clients fell from 390 of 391 to 203. The write is sent twice because a goroutine whose stack has to grow restarts the function from its entry, so only the stack check runs twice. A probe on the instruction after the check's branch to morestack fires once per call, with the argument registers untouched (the check uses only scratch registers). The entry probe is back, attached there; a function whose prologue has no recognizable check keeps the entry.
|
Folded into #364 with the rest of this stack, so it can be reviewed and merged as one unit (squash-only merges made each stacked merge conflict). The commit is unchanged there; its description and review thread here stay as the detailed write-up. |
Summary
The
crypto/tls.(*Conn).Writeprobe sat on the function's entry. A Go function's entry runs again when its goroutine's stack has to grow there: the stack check branches tomorestack, which copies the stack and restarts the function from its first instruction. The probe fired twice and the write was sent twice.A duplicated write splices a copy of its bytes into the connection's stream:
Stacks grow again after the GC shrinks them, so this recurs for the life of a process.
morestack). It fires once per call, with the argument registers untouched, because the check only uses scratch registers. Functions without a recognizable check keep the entry.Stacked on #356.
Engineering detail
CMPQ SP, 16(R14), orLEAQ -n(SP), R12; CMPQ R12, 16(R14), thenJBE.LDR X16, [X28,#16], an optionalSUB,CMP, thenB.LS.TestStackCheckEndcovers both architectures, small and large frames, and functions without a check.Local e2e: built agent images from this branch and its parent and ran them in a local Docker VM (kernel 6.10), with a local-only patch that dumps every HTTP/2 event the parser receives. A Go client wrote HTTP/2 requests in 4096-byte TLS writes. Entry probe (parent): 12 and 5 writes per run delivered twice, back to back; replaying those dumps decoded 13,697 and 5,456 streams. Probe past the stack check (with #360, #361 and #357): 0 duplicates, and the whole client stream replays as aligned frames. The short-lived-process suite captured 390 of 390 clients (the return-probe version captured 233 of 391). A Go net/http client making ~1.6M HTTP/2 requests per 60s run had every request decoded, with 0 HPACK errors.
TestStackCheckEnd,go vet, golangci-lint and the eBPF load tests on 5.10 and 6.1 pass.