feat(factory-reset): implement mode 1 (backup / reformat / restore)#16
Conversation
Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>
Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>
Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>
Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>
Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>
Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>
Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>
Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>
- Add Path to imports (alongside PathBuf) - Collapse nested if-let chains with && per clippy::collapsible_if - Replace magic number 1 with SUPPORTED_RESET_MODE constant Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>
Add BootMode::FactoryReset variant and implement detect() logic to: - Read factory-reset key from bootloader environment - Parse JSON config (mode, preserve list) - Gracefully fall back to Normal on invalid JSON or env read errors - Add comprehensive test coverage for all detect() paths Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>
Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>
Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>
- Replace unreachable RestoreFailed match arm with explanatory comment; restore_all() accumulates path failures as PartialFailure, never propagates RestoreFailed via ? - Map MountError/ReformatFailed/Io to Error status code (not ConfigError); ConfigError is now reserved for MissingField only - Add spec comment explaining the infinite-reset-loop trade-off on persistent set_env failure - Add comment explaining why factory partition is re-mounted during restore phase Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>
Mount /boot with gid=6,fmask=0002,dmask=0002,allow_utime=0020 so the disk group can write the bootloader environment file (grubenv/uboot-env). Previously mounted with default fmask/dmask=0022, which prevented group-write access. Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>
…lidation Fully implements Factory Reset Mode 1, including selective-preserve backup, reformatting of `data` and `etc` partitions, and restore. This change also: - Updates the VFAT boot partition mount to dynamically resolve the `disk` group GID from `/etc/group` for improved compatibility and robustness. - Introduces validation for factory reset preserve list paths, preventing directory traversal vulnerabilities. - Refines the mount and unmount sequence during the reset process for better error handling and adherence to the factory reset specification (e.g., omitting the factory partition during the restore phase). - Updates `README.md` with the new feature description, a detailed flowchart, and feature status.
Keeps the startup-flow diagram's color coding reserved for terminal outcomes (success/reboot/halt/shell), matching every other process step which uses the default node style. Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>
Resolves ambiguity where the `FRESET` identifier was used for both the entry point to the factory reset process and for the subgraph representing it. The entry point is now `FCLEAR`. Also, clarifies the flow out of the factory reset process, connecting `FSTATUS` (the final status update) to `MREM` for improved accuracy.
JanZachmann
left a comment
JanZachmann
left a comment
There was a problem hiding this comment.
Automated multi-aspect review (factory-reset mode 1)
Review across correctness, silent-failure, error handling, comments, tests, type design, and spec conformance. Inline comments follow, tagged [Critical] / [Important] / [Suggestion].
Spec conformance: the implementation is faithful to 2026-06-29-factory-reset-mode1-design.md. Note two of the critical findings below (brick-on-partial-reformat, lost factory /etc defaults) are also latent in the spec itself — fixing them needs a spec update, not just code.
Type design (not inline — FactoryResetStatus lines are outside the diff): FactoryResetStatus (src/runtime/omnect_device_service.rs) has pub fields with no enforced relationship between status and error/context/paths, so illegal combinations (Success + error: Some, Error + error: None) are representable. Consider an enum payload per outcome with a flat Serialize impl, or at least document that the producer is the sole guarantor of consistency. FactoryResetStatusCode, RestoreResult, and IncludeFactory are exemplary — no change.
Scope: commit 6f2b1c0 (vfat boot-mount group-write options) is unrelated to factory reset and not in the plan — flag for reviewer awareness.
Strengths: faithful spec conformance, path-traversal hardening beyond spec (with tests), ContinueDegraded classification, crate:: imports, named constants, no bare unwrap() in production paths, strong leaf-level test coverage, good "why"-focused comments.
| })?; | ||
|
|
||
| reformat_ext4(data_dev, "data")?; | ||
| reformat_ext4(etc_dev, "etc")?; |
There was a problem hiding this comment.
[Critical] A partial reformat can brick the device — violates "a failed reset must never block boot".
If reformat_ext4(data) succeeds but reformat_ext4(etc) fails partway, run_reset returns Err → classified ContinueDegraded → run() calls normal::run. Normal boot then runs a strict mount() on the half-mkfs'd etc partition → FilesystemError::MountFailed → normal.rs:57 propagates via ? → recovery_class maps it to Fatal (error.rs:65) → infinite loop on a release image. The data partition is already wiped and the tmpfs backup is discarded at switch_root: total preserved-data loss plus a brick — the exact scenario the design forbids.
The factory-reset feature owns the destructive window. After the first reformat starts, a mount/overlay failure must be absorbed into ContinueDegraded: verify the reformatted partitions mount before handing off to normal::run, or re-mkfs to a known-good empty state and continue.
There was a problem hiding this comment.
Confirmed the chain: MountFailed on a corrupted etc partition classifies Fatal (error.rs:65), and mount_result? in normal.rs:57 propagates before create_ods_runtime_files runs — so on a release image this halts before any status is ever written to disk.
Fixed in cc63354 + c952d77 for the dominant realistic trigger: run_reset now always includes factory on the restore-phase remount (see reply below), and any failure from that point on resolves to a FactoryResetStatus with data_wiped: true instead of propagating — and mount_remaining_partitions now guards against re-mounting an already-mounted partition (c952d77), closing the EBUSY path a failed cleanup could hit.
Residual case not fixed: if mkfs.ext4 itself genuinely corrupts the partition (as opposed to leaving a stale mount), normal::run's mount_remaining_partitions still hits a real MountFailed → Fatal → halt, and the status still won't reach disk before that halt. Verifying-then-degrading here would mean adding a new fallback boot mode (e.g. mount etc read-only from factory defaults, skip the overlay) — that's a real feature, not a patch, and needs a design decision rather than something to invent unilaterally while triaging review comments. Flagged in the local review-analysis doc for a decision.
There was a problem hiding this comment.
Agreed the realistic triggers are handled (mount guard c952d77 + data_wiped status cc63354). Still pushing on the residual: a genuine mkfs.ext4 corruption of etc leaves normal::run mounting an unmountable partition → MountFailed → Fatal → halt, and that halt is before create_ods_runtime_files, so the new data_wiped status never reaches disk in exactly that case — still a brick plus a silent outcome. Worth weighing re-mkfs etc to a known-good empty state on a failed remount (keeps it bootable, factory reseeds /etc) as a lighter path than a whole new boot mode. Keeping this thread open until the design decision is made — this belongs in a tracked issue, not only the local review doc.
There was a problem hiding this comment.
Now tracked in #17 (flagged as the higher-priority safety item there).
| return Ok(()); | ||
| } | ||
|
|
||
| let entries = std::fs::read_dir(&dir).map_err(|e| { |
There was a problem hiding this comment.
[Suggestion] I/O errors misclassified as InvalidConfig. A transient failure reading the config dir/files maps to InvalidConfig → run() reports status code Invalid, telling ODS the config is malformed when the real cause is I/O. Safe for data (aborts before reformat), but misleads diagnosis. Consider a separate Io mapping. (Same at the read_to_string on line 129 and the factory-reset.json reads.)
There was a problem hiding this comment.
Confirmed, and confirmed pre-existing (predates this session — same pattern throughout build_preserve_list/collect_application_paths before this file was touched). Not fixed in this pass — it's a diagnostics-quality gap, not a correctness/data-loss issue, since it still safely aborts before any reformat either way. Lower priority than the silent-skip fixes; left open in the local review-analysis doc.
There was a problem hiding this comment.
Agreed — diagnostics-quality, safe (aborts before reformat). Tracked in #17. Leaving open.
…atus The restore-phase remount excluded factory, but setup_etc_overlay needs it mounted to reseed etc's freshly-wiped overlay upper dir with factory defaults before restore_all overlays preserved paths on top. Skipping it left the upper dir permanently unseeded once any preserved path populated it. The now-pointless IncludeFactory split is removed; both mount passes always include factory. Corrected the design spec's step 8, which had the same oversight. Also splits run_reset at the point the first reformat begins: failures before that are safe pre-reformat aborts (unchanged), failures at or after are resolved into FactoryResetStatus with a new data_wiped flag and the preserve list populated, so ODS/cloud can distinguish a safe no-op abort from a failure after data was already wiped. Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>
factory/cert/etc/data mounts had no idempotency check, unlike the boot partition in mount_core_partitions. A mount leaked by any earlier failed step (e.g. an aborted factory reset whose cleanup unmount also failed) would hit mount(2) again on an already-mounted target, fail as MountFailed, and get classified Fatal — turning a leaked mount into a halt on a release image. Each mount is now skipped with a warning if already mounted, matching the existing boot-partition pattern. Also corrects a stale comment on the vfat boot mount: the group is resolved dynamically via lookup_disk_gid with a gid=6 fallback, not hardcoded. Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>
Non-string array elements and unreadable factory-reset.d directory entries were silently skipped (as::str() returning None, or entries.flatten() dropping Err results). Given a preserve list is what survives a reformat, silently dropping an entry means the intended path is wiped and restore_all still reports Success. Both now hard-fail with InvalidConfig, which is safe since they run before any reformat. Also swaps unwrap_or_else(|| unreachable!()) for the equivalent, clearer expect(). Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>
Shorten three doc comments flagged as too verbose in PR review: validate_preserve_path, run_reset, and the restore-phase remount call. Keep the "why", drop restated context already evident from the code. Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>
Shorten the mount-idempotency explanation flagged as too verbose in PR review. Keep the "why" (guards against a leaked mount escalating to a fatal MountFailed), drop the restated convention citation and error classification detail already evident from the code. Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>
…d gid handling Addresses @mlilien's review findings on PR omnect#16: - Remove a boot_sequence.rs comment restating detail already documented on MountOptions::vfat()/VFAT_BOOT_DATA in mount.rs. - Revert the disk-group GID from a runtime /etc/group lookup back to a hardcoded constant: this crate only ever boots omnect's own Yocto-built images, where the disk group's GID is fixed by the group recipe, so the three-branch fallback lookup (missing file, missing entry, parse failure) added no value and only added boot-time attack surface and doc-comment confusion about its scope. - Fix a misleading doc comment on CP_CMD: it described the -a flag as a property of the constant, but the flag is applied per call site and isn't even uniform across all three call sites. Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>
Move factory-reset's bespoke mount/fsck/unmount logic into the shared filesystem module: mount_tracked_partition (with fsck, fixing a gap where factory-reset previously skipped fsck before backing up data/etc), unmount_tracked, and setup_etc_overlay_tracked/setup_data_overlay_tracked. factory_reset_mount now only supplies factory-reset-specific partition choices instead of duplicating mount mechanics. Also fixes a partial-mount-leak: the tracked overlay setup functions now record each sub-mount target immediately after it succeeds, instead of only after the whole overlay setup call returns Ok, so a partially-failed overlay setup no longer leaves an untracked mount behind on cleanup. Dropped the is_path_mounted guard in the unmount loop: mounts is only ever populated immediately after a successful mount in a single-threaded flow, so the guard never observed an unmounted tracked path. Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>
The previous comment included implementation details about cross-module usage and sharing of `CP_CMD`. This change removes that specific context, making the comment more concise and focused on the constant's immediate purpose, aligning with prior efforts to trim verbose documentation.
3 participants
Summary
BootMode::FactoryReset, detected via a newBootEnvKey::FactoryResetbootloaderenv key (feature
factory-reset).mode::factory_reset: build a selectivepreserve-list (
config.rs), mount factory(ro)/etc(rw)/data(rw) + overlays, back uppreserved paths, reformat
dataandetc(ext4), remount etc(rw)/data(rw) (factoryexcluded per spec), and restore.
FactoryResetError, classified asContinueDegraded-> a failed or unsupportedreset never blocks boot; outcome is always recorded in the ODS status JSON
(
FactoryResetStatus) and the boot always falls through tomode::normal::run()...-containing entries beforethey're joined onto rootfs, closing a path-traversal gap for third-party
factory-reset.d/*.jsonfiles.factory_reset_mount()failure and anis_path_mounted()-error-swallowing bug in the unmount cleanup path.<rootfs>/etc/groupinstead of a hardcodedgid=6.README.md: factory reset moves from "planned" to "implemented", and thestartup-flow diagram now shows the
BootMode::detect()branch and reset sequence.Reason
Ports the last selective-preserve factory-reset behavior (mode 1) from the legacy bash
initramfs scripts into the Rust binary, per
docs/superpowers/specs/2026-06-29-factory-reset-mode1-design.md. ODS triggers a resetby writing the
factory-resetbootloader env key; the device must always continuebooting regardless of reset outcome.
Verification
cargo testacross the standard feature matrix plusfactory-reset(
grub|ubootxgpt|dosxtest-utils, with/withoutfactory-reset,resize-data,release-image) - all passing.cargo fmt -- --checkandcargo clippy --tests -- -D warningsclean.cargo audit- no new advisories...traversal / normalpaths) and the dynamic vfat GID lookup (missing file, missing entry, configured
value).
test table is still outstanding.