Skip to content

fix(tls): send each Go TLS write once - #359

Closed
mayankpande88 wants to merge 2 commits into
fix/l7-ringbuf-sizingfrom
fix/go-tls-write-once
Closed

mayankpande88 wants to merge 2 commits into
fix/l7-ringbuf-sizingfrom
fix/go-tls-write-once

Conversation

@mayankpande88

@mayankpande88 mayankpande88 commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Summary

The crypto/tls.(*Conn).Write probe 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 to morestack, 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:

  • If the write ends inside a frame, as Go's 4 KB flushes usually do, the HTTP/2 parser loses frame alignment for the rest of the connection.
  • If it repeats a header block, that block's HPACK entries are inserted twice.

Stacks grow again after the GC shrinks them, so this recurs for the life of a process.

  • Probe placement: the write probe is now attached to the first instruction past the prologue's stack check (the branch to 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.
  • Why not return probes: the first version of this PR emitted the write from return probes. That needs a uprobe on every return instruction, 17 per Go binary instead of 9. Attaching is per process, so a burst of short-lived processes outran it: locally, 233 of 391 short-lived clients were captured, against 388 of 390 before. The second commit replaces it.

Stacked on #356.

Engineering detail
  • The prologue patterns come from Go 1.26 binaries:
    • amd64: CMPQ SP, 16(R14), or LEAQ -n(SP), R12; CMPQ R12, 16(R14), then JBE.
    • arm64: LDR X16, [X28,#16], an optional SUB, CMP, then B.LS.
  • TestStackCheckEnd covers 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.

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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread ebpftracer/ebpf/l7/gotls.c Outdated
Comment on lines +369 to +380
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");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Comment thread ebpftracer/ebpf/l7/gotls.c Outdated
Comment on lines +382 to +399
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;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Comment thread ebpftracer/ebpf/l7/gotls.c Outdated
Comment on lines +401 to +422
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);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@mayankpande88 mayankpande88 changed the title fix(tls): emit Go TLS writes once, at return fix(tls): send each Go TLS write once Oct 5, 2026
@mayankpande88

Copy link
Copy Markdown
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant