erofs: share one inode per hardlink, and fix the stale mode-bit comments

[smoser]

Two loose ends from #2408, both falling out of the go-erofs bump in
#2412. Rebased onto main now that #2418 (multi-layer splitting) has
landed; the two overlap only in one docs/erofs.md bullet, merged
here.

1. The mode-bit comments are stale

pkg/build/erofs.go described the Chmod after Mkdir/Mknod/
Create as a workaround for the pinned go-erofs, to revisit "once a
release containing erofs/go-erofs#41 is out". #2412 bumped the pin
past that merge, so both halves of the claim are now wrong. Probing
the pinned version:

• Mkdir with setuid/setgid/sticky: claimed to drop them, actually
  keeps them.
• FileInfo.Mode() on read: claimed untrustworthy for those bits,
  actually reports them.

The Chmod still has to stay, for a reason that has nothing to
do with #41 and is not going away: Writer.Create takes no mode
argument at all, so every regular file starts life 0644. Mkdir
and Mknod do take one, but apko hands them mode.Perm() and lets
the single Chmod cover all three rather than splitting the rule
across three call sites. So this is a comment change, not a code
change.

pkg/erofsmount/ls.go keeps reading *erofs.Stat off Sys() --
fs.FileInfo has nowhere to put a uid or a device number -- but its
comment justified that with the setuid claim, which no longer holds.

TestWriteErofs_SpecialModeBits now also asserts FileInfo.Mode() == Stat.Mode for every case it covers (setuid and setgid regular files,
a sticky directory, a char device, a symlink), which pins the half
that changed.

2. Hardlinks point at one inode

go-erofs grew Writer.Link(oldname, newname), which gives a second
name the same fsInode as the first and maintains nlink. apko was
still materializing every hardlink as an independent copy, because
when #2249 landed there was no API for it -- SetNlink sets the
reported count without sharing the inode, so it would only have made
the metadata lie.

A hardlink reaches the writer as an ordinary second dirent; its
linkness lives only in the *tar.Header from Sys(), which
pkg/tarfs fills in from the apk it unpacked. hardlinkTarget reads
it from there -- the same place the tar layer path finds it via
tar.FileInfoHeader. A rootfs read back off disk (apkfs.MemFS,
rwosfs) records nothing, so those keep getting a copy per name,
exactly as before.

Four details worth review:

• Links are held back until the walk finishes. Writer.Link
  needs the target to already exist and fs.WalkDir is
  lexicographic, so /usr/bin/[ arrives long before
  /usr/bin/coreutils.

• A link whose target the writer cannot find under the Linkname it
  was handed falls back to a copy. Spec §3.7 leaves
  materialize-or-fail to the producer and apko materializes. This is
  not a cross-layer case: the erofs: restore multi-layer splitting, with the bugs review found fixed #2418 split walk never calls into
  this path, so a layered build materializes every hardlink in every
  layer regardless. What reaches the fallback in a single-layer
  build: a target a paths directive removed, a Linkname routing
  through a symlinked directory component (the writer's lookup is a
  flat path map and follows nothing, while tarfs resolved the name at
  unpack), and a chain of links, whose middle name is deferred rather
  than emitted and so may not exist yet when the last one is linked.

• A directory target aborts the build, deliberately. Writer.Link
  returns ErrIsDirectory, and pkg/tarfs's link() has no type
  check on the target, so a crafted apk gets that far. link(2)
  refuses a directory hardlink too, and the copy fallback would
  duplicate the whole subtree.

• A link name binds to the target path in the image, not to the
  node the rootfs resolved. A Linkname landing on a symlink shares
  the symlink; a target a later package replaced binds to the
  replacement. That is what link(2) does when the same rootfs is
  replayed from a tar layer, so the two layer formats agree -- the
  independent copies apko used to write were the outlier. A Linkname
  that still holds .. after the Clean is rejected outright and
  written from fsys instead; nothing downstream catches it
  (go-erofs's cleanPath re-roots /../etc/passwd to /etc/passwd,
  and checkPath only rejects duplicates).

Nothing is re-applied to a shared inode -- mode, ownership,
timestamps and xattrs came with it, and the tests assert mode, uid,
gid and an xattr through every link name.

Verification

gofmt -l clean, golangci-lint run reports 0 issues, go build ./... and GOOS=darwin go build ./... both succeed,
SOURCE_DATE_EPOCH=0 go test ./... passes. git rebase --exec
confirms every commit in the series builds and vets on its own.

The behavioural tests were checked to fail without their fix. Making
hardlinkTarget always return false:

Error: Not equal:            Messages: link count
Error: "196608" is not less than "32768"
       three hardlinks grew the image by 196608 bytes, which looks
       like copied data

196608 is exactly three more copies of the 64K fixture file. With the
change the three names share one inode, report nlink 3, and
fsck.erofs accepts the image.

Likewise: reverting the ErrNotDirectory arm fails
TestWriteErofs_HardlinkThroughSymlinkedDirIsCopied with link ...: not a directory; dropping the .. rejection fails
TestHardlinkTarget; and adding ErrIsDirectory to the fallback gate
fails TestEmitErofsHardlinks_DirectoryTargetAborts.

Refs #2408

🤖 Generated with Claude Code

[mattmoor]

Comments inline. What already checks out, verified against the pinned go-erofs source so the threads can skip it: Create takes no mode (files start 0644, so the Chmod stays), Mkdir/Mknod honor setuid/setgid/sticky since the #2412 bump, Link shares the fsInode and maintains nlink while SetNlink only overrides the count, and the mode-bit comment commit is accurate throughout — including the ls.go half. The happy-path tests genuinely pin inode sharing, nlink, and the one-copy size bound. The inline comments are: one real bug in the fallback gate, two semantics questions (probed against tarfs + the writer), and comment/docs drift. The cross-PR items were probed on a merged tree with #2418.

[mattmoor]

This gate misses go-erofs's ErrNotDirectory sentinel. The writer's resolveEntry is a flat byPath lookup — no symlink following — and classifyMiss returns ErrNotDirectory, not fs.ErrNotExist, when the Linkname routes through a symlink component. Concretely: rootfs with usr/lib/libfoo, symlink lib -> usr/lib (wolfi-baselayout ships bin/lib/lib64/sbin aliases in every image), and a TypeLink with Linkname lib/libfoo. tarfs resolves that fine at unpack (getNode follows symlinks) and the tar layer path works at runtime (link(2) follows intermediate symlink dirs), but this now fails the whole build with link …: not a directory where the old code wrote a working copy. Probed both halves: fsys reads all three names; writeErofs hard-fails; errors.Is(err, erofs.ErrNotDirectory) is true. A scan of 294 wolfi + 8 alpine apks found no such Linkname in the wild (melange records first-seen literal paths), so it's crafted-shaped — but apks are third-party input and the failure is a build abort on input the design says should degrade to a copy. || errors.Is(err, erofs.ErrNotDirectory) restores the fallback, and it can't mask a real error: the copy comes from fsys, which unpack already validated.

[smoser]

@r1.md

[smoser]

Confirmed, and fixed in df761bdd — the gate is now
fs.ErrNotExist || erofs.ErrNotDirectory.

Your reproduction is what the test builds:
TestWriteErofs_HardlinkThroughSymlinkedDirIsCopied lays down
usr/lib/libfoo.so, a lib -> usr/lib symlink, and a TypeLink at
usr/bin/libfoo.so with Linkname lib/libfoo.so. It fails without the
ErrNotDirectory arm with link ...: not a directory, and with it the link
is materialized as a copy (asserted by inode inequality against
usr/lib/libfoo.so).

Agreed it can't mask a real error: the fallback re-reads from fsys, which
unpack already validated.

One thing your comment prompted that went further than the fix: the gate was
still silently exhaustive-looking, and it isn't — see the ErrIsDirectory
thread below, now handled in 4f211eff.

[mattmoor]

Two problems with this paragraph. First, "layer splitting (#2418) is what puts a link and its target in different writers" doesn't hold: #2418's split walk never calls hardlinkTarget (nothing routes through this function after it lands — thread on that PR), and even once it's ported, tarfs routes a TypeLink name by the target's node — WriteHeader drops the pkg argument for TypeLink, and Package() comes off the shared node — so link and target structurally land in the same writer. The reachable triggers are a Linkname whose target was removed (paths directives), and chained links. Second, chained links also falsify "Within a single image it cannot happen": a Linkname naming another link was deferred, not emitted, so it falls back to a copy iff it sorts before its target — probed: A→B→C yields nlink 2/2/1 where fsys has one inode, nlink 3. That shape is crafted-only (melange/GNU tar always point Linkname at the first-seen regular name) and the silent copy is the forgiving outcome — the tar path fails at extraction on the same input — so I'd fix the wording here, in the PR body's §3.7 framing, and in the mirrored sentence on TestEmitErofsHardlinks_MaterializesWhenTargetIsAbsent; chasing targets through the pending-links slice is optional polish.

[smoser]

@r2.md

[smoser]

You're right on both counts, and the wording is corrected in df761bdd — the
code comment, the PR body's §3.7 framing, and the mirrored sentence on
TestEmitErofsHardlinks_MaterializesWhenTargetIsAbsent. The comment now names
the reachable triggers (a target a paths directive removed, a Linkname
routing through a symlinked directory component, and a chain of links whose
middle name is deferred) instead of claiming a cross-layer cause.

The layering half is now settled rather than predicted: #2418 has landed and
this branch is rebased onto it. I checked the merged tree —
pkg/build/erofs_layers.go calls emitErofsEntry at all four of its emit
sites and never calls hardlinkTarget/emitErofsHardlinks — so the split
walk doesn't route through this function at all, exactly as you said, and a
layered build materializes every hardlink in every layer. docs/erofs.md now
says that outright.

I left the pending-links chase alone, since you called it optional polish.

[mattmoor]

There is no writer-side path check that catches this: go-erofs cleanPath silently re-roots escapes (probed: Linkname ../etc/passwd → Link("/../etc/passwd") → links /etc/passwd), and checkPath only rejects duplicates. The actual backstop is that tarfs unpack fails on .. Linknames before writeErofs ever runs. Reword — or reject targets that still contain .. after the Clean — so the documented mechanism matches what runs; as written it invites a non-tarfs FullFS caller to rely on a check that isn't there.

[smoser]

@r3.md

[smoser]

Took your second option — rejecting rather than rewording alone — since the
"documented mechanism that isn't there" was the actual problem.

hardlinkTarget now returns false for a target still holding .. after the
path.Clean, and its comment says such a target is "rejected here and nowhere
else", naming both reasons: cleanPath silently re-roots escapes, and
checkPath only rejects duplicates. So a non-tarfs FullFS caller no longer
has an absent check to lean on.

The table in TestHardlinkTarget pins the boundary, including the case that
is not an escape:

• /../etc/passwd → etc/passwd, accepted — rooted, so the Clean absorbs the
  .. the way the kernel would
• ../etc/passwd, usr/bin/../../../etc/passwd, .. → rejected

Both escaping rows were checked to fail without the guard.

[mattmoor]

Writer.Link only rejects directory targets, so two shapes silently bind the link name to whatever inode sits at the target path in the writer, diverging from the fsys being serialized: (a) a Linkname landing on a symlink — tarfs resolves the final component at unpack, so fsys reports a regular file with the resolved content, but the image gets a shared symlink, dangling whenever its target is relative and the link name lives elsewhere (probed: other/mysh → usr/bin/sh → busybox; ReadFile works on fsys, ENOENT on the image); (b) a package replacing a hardlinked target — tarfs keeps the old node on the link dirent, but Link binds the name to the replacement inode (probed: fsys reads "A", image reads "B"). Both match what the tar layer path produces at runtime via link(2) — the pre-#2422 copies were the outlier — so if converging on tar/runtime semantics is the intent, a comment and a pinning test would nail that down; copying instead would preserve fsys fidelity at the cost of a new erofs-vs-tar divergence.

[smoser]

@r4.md

[smoser]

Converging on tar/link(2) semantics is the intent, so I took the
comment-and-pinning-test option rather than copying.

emitErofsHardlinks' doc comment now states the rule directly: the new name
binds to whatever the target path holds in the image being written, not to the
node fsys resolved, which is what link(2) does when the same rootfs is
replayed from a tar layer — and the independent copies apko wrote before were
the outlier.

Your shape (a) is pinned by
TestWriteErofs_HardlinkOntoSymlinkSharesTheSymlink, built as you probed it:
usr/bin/busybox, a relative usr/bin/sh -> busybox symlink, and a
TypeLink at opt/mysh naming usr/bin/sh. It asserts that fsys reads
regular-file content through opt/mysh, that the image has a symlink there
sharing usr/bin/sh's inode with nlink 2, and that reading it through the
image errors — "the shared symlink dangles, exactly as link(2) would leave
it". So the divergence is now deliberate and load-bearing rather than
incidental.

I didn't add a separate fixture for shape (b); it's the same binding rule with
a different final inode, and the comment covers it.

[mattmoor]

"mode, ownership, timestamps and xattrs came with it" is pinned only for mode perms. Cheap to give hardlinkFS's target a uid/gid and an xattr and assert them through the link names.

[smoser]

@r5.md

[smoser]

Done in df761bdd. hardlinkFS's target now carries a uid, a gid and an
xattr (1234/5678, user.apko.test), set via a SCHILY.xattr. PAX record
plus an explicit Chown — WriteHeader doesn't carry uid/gid onto the node,
apk chowns separately, which cost me a debugging round.

TestWriteErofs_HardlinksShareOneInode now asserts mode, uid, gid and the
xattr through every name. All four differ from what the writer would produce
on its own (0644, uid 0, gid 0, no xattrs), so the assertions fail if a name
ever stops sharing the inode.

[mattmoor]

"Names pointing at one inode in the source rootfs point at one inode in the image" holds for the single-layer path only, and the exceptions in the code threads cut against "is written as an independent copy instead" (a symlink-routed Linkname currently fails the build; a symlink target becomes a shared symlink; a chained link may copy). Also a heads-up: this bullet is where #2418 conflicts — whichever PR rebases second needs merged wording along the lines of "inodes are shared in single-layer builds; a layered build currently materializes every hardlink as a copy".

[smoser]

@r6.md

[smoser]

Thanks for the heads-up — it played out exactly as you predicted. #2418 landed
first, so this PR rebased second and owns the merged wording. The bullet is
now:

Hardlinks are shared in single-layer builds. [...] A layered build
materializes every hardlink as a copy, in every layer.

which I verified against the merged tree rather than assuming (see the
cross-layer thread: erofs_layers.go never calls into this path). I also took
upstream's overlay-data/device roles line, since #2418 made mine stale.

The exceptions list is reworded too, and your newer comment on it is handled
in 4f211eff.

[mattmoor]

One meta item first: the six review replies here are literal placeholders — each body is just @r1.md … @r6.md. Looks like the reply tooling posted the filenames instead of their contents, so the threads carry no readable disposition for anyone following along. Worth reposting the real text (the commit speaks for itself, but the threads don't).

ccedcfb verified by re-running the probes from the review: the merged-usr shape (lib -> usr/lib, Linkname lib/libfoo) now degrades to a full-content copy with the right mode where it previously aborted the build, and the new regression test drives it through writeErofs; hardlinkTarget now rejects ..-after-Clean with all four shapes in the test table (and the cleanPath re-rooting claim checks out against the pinned go-erofs); the reworded fallback comment names the real triggers and drops the cross-layer claim, with the mirrored test comment fixed; the shared-symlink binding is declared and pinned including nlink and the dangling read; and uid/gid plus the xattr are asserted through every link name. The copy fallback also behaves end-to-end for the one real-build-reachable trigger — install, paths-style Remove of the target, then writeErofs — producing the full original bytes, mode, and mtime under the link name. Two nits inline.

Found while probing, pre-existing and untouched by this PR (separate-issue material): tarfs link() accepts a Linkname of /, which shares the root dirent and sends fs.WalkDir into a cycle before any hardlink logic runs.

[mattmoor]

One shape still aborts, and the comment reads as if ErrNotExist/ErrNotDirectory were exhaustive: tarfs's link() has no dir-target check, so a crafted apk whose Linkname resolves to a directory unpacks fine and Writer.Link returns ErrIsDirectory here (probed: link dirlink -> target: link /target: is a directory). Failing closed looks right — runtime link(2) refuses directory hardlinks too, and the pre-PR behavior of silently duplicating the directory tree was worse — but one line marking the abort deliberate would keep it from reading as an oversight.

[smoser]

Verified both halves independently before touching it: Writer.Link returns
ErrIsDirectory for a directory target (mkfs.go:730, after resolveEntry
succeeds), and pkg/tarfs's link() does no type check on the target at all
(fs.go:684-706 — it only checks the parent, getNode, and duplicate
children), so a crafted apk really does get that far.

Marked deliberate in 4f211eff, and I added
TestEmitErofsHardlinks_DirectoryTargetAborts so the claim is pinned rather
than just asserted in a comment: it puts a directory at the target path and
requires errors.Is(err, erofs.ErrIsDirectory). Mutation-checked — adding
ErrIsDirectory to the fallback gate makes it fail with "An error is expected
but got nil".

Agreed on the reasoning: link(2) refuses a directory hardlink too, and the
old behavior of duplicating the subtree was worse.

[mattmoor]

Wording nit: "not in the image at all" slightly overstates the symlinked-directory case — that inode is in the image under its real path; only the literal-Linkname lookup misses it. The code comment states this precisely; fine to mirror that here.

[smoser]

Fair — fixed in 4f211eff. That case is a lookup miss, not an absence. It now
reads "A link whose target the writer cannot find under the Linkname it was
handed", with the reason inline for the symlinked-directory row ("which its
flat path lookup does not follow"), mirroring the code comment.

[mattmoor]

Re-approving after the force-push to 4f211ef dismissed the earlier approval.

Verified the new head locally (worktree at 4f211ef, rebased on main with #2418/#2426 in):

• Both round-2 nits are addressed: the ErrIsDirectory abort is now marked deliberate with TestEmitErofsHardlinks_DirectoryTargetAborts pinning it against being added to the fallback gate, and the docs bullet now reads as a lookup miss rather than an absence.
• The inode-sharing commit (c5e1d519 + df761bdd) folds in all of the round-1 findings: the fallback gate is fs.ErrNotExist || erofs.ErrNotDirectory, hardlinkTarget rejects targets still holding .. after the Clean, the link-binds-to-image-node semantics are stated in the code and docs and pinned by TestWriteErofs_HardlinkOntoSymlinkSharesTheSymlink, and the parity test asserts mode, uid, gid and the xattr through every name.
• Full erofs/hardlink test suite green at this head locally, including the seven new tests.

The StepSecurity Harden-Runner failure is the app's own network-anomaly annotation — this diff touches no workflows or dependencies, the check isn't present on the sibling PRs' heads, and every real CI leg (EROFS, Go Tests, ci, verify, CodeQL, Build Images) passed at 4f211ef — so I'm not treating it as blocking.