sysupgrade: stop the ramfs pivot leaving debris in the overlay by widgetii · Pull Request #2280 · OpenIPC/firmware

widgetii · 2026-08-17T13:28:09Z

Reported by @usa- in OpenIPC/majestic-webui#120: cameras upgraded since the ramfs pivot landed come back carrying two entries nothing put there on purpose, on four SoCs at once.

/overlay/root/dev/null
/overlay/root/ram/

His guess in the report was right — "something is writing to /dev/null (like 2>/dev/null) while it has not yet been created or is no longer available."

`/dev/null`

A shell redirection creates its target. enter_ramfs moves /dev — the devtmpfs that supplied the node — out of the old root, and then keeps running there for four more commands. Every 2>/dev/null in that window therefore discards nothing; it writes a regular file named null into a directory the shipped rootfs leaves empty. That root is an overlay, so the file lands in the upper layer and outlives the upgrade.

The mode is the fingerprint. Root's umask on the camera is 0077, and the artifact is 0600 — exactly what open(O_CREAT, 0666) yields:

# ls -l /overlay/root/dev/null
-rw-------    1 root     root             0 Aug 16 09:59 /overlay/root/dev/null

/rom/dev ships no null node at all, so there is nothing for overlayfs to have copied up — the file is new.

Holding a descriptor open on the real node survives the mount move, the pivot and the exec, because a descriptor names the inode rather than the path. Reproduced both ways on real hardware with no flash writes, by moving /dev aside and putting it back:

inside the window	result
`2>/dev/null` (old)	creates `-rw-------` regular file
`2>&3` (new)	creates nothing, still discards

`/ram`

The mount point itself. The rootfs ships no /ram, so mkdir -p "$RAM_ROOT" writes it into the upper layer, and nothing ever removed it — not after a successful pivot, not after a failed one, and not after the early bail-outs, which leave a tmpfs mounted on it as well.

Reclaimed on each path now. After the pivot the old root sits at /mnt with the tmpfs already detached, so the second phase takes it back before the first flashcp write destroys the filesystem it lives on; the single enter_ramfs call site covers every bail-out. rmdir, never rm -rf — if the tmpfs is somehow still attached, refusing is the right answer.

Impact

Neither artifact is dangerous today: /init moves devtmpfs back over /dev early enough that nothing has been observed writing through the file. But it is a silent write to flash on every upgrade, and a trap for anyone who later adds a redirection to /init between its own pivot_root and its mount -o move /rom/dev /dev.

Verification

End to end on the lab hi3516av300, with the patched script and the artifacts cleared first:

2.6.08.15 -> 2.6.08.16, clean reboot
/overlay/root/dev/   empty
/overlay/root/ram    No such file or directory

test_sysupgrade.sh gains one behavioural check (a failed pivot reclaims its mount point) and seven source invariants covering the redirect window, ramfs_unwind, where fd 3 is opened, and rmdir-not-rm -rf. Both halves fail the suite when reverted:

FAIL failed pivot left RAM_ROOT behind
FAIL enter_ramfs names /dev/null after moving /dev away -- that CREATES the file

scr_version is bumped to 1.0.58 so self_update actually delivers this — it compares versions, so reusing 1.0.57 would never install on a running camera.

🤖 Generated with Claude Code

Cameras upgraded since the ramfs pivot landed come back carrying two files that nothing put there on purpose, reported on four SoCs at once: /overlay/root/dev/null /overlay/root/ram/ Both are enter_ramfs leaking into the root it is leaving. /dev/null is the interesting one. A shell redirection CREATES its target, and enter_ramfs moves /dev — the devtmpfs that supplied the node — out of the old root and then keeps running there for four more commands. So every `2>/dev/null` in that window discards nothing: it writes a regular file named `null` into a directory the shipped rootfs leaves empty. That root is an overlay, so the file lands in the upper layer and outlives the upgrade, shadowing the character device on every subsequent boot until /init moves devtmpfs back over it. Confirmed on an hi3516av300, and the mode is the fingerprint: root's umask is 0077 and the artifact is 0600, exactly what open(O_CREAT, 0666) yields. Holding a descriptor open on the real node instead survives the mount move, the pivot and the exec, because a descriptor names the inode rather than the path — the same window then leaves nothing behind. Reproduced both ways on hardware with no flash writes at all: old spelling creates the file, `2>&3` does not. /ram is the mount point itself. The rootfs ships no /ram, so `mkdir -p` writes it into the upper layer, and nothing ever removed it — not after a successful pivot, not after a failed one, and not after the early bail-outs that leave a tmpfs mounted on it too. Reclaim it on each: after the pivot the old root is at /mnt with the tmpfs already detached, and the single call site is the one place that means "no pivot happened". rmdir, never rm -rf: if the tmpfs is somehow still attached, refusing is the right answer. Neither artifact is dangerous today — devtmpfs masks the file early enough that nothing has been observed writing through it — but it is a silent write to flash on every upgrade, and a trap for anyone who later adds a redirection to /init between its own pivot and its /dev move. scr_version bumped so self_update actually delivers this to a running camera: it compares versions, so a fix that reuses 1.0.57 would never install. Verified end to end on the lab hi3516av300: 2.6.08.15 -> 2.6.08.16 with the patched script, clean reboot, /overlay/root/dev empty and /overlay/root/ram absent afterwards. Tests cover both halves, and fail when either is reverted. Reported-by: usa- <https://github.com/usa-> Ref: OpenIPC/majestic-webui#120 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

qodo-free-for-open-source-projects · 2026-08-17T13:29:59Z

PR Summary by Qodo

sysupgrade: prevent ramfs pivot from writing /dev/null and /ram into overlay

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Redirect stderr via pre-opened fd 3 to avoid creating a regular /dev/null during pivot.
• Add ramfs_discard to rmdir $RAM_ROOT (and detach only our tmpfs) on all exit paths.
• Extend sysupgrade test harness to assert both behaviours and enforce redirect invariants.

Diagram

graph TD
  A["sysupgrade (start)"] --> B["open fd3 (/dev/null)"] --> C["enter_ramfs (move mounts)"]
  C -- "success" --> D["ramfs phase (pivot+exec)"] --> E["rmdir /mnt$RAM_ROOT"] --> G["flash_and_reboot"]
  C -- "failure" --> F["ramfs_discard (umount+rmdir)"] --> G
  B -. "prevents /dev/null file" .-> H[("overlay upper layer")]
  E -. "removes /ram dir" .-> H
  F -. "removes /ram dir" .-> H

High-Level Assessment

The following are alternative approaches to this PR:

1. Redirect to $RAM_ROOT/dev/null by path during the window

➕ Avoids managing a dedicated fd across the script
➕ Keeps redirections visually similar to existing 2>/dev/null patterns
➖ Still path-based; correctness depends on $RAM_ROOT/dev being present and consistently addressed during pivot/exec
➖ More fragile across refactors (changing RAM_ROOT layout or pivot timing can reintroduce debris)

2. Ship a real /dev/null node in the underlying rootfs (/rom/dev/null)

➕ Would make 2>/dev/null resolve to a character device even after devtmpfs is moved away
➕ Fix applies to any scripts that accidentally redirect during the window
➖ Requires rootfs/build-system changes beyond sysupgrade, touching more platforms and release artifacts
➖ Doesn’t address the separate /ram mount-point directory leak; still needs cleanup logic

Recommendation: Keep the PR’s approach (pre-open fd 3 + use 2>&3 in the post-/dev-move window, plus explicit rmdir-based RAM_ROOT cleanup). It’s minimally invasive to the runtime environment, robust across mount moves/pivot/exec, and the added invariants in the test harness help prevent regressions in a particularly subtle failure window.

Files changed (2) +139 / -14

Bug fix (1) +82 / -14

sysupgradePrevent overlay debris during ramfs pivot and clean up RAM_ROOT reliably +82/-14

Prevent overlay debris during ramfs pivot and clean up RAM_ROOT reliably

• Bumps scr_version to 1.0.58 and opens fd 3 to the real /dev/null early, then switches pivot-window redirections from 2>/dev/null to 2>&3 to avoid creating /overlay/root/dev/null. Introduces ramfs_discard to detach only a tmpfs mounted at RAM_ROOT and rmdir the mount point, calls it when enter_ramfs fails, and removes the stranded /mnt$RAM_ROOT directory after a successful pivot before flashing begins.

general/overlay/usr/sbin/sysupgrade

Tests (1) +57 / -0

test_sysupgrade.shAdd behavioural + invariant checks for ramfs cleanup and safe redirections +57/-0

Add behavioural + invariant checks for ramfs cleanup and safe redirections

• Adds a behavioural test asserting that a failed ramfs pivot reclaims the RAM_ROOT mount point (via a stubbed rmdir). Extends source invariants to forbid path-based /dev/null redirections in the post-/dev-move window, require fd-3 usage, and enforce rmdir-only cleanup semantics for RAM_ROOT.

.github/scripts/test_sysupgrade.sh

qodo-free-for-open-source-projects · 2026-08-17T13:33:26Z

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Ramfs cleanup may fail 🐞 Bug ≡ Correctness

Description

The ramfs phase now runs rmdir "/mnt$RAM_ROOT", but if the script takes the existing “busybox
--list is a build option” fallback path, it does not create a rmdir applet symlink, so this
cleanup can fail silently and leave the mount-point debris behind.

Code

general/overlay/usr/sbin/sysupgrade[R1084-1086]

+	# /mnt to take it from: the first flashcp write is a few lines away and the
+	# old root does not survive it (OpenIPC/majestic-webui#120).
+	rmdir "/mnt$RAM_ROOT" 2>/dev/null

Evidence

The script explicitly supports a fallback where it cannot enumerate BusyBox applets via --list and
instead symlinks only a fixed set; that set omits rmdir. The PR adds a new rmdir "/mnt$RAM_ROOT"
call in the ramfs phase, which will therefore fail (quietly) in the fallback configuration,
defeating the intended cleanup.

general/overlay/usr/sbin/sysupgrade[622-627]
general/overlay/usr/sbin/sysupgrade[1078-1087]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The ramfs (second-phase) cleanup uses `rmdir "/mnt$RAM_ROOT"`, but in the code path where BusyBox `--list` is not available, the staged ramfs only gets symlinks for a fixed list of applets that currently omits `rmdir`. In that fallback scenario, `rmdir` may be `not found` (and the error is redirected away), so the old-root mount-point directory may not be reclaimed.
### Issue Context
`enter_ramfs()` explicitly supports a BusyBox configuration where `busybox --list` is unavailable and falls back to a hardcoded applet list; the new cleanup depends on `rmdir` being available in that staged `/bin`.
### Fix Focus Areas
- general/overlay/usr/sbin/sysupgrade[622-627]
- general/overlay/usr/sbin/sysupgrade[1078-1086]
### Suggested fix
Either:
1) Add `rmdir` to the fallback applet list that gets symlinked into `$RAM_ROOT/bin`, **or**
2) Call it via BusyBox directly in phase 2 (e.g. `/bin/busybox rmdir "/mnt$RAM_ROOT" ...`) so it does not rely on a symlinked applet name.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. ~~RAM_ROOT join missing slash~~ ✓ Resolved 🐞 Bug ≡ Correctness

Description

In the ramfs phase cleanup, rmdir "/mnt$RAM_ROOT" only targets the intended old-root path when
RAM_ROOT starts with /. Since RAM_ROOT is environment-overridable, a relative value (e.g.
RAM_ROOT=ram) produces /mntram instead of /mnt/ram, so the intended mount-point directory is
not reclaimed.

Code

general/overlay/usr/sbin/sysupgrade[1079]
+	rmdir "/mnt$RAM_ROOT" 2>/dev/null

Evidence
The script explicitly allows RAM_ROOT to be overridden via the environment, but the newly added
cleanup concatenates it directly to /mnt without inserting a separator when RAM_ROOT is
relative, producing an incorrect path.
general/overlay/usr/sbin/sysupgrade[10-28]
general/overlay/usr/sbin/sysupgrade[468-488]
general/overlay/usr/sbin/sysupgrade[1071-1080]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`rmdir "/mnt$RAM_ROOT"` assumes `RAM_ROOT` is an absolute path (leading `/`). But `RAM_ROOT` is environment-overridable, so a relative override constructs the wrong old-root path during the ramfs phase cleanup.
## Issue Context
- `RAM_ROOT` is set from the environment (`RAM_ROOT=${RAM_ROOT:-/ram}`), and the script itself documents that it is overridable.
- The PR adds a new cleanup in the `_ramfs_phase` path: `rmdir "/mnt$RAM_ROOT"`.
## Fix Focus Areas
- general/overlay/usr/sbin/sysupgrade[10-28]
- general/overlay/usr/sbin/sysupgrade[1071-1080]
- .github/scripts/test_sysupgrade.sh[941-946]
## Suggested fix
1. Normalize or validate `RAM_ROOT` early:
- Option A (normalize):

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Tip of the day

💡 Did you know, you can route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Raised in review. RAM_ROOT is overridable from the environment, and two places downstream quietly assume it starts with a slash: /proc/mounts records mount points absolutely, so the "is this tmpfs one of ours" check in ramfs_discard could never match a relative path; and the second phase reaches the same directory through the old root as "/mnt$RAM_ROOT", which for `ram` composes to "/mntram" and removes nothing. Neither is reachable today — the default is /ram and the test harness passes an absolute sandbox path — but both fail silently rather than loudly, which is the kind of thing that survives a refactor. Normalise once at the definition instead of teaching every use site to cope. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

widgetii · 2026-08-17T16:30:51Z

/review

qodo-free-for-open-source-projects · 2026-08-17T16:37:53Z

+	# /mnt to take it from: the first flashcp write is a few lines away and the
+	# old root does not survive it (OpenIPC/majestic-webui#120).
+	rmdir "/mnt$RAM_ROOT" 2>/dev/null


1. Ramfs cleanup may fail 🐞 Bug ≡ Correctness

The ramfs phase now runs rmdir "/mnt$RAM_ROOT", but if the script takes the existing “busybox --list is a build option” fallback path, it does not create a rmdir applet symlink, so this cleanup can fail silently and leave the mount-point debris behind.

Agent Prompt

### Issue description The ramfs (second-phase) cleanup uses `rmdir "/mnt$RAM_ROOT"`, but in the code path where BusyBox `--list` is not available, the staged ramfs only gets symlinks for a fixed list of applets that currently omits `rmdir`. In that fallback scenario, `rmdir` may be `not found` (and the error is redirected away), so the old-root mount-point directory may not be reclaimed. ### Issue Context `enter_ramfs()` explicitly supports a BusyBox configuration where `busybox --list` is unavailable and falls back to a hardcoded applet list; the new cleanup depends on `rmdir` being available in that staged `/bin`. ### Fix Focus Areas - general/overlay/usr/sbin/sysupgrade[622-627] - general/overlay/usr/sbin/sysupgrade[1078-1086] ### Suggested fix Either: 1) Add `rmdir` to the fallback applet list that gets symlinked into `$RAM_ROOT/bin`, **or** 2) Call it via BusyBox directly in phase 2 (e.g. `/bin/busybox rmdir "/mnt$RAM_ROOT" ...`) so it does not rely on a symlinked applet name.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

qodo-free-for-open-source-projects · 2026-08-17T16:37:54Z

Code review by qodo was updated up to the latest commit 874036c

Caught in review, and a real hole in the previous commit. The mount-point reclaim it added runs AFTER the pivot, where the staged applet symlinks are the only tools in existence — and `rmdir` was not among them. `rm` is on the fallback list; `rmdir` never was, because until now nothing after the pivot needed it. `busybox --list` populates the symlinks when it is available, which is the path every stock OpenIPC build takes (282 applets on the lab av300, rmdir included), so this would not have bitten there. But --list is a build option, and the hardcoded list is precisely the "camera without it" case — where the cleanup would have failed as "not found", swallowed by its own 2>/dev/null, and left the debris it exists to remove. Silently doing nothing is the worst shape for this bug, since the symptom is identical to not having the fix at all. The test now derives the requirement from the fallback list rather than trusting it, and fails if rmdir leaves it again. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

widgetii · 2026-08-17T16:45:41Z

Good catch on both — fixed in 874036cad and e54aa6946.

RAM_ROOT absolute. Normalised once at the definition rather than at each use site. Two things assumed the leading slash and both failed quietly: /proc/mounts records mount points absolutely, so ramfs_discard's "is this tmpfs ours" check could never match a relative path, and "/mnt$RAM_ROOT" composes ram into /mntram.

rmdir not staged. This one was a genuine hole in the fix, and the worst possible shape for it — the reclaim runs after the pivot, where the applet symlinks are the only tools that exist, and rmdir was never on the hardcoded fallback list (rm is; nothing after the pivot had needed rmdir before). It would have failed as "not found", been swallowed by its own 2>/dev/null, and left exactly the debris the PR removes. Indistinguishable from not having the fix.

Worth noting for scope: busybox --list populates the symlinks wherever it is available, which is every stock OpenIPC build — the lab av300 reports 282 applets with rmdir among them — so the hardware verification in the PR description exercised the --list path and would not have caught this. The fallback list is precisely the "build without --list" case.

Both now have test invariants, and both fail the suite when reverted:

FAIL the ramfs phase runs rmdir, but a busybox without --list would not have that applet

widgetii · 2026-08-17T16:51:22Z

/review

qodo-free-for-open-source-projects · 2026-08-17T16:55:32Z

Code review by qodo was updated up to the latest commit e54aa69

widgetii · 2026-08-17T16:57:15Z

The remaining Bugs (1) — Ramfs cleanup may fail on the review summary is stale: it is the same rmdir finding, carried forward rather than re-derived. It was fixed in e54aa6946, which is the commit the summary now points at.

At the exact sha and the exact lines the finding cites (sysupgrade[622-627] @ e54aa694):

	# --list is a build option; fall back to what the flash phase actually uses.
	for applet in sh ash awk basename cat cut dd flash_eraseall flashcp grep \
		head ln losetup ls mkdir mount od printf reboot rm rmdir sed sleep \
		sync tail timeout umount xxd; do

rmdir is there, so the premise ("that set omits rmdir") no longer holds. The item also lost its ⭐ New badge on the re-run, which is consistent with a carried-over entry rather than a fresh confirmation.

Guarded by a test either way, so a regression would be caught rather than argued about:

ok   the fallback applet list stages rmdir for the ramfs phase

widgetii requested review from cronyx, flyrouter and viktorxda as code owners August 17, 2026 13:28

widgetii mentioned this pull request Aug 17, 2026

fw-update: say what the log is doing when the camera goes quiet OpenIPC/majestic-webui#156

Merged

widgetii mentioned this pull request Aug 17, 2026

Firmware update fails or progress gets stuck OpenIPC/majestic-webui#120

Open

qodo-free-for-open-source-projects Bot reviewed Aug 17, 2026

View reviewed changes

widgetii merged commit b40881e into master Aug 17, 2026
101 of 102 checks passed

widgetii mentioned this pull request Aug 17, 2026

Firmware → Settings → Reset firmware: upgrade-like issues OpenIPC/majestic-webui#154

Open

Navigation Menu

Uh oh!

Uh oh!

sysupgrade: stop the ramfs pivot leaving debris in the overlay - #2280

sysupgrade: stop the ramfs pivot leaving debris in the overlay#2280
widgetii merged 3 commits into
masterfrom
fix/ramfs-overlay-debris

widgetii commented Aug 17, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 17, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 17, 2026 •

edited

Loading

Uh oh!

widgetii commented Aug 17, 2026

Uh oh!

qodo-free-for-open-source-projects Bot Aug 17, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 17, 2026

Uh oh!

widgetii commented Aug 17, 2026

Uh oh!

widgetii commented Aug 17, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 17, 2026

Uh oh!

widgetii commented Aug 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

widgetii commented Aug 17, 2026

/dev/null

/ram

Impact

Verification

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 17, 2026

PR Summary by Qodo

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

widgetii commented Aug 17, 2026

Uh oh!

qodo-free-for-open-source-projects Bot Aug 17, 2026

Choose a reason for hiding this comment

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 17, 2026

Uh oh!

widgetii commented Aug 17, 2026

Uh oh!

widgetii commented Aug 17, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 17, 2026

Uh oh!

widgetii commented Aug 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

`/dev/null`

`/ram`

qodo-free-for-open-source-projects Bot commented Aug 17, 2026 •

edited

Loading