fix(learn): agent workload bug fixes by ghazariann · Pull Request #209 · multikernel/sandlock

ghazariann · 2026-09-06T20:53:02Z

Running a full learn-then-run cycle on claude -p "hello" exposed two bugs in sandlock learn.

Bug 1 - `write = ["/"]`

The generated profile granted write access to the entire filesystem:

write = ["/"]

Two root causes:

mkdirat("/root") fires pre-syscall during learn. The target already exists (EEXIST), but the handler still inserted the parent / into the write set. Fix: skip mkdirat/mknodat if the target already exists.
The existing-path fast-path in collapse_write_paths had no check at all - only the ancestor-walking branch had guards. A direct write to / would pass straight through. Fix: check for / explicitly in the direct-write branch and drop it with a warning. Protected/Guarded paths in the direct-write branch emit a notice but are still recorded.

Bug 2 - `/proc/self/maps` missing from reads

canonicalize_or_keep resolved /proc/self/maps to /proc/<pid>/maps, which is_junk_path then dropped as a pid-specific path. V8 reads /proc/self/maps on every startup - without it in the profile, Landlock denies the read and the process crashes.

Fix: don't resolve /proc/self paths; keep them as-is. Only junk volatile sub-trees (/proc/self/fd/, /proc/self/task/, etc.) and numeric-pid entries.

Repro

# Learn
./target/debug/sandlock learn --http-inject-ca /etc/ssl/certs/ca-certificates.crt -o profile.toml -- claude -p "hello"

# Run
./target/debug/sandlock run --profile-file profile.toml -- claude -p "hello"

congwang-mk

The two fixes look right and CI is green, but a few things to address before merge. Also: none of the three behavior changes here (existing-path tier check, mkdirat on an existing target, /proc/self surviving is_junk_path) has a test; learn_test.rs already covers the ancestor-walk branch, so adding cases next to those would be natural. The three commits also move /root guarded, then protected, and end with classify_path identical to main; please squash into logical commits with a body explaining why.

congwang-mk · 2026-09-08T01:58:49Z


+The tiers apply to **write collapse only** (when a non-existent path's nearest
+existing ancestor is used as the Landlock grant). Direct writes to an existing
+path — including Protected and Guarded paths — are recorded as-is without


This contradicts the code in this PR: the existing-path branch of collapse_write_paths now skips Protected and warns on Guarded, so direct writes are filtered. Please rewrite. Also drop the em-dashes; the project does not use them.

Now the doc is in sync with the PR.

congwang-mk · 2026-09-08T01:58:49Z

+            match classify_path(p) {
+                PathTier::Protected => {
+                    eprintln!(
+                        "sandlock learn: WARNING: observed a write of '{}' (protected path), skipping",


Per the PR body, /root landed in the write set because the workload itself renamed into it. Dropping it makes the generated profile unable to satisfy that rename, so sandlock run will fail on it, yet learn still exits 0. The docs table promises "skip + error" for this tier. Either exit non-zero or make the warning say the profile is known incomplete. What was the rename target in your repro, given you report the full cycle works?

I initially thought the profile was complete because sandlock run exited 0 and claude responded normally even without /root in the write list, but the rename of .claude.json.tmp → .claude.json was silently failing. Claude somehow swallowed the EACCES and continued. I confirmed this by checking .claude.json's mtime before and after the run: it didn't change.

After looking at this more closely, I removed all filtering from the direct-write branch (4f8f74d), only / is still dropped, since granting it subsumes everything else. After all, sandlock learn is an observer: if a legitimate workload wrote to /root, the user should see it and decide. Thoughts?

Agreed, this is the right call. My objection was that dropping /root produced an incomplete profile while learn still exited 0. Recording it removes the incompleteness and the NOTE gives the operator the visibility, which is better than either option I suggested.

One thing to keep in mind for later, not for this PR: mkdir /root/foo now grants /root with a NOTE, while open("/root/foo/bar", O_CREAT) with /root/foo missing goes through the ancestor walk and hits the Protected skip + error. Same effective grant, two outcomes. That inconsistency predates this PR, so a tracking issue is enough.

Tracking #217

congwang-mk · 2026-09-08T01:58:49Z

+    let proc_numeric_pid = b.starts_with(b"/proc/")
+        && b.get(6).map_or(false, u8::is_ascii_digit);
+    // Under /proc/self, only sub-trees with volatile numeric components are
+    // junk. Stable entries like /proc/self/maps legitimately needed across runs (e.g. V8 reads maps on every startup).


Does this grant cover child processes? Landlock is applied in the forked child before exec, so a rule on /proc/self/maps binds to that one pid's proc inode. Any subprocess the workload spawns has a different /proc//maps inode and would still be denied. Worth checking with a workload that forks a node child.

You are right. I tested with a simple program forking a child that reads /proc/self/maps: parent ok, child fails with Permission denied. The Landlock grant pins the top-level process's /proc/<pid>/maps inode; children get different inodes not covered by the rule.

One option during learn: when a /proc/self/ access is observed from a child process, widen the grant to /proc. Simple but broad. Another option: intercept these opens at run time via seccomp notif and handle them in the supervisor instead. Any thoughts ?

Thanks for confirming. Please go with the seccomp option, not the /proc widening: in non-chroot mode a /proc read grant exposes every same-uid process's /proc// to the workload, which is exactly what the Guarded tier exists to flag.

The seccomp route is smaller than it sounds because the hook already exists. procfs::handle_proc_open runs on every open-family syscall in every mode and already injects memfds for virtualized files. The extension would be: when the profile's read list contains /proc/self/X and the calling process opens /proc/self/X, /proc/thread-self/X, or /proc/<its own pid>/X, the supervisor opens /proc/<notif.pid>/X and injects that fd. That is scoped to the caller's own pid and to paths the policy explicitly lists, so it does not reopen the issue #27 concern about on-behalf opens bypassing Landlock.

That belongs in a follow-up PR: it changes what run enforces rather than what learn records, lives in a different crate, and needs its own test with a forking workload. For this PR, please add a one-line caveat to docs/learn.md that a /proc/self/* grant currently only covers the top-level process, or open a tracking issue and link it here.

Tracking #218

congwang-mk · 2026-09-08T01:58:49Z

+/// /proc/self/maps into /proc/<pid>/maps, making it look like a volatile
+/// pid-specific path and causing is_junk_path to drop it.
 fn canonicalize_or_keep(p: PathBuf) -> PathBuf {
+    if p.as_os_str().as_encoded_bytes().starts_with(b"/proc/self") {


Minor: this also leaves /proc/self/cwd/x and /proc/self/exe unresolved. Not a regression, since canonicalize previously resolved these against the supervisor's own /proc/self, which was the wrong process anyway. A cleaner fix would rewrite the prefix to /proc/<event.pid>/, canonicalize, then map back to /proc/self.

Done e13cd14

congwang-mk · 2026-09-09T22:01:31Z

@ghazariann Thanks for the update, please also do a rebase since there is a conflict.

mkdirat fires pre-syscall, so the handler ran even when the target already existed and the syscall would fail with EEXIST. The parent "/" ended up in the write set, which dedup_subsumed then collapsed every other write path under it, producing write = ["/"]. Fix: skip mkdirat/mknodat when the target already exists on the real filesystem. Add a "/" guard to the existing-path fast-path to match the reads side. Add a NOTE when a direct write lands on a Protected or Guarded path. Rewrite path tiers table; add paragraph on direct writes with the "/" exception bolded. Tests: add test_mkdirat_eexist_no_write and test_direct_write_root_skipped. Signed-off-by: Vahagn <wanghang25@mails.tsinghua.edu.cn>

…ntries

Pass event.pid to canonicalize_or_keep so /proc/self/X is rewritten to /proc/<pid>/X, canonicalized against the workload (not the supervisor), then mapped back to /proc/self/X. Symlinks like /proc/self/exe correctly resolve to the real binary path instead of staying as a proc alias. Signed-off-by: Vahagn <wanghang25@mails.tsinghua.edu.cn>

…havior Signed-off-by: Vahagn <wanghang25@mails.tsinghua.edu.cn>

congwang-mk requested changes Sep 8, 2026

View reviewed changes

ghazariann force-pushed the feat/agent-workload-fixes branch from fc73468 to ab30d58 Compare September 9, 2026 21:57

ghazariann added 2 commits September 10, 2026 02:01

[fix] learn: preserve /proc/self paths, junk only numeric-pid /proc e…

c731be3

…ntries

ghazariann force-pushed the feat/agent-workload-fixes branch from ab30d58 to 9857e4c Compare September 9, 2026 22:02

ghazariann force-pushed the feat/agent-workload-fixes branch from 9857e4c to e13cd14 Compare September 9, 2026 22:12

ghazariann requested a review from congwang-mk September 9, 2026 22:38

[docs] learn: rewrite path tiers paragraph to clarify direct-write be…

72e0d9f

…havior Signed-off-by: Vahagn <wanghang25@mails.tsinghua.edu.cn>

This was referenced Sep 9, 2026

learn: inconsistent behavior for write grants to Protected paths #217

Open

run: /proc/self/* grants only cover the top-level process, child processes are denied #218

Open

congwang-mk merged commit 5e4eb8d into multikernel:main Sep 10, 2026
17 checks passed

congwang-mk mentioned this pull request Sep 10, 2026

learn: follow-ups from the #209 review #220

Merged

Uh oh!

Conversation

ghazariann commented Sep 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug 1 - write = ["/"]

Bug 2 - /proc/self/maps missing from reads

Repro

Uh oh!

congwang-mk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghazariann Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghazariann Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

congwang-mk commented Sep 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ghazariann commented Sep 6, 2026 •

edited

Loading

Bug 1 - `write = ["/"]`

Bug 2 - `/proc/self/maps` missing from reads

ghazariann Sep 9, 2026 •

edited

Loading

ghazariann Sep 9, 2026 •

edited

Loading