Skip to content

feat(memfill): join target cgroup without cgexec - #478

Closed
achoimet wants to merge 1 commit into
mainfrom
feat/memfill-join-cgroup-without-cgexec
Closed

feat(memfill): join target cgroup without cgexec#478
achoimet wants to merge 1 commit into
mainfrom
feat/memfill-join-cgroup-without-cgexec

Conversation

@achoimet

Copy link
Copy Markdown
Member

Why

memfill placed its process into the target's memory cgroup with cgexec -g memory:<path>, which depends on libcgroup-tools (cgroup-tools on deb, libcgroup-tools on rpm). That package:

  • does not exist for Enterprise Linux 9 — Red Hat removed libcgroup in RHEL 9 (solution 7115132) and it was never packaged for EPEL 9. So steadybit-extension-host and steadybit-extension-container cannot be installed on RHEL 9 / Rocky 9 / Alma 9 (nothing provides /usr/bin/cgexec).
  • predates cgroup v2 (the shipped libcgroup is 0.41), so even where it installs, cgexec -g memory: does not do the right thing on the unified hierarchy that RHEL 9 / Ubuntu 22+ use by default.

This is the long-term fix for the packaging problem worked around in extension-host#240 and setup-scripts#71.

What

Join the cgroup directly instead of shelling out to cgexec. A tiny sh wrapper writes its own PID to <cgroup>/cgroup.procs and then execs memfill:

  • cgroup v1 (memory controller) is preferred, v2 unified is the fallback — matching the historical memory:<path> semantics while adding v2 support.
  • The move happens before exec: under cgroup v2 a process keeps memory already charged to it when migrated, so joining first (while nothing is allocated yet) and then execing keeps the same PID and charges all of memfill's allocation to the target cgroup.
  • The cgroup path is passed as a positional argument, not interpolated into the script, so it cannot break out of the shell.

The public API (NewMemfillProcess) is unchanged, so extension-host and extension-container pick this up on their next action-kit bump with no code changes.

Verification

  • Unit test asserts the exact command vector and that cgexec no longer appears.
  • Validated the join wrapper in a cgroup v2 container: the exec'd process reports 0::/memfilltest and appears in the target's cgroup.procs — i.e. memfill runs inside the target cgroup, move-before-exec confirmed.

Follow-up (separate PRs, after this is released)

Once a released action-kit carries this, both extensions can drop the cgexec dependency from their .goreleaser.yaml entirely (rpm Requires/Recommends and deb Depends), which also restores the fill-memory attack on EL9 / cgroup-v2 hosts.

memfill placed its process into the target's memory cgroup via
'cgexec -g memory:<path>', which depends on libcgroup-tools
(cgroup-tools / libcgroup-tools). That package does not exist for
Enterprise Linux 9 and never supported cgroup v2, so the extensions
using memfill (extension-host, extension-container) could not be
installed on RHEL 9 / Rocky 9 / Alma 9, and fill memory could not
charge the right cgroup on cgroup-v2 hosts.

Join the cgroup directly instead: a tiny sh wrapper writes its own PID
to <cgroup>/cgroup.procs (cgroup v1 memory controller preferred, v2
unified fallback) and then execs memfill. The move happens before exec
so nothing is allocated in the wrong cgroup under v2.

This removes the cgexec runtime dependency entirely, letting the rpm
packages drop it from both Requires and Recommends.
@sonarqubecloud

Copy link
Copy Markdown

@claude

claude Bot commented Aug 14, 2026

Copy link
Copy Markdown

Claude finished @achoimet's task in 2m 52s —— View job


Claude's Review

  • Check previously unresolved review threads (none found)
  • Gather context (diff, files, related code)
  • Review code quality, bugs, security, concurrency
  • Post findings

Note: I wasn't able to run go build/go test in this sandbox (shell commands required approval that wasn't available), so this review is based on static reading of the code and cross-referencing the unit test's expected argument vector.

Summary

This is a well-scoped, well-verified change. The sh wrapper logic in go/action_kit_commons/memfill/memfill_process.go correctly:

  • Takes the cgroup path as a positional arg ($1) rather than interpolating it into the script text, so it can't be used for shell injection.
  • Prefers cgroup v1 (/sys/fs/cgroup/memory<path>) and falls back to v2 unified (/sys/fs/cgroup<path>), matching the documented intent.
  • Writes $$ to cgroup.procs before exec "$@", preserving the same PID across the exec chain (shnsentermemfill), which is required for cgroup v2's "keep already-charged memory" semantics described in the PR body.
  • Traced through the full nsenter/sh argument chain by hand (cg="$1"; shift then exec "$@") — it matches exactly what TestMemfillCommandArgs asserts, and no cgexec remains in actual code paths (only in a doc comment describing the old behavior and in the test's negative assertion).

No NewMemfillProcess/memfillCommandArgs callers exist elsewhere in this repo, consistent with the PR's claim that the public API is unchanged.

Minor, non-blocking observations

  • There's a small TOCTOU window between the [ -e ".../cgroup.procs" ] check and the printf > "$procs" write (e.g., if the target cgroup is removed in between). This is handled gracefully — the write failure is caught and reported via the existing || fallback — so it's not a correctness issue, just worth knowing about.
  • Cgroup-path resolution itself (ociruntime.readCgroupPath/parseProcCgroupFile) is unchanged by this PR, so the existing behavior of picking the lowest-hid v1 hierarchy (not necessarily the memory controller specifically) carries over as-is — not introduced by this change.

No bugs, security issues, or concurrency problems found in the diff.

@joshiste

Copy link
Copy Markdown
Member

Superseded by #479 — closing in favour of that one.

To be clear about what's being kept: your diagnosis is what drove the whole change. The EL9 problem (libcgroup-tools has no Enterprise Linux 9 build, and cgexec never supported cgroup v2, so the extensions can't be installed on RHEL/Rocky/Alma 9) was identified here first, and #479 exists because of it.

Two things from this PR carried over directly:

  • Probing which cgroup.procs actually exists — v1 memory controller first, v2 unified as fallback — rather than inferring the root from a cgroup-version check. feat(memfill): let memfill join the target cgroup and PID namespace #479 originally inferred it; your approach is more robust because it agrees with the caller, which resolved the path from /proc/<pid>/cgroup and prefers the v1 line. That's now what memfill does internally.
  • The reasoning that the move must happen before anything is allocated, since under v2 a process keeps memory already charged to it when migrated.

The difference is where the work lives. This PR keeps the second nsenter -t <pid> -p -F and does the cgroup join from an sh -c wrapper; #479 pushes both into memfill itself (v1.5.0, via --target-cgroup-path / --target-pid), so both inner hops go away and the argv collapses to nsenter -t 1 -C -- memfill --target-....

The honest trade-off: this PR's real advantage is that it works against the memfill binaries that already exist, with no release coordination. #479 needed a memfill release first — that's now done, so the coordination cost is paid.

Follow-ups that depend on it: steadybit/extension-host#245 and steadybit/extension-container#492, which drop cgroup-tools / /usr/bin/cgexec from the deb, rpm and container images — the packaging cleanup this PR was aiming at.

If you'd rather land this one instead, happy to reopen and close #479 — say the word.

@joshiste joshiste closed this Aug 17, 2026
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 17, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants