switch-root: return real errors instead of false success

[FixeQD]

So this started because I hit a (I would name it "a bug") in a downstream project (finix) where initctl switch-root always exited 0 no matter what happened, even when the switch actually failed. Turned out do_switch_root_api() was sending ACK to the client before switch_root() had validated anything, so any failure after that point was just invisible, client already thought it worked

3 commits, each one is a separate thing so they're easy to review/revert on their own if needed:

1. Split validation out of switch_root() into switch_root_precheck(), run it before ACK. If precheck fails (bad newroot, missing/non-executable init, whatever), the client now gets a real NACK with an actual error message instead of silence. initctl switch-root finally returns exit code 1 on failure instead of always 0. ACK only goes out once precheck passes, since after that we're committed anyway
2. Precheck was missing two things: access(init_path, X_OK) happily passes for directories since X_OK is just "search permission" and dirs almost always have it, so pointing newinit at a dir by mistake would only blow up at the very end, after everything's already torn down. Added a stat() + S_ISREG check. Also snprintf building init_path wasn't checking for truncation, so a long enough newroot+newinit would silently validate the wrong path. Both are cheap one-off checks, no behavior change for the normal case
3. do_move_mount() returns an int but nobody was checking it. If moving /proc, /sys, /dev or /run to the new root failed, it only logged at dbg() level (so basically nowhere by default) and switch_root kept going straight into chroot+exec anyway. New init boots into an environment missing core virtual filesystems and fails in some completely unrelated, confusing way later. Now it aborts and logs at LOG_ERR if any of the 4 moves fail

Didn't touch the deeper issue that a failure after all this (mid-teardown, e.g. execl() itself failing once everything's already killed and mounted still leaves the process alive but the system in a pretty rough state with nothing left to talk to it. That's a real design question (rescue shell as last resort? just die loud?) and I didn't want to sneak a behavior change like that into a validation PR without discussing it first

Sorry if this description is longer than it needs to be, heard you're (Yes, I'm talking about u @troglobit :P) pretty serious about code quality on this project so figured I'd rather over-explain the reasoning than have you guess at it from the diff alone

Btw C is a dystopian hell 😭

[troglobit]

Sorry for the slow reply, been on vacation and stayed off the keyboard a bit more than usual.

Thanks for the writeup, and no need to apologize for the length, it helped. You are right about the bug: the ACK went out before anything was checked. Factoring out a precheck is a good way around that, and three small commits made it easy to read.

A few things before this can go in.

do_move_mount() cannot tell "not mounted" from "not a mount point":

if (stat(oldpath, &st))
      return 0;               /* Not mounted, skip */

stat() succeeds on a plain directory, so we reach MS_MOVE and get EINVAL back. /run can be in that state: fs_finalize() only mounts a tmpfs there if !fistmpfs("/run"), and fistmpfs() uses statfs(), which reports the containing filesystem, so on a tmpfs rootfs /run stays a plain directory. A ramfs initramfs does get the tmpfs, so it depends on the layout. But where it happens, your patch turns a dbg() on a working system into a switch-root that dies after everything has been killed. The guard needs fismnt(), or an st_dev compare against the parent, before a -1 can be fatal.

Also, the || chain stops at the first failure, so a bad /dev means /proc, /sys and /run are never tried. Try all four and log each one.

Now, on the question you left open; I think rescue mode is a good idea here. We already do this for other boot failures we cannot come back from, see fsck() and fs_mount_all() in finit.c: log with LOG_CONSOLE | LOG_ALERT, then call sulogin(1), which gives you a maintenance shell and reboots when you exit it. A switch-root that dies mid-teardown is the same kind of failure, and this is complex enough to debug that I want a shell instead of a dead machine.

So for anything past the point of no return, the moves, the chdir, the mount, the chroot, and a failed execl, drop the return -1 and go to sulogin instead. Two things to watch if you take it on: sulogin() is static in finit.c, so it needs exporting, and sig_unblock() only runs just before the execl today, so it
has to happen first or the shell inherits a blocked signal mask. The shell will also be in better shape before the moves than after a partial one, since by then /dev has moved. Still much better than what we have.

Next, the runlevel guard still ACKs, which is the same bug you found:

case INIT_CMD_SWITCH_ROOT:
      if (runlevel != INIT_LEVEL && runlevel != 1) {
              warnx("switch-root only allowed in runlevel S or 1");
              goto done;
      }

result is still 0, and done: sends ACK when result is 0. So switch-root in the wrong runlevel exits 0 and prints nothing. Needs result = 1; before the goto.

The NACK also loses the reason. The precheck logs what is actually wrong, but only errno reaches the client, so the user gets switch-root: Invalid argument while the useful text goes to the console. Pass the message out too. Some precheck paths also let close() and logit() run before errno is read, so you can end up with switch-root: Success.

Smaller things:

• Let api_cb() send the reply. Set result and fall through to done: instead of writing the NACK and closing sd yourself. leave: closes sd as well, so it now gets closed twice on the common path
• The precheck reuses newroot_st for the init binary. Correct, but a separate struct stat st is easier to follow
• errno = EACCES covers both "stat failed" and "not a regular file". EISDIR or ENOEXEC fits what we print
• logit(LOG_ERR, "switch_root: cannot stat /") drops strerror(errno), unlike its neighbours
• Add a comment on the client_send() && rq.cmd == INIT_CMD_NACK test saying why it differs from do_cmd(), that a lost connection is the expected success here. Otherwise it reads like a broken copy. It also still exits 0 when finit is not running, since client_send() returns 255 for connect and write failures
  too and leaves rq.cmd alone

On the commits themselves: we do not use the Conventional Commits style. So write initramfs: ... not fix(initramfs): ..., two subjects also wrap onto a second line with no blank line, so git reads that as the body. And all three are subject only, which is the main thing I want fixed. The reasoning in your PR text belongs in the commit bodies, commit messages are there to tell the story of why :-)

[FixeQD]

Fixed everything from the review, ready for another pass whenever you've got time
Lmk if I missed anything or messed something up.

Also hope the vacation actually recharged you and this PR isn't what welcomes you back lol

[FixeQD]

Time to fix merge conflicts... Btw @troglobit u still alive?

[troglobit]

Yes, still alive @FixeQD :-) Been really busy lately, first evening I've had to sit down with this properly.

The update addresses everything from the review, nice work! One thing left: the wrong-runlevel path now NACKs, but rq.data is sent back untouched, and initctl prints rq.data on NACK. So initctl switch-root /mnt in runlevel 2 exits 1 but prints /mnt as the reason. It needs a message written into rq.data, same as do_switch_root_api() does.

On the commits, I think you read more into my last comment than I intended. One commit per review point is more than the change needs, and the new ones are still subject-only, which was the main thing I wanted fixed. Folded together this is maybe four commits, each with the why in the body, and your PR text already has all of it.

Rather than sending you around another lap: if it's OK with you I'll take the branch from here, sort out the conflicts, fold the commits into a final series with your authorship intact, fix the runlevel message, and merge.

You've done the hard part. If you'd rather finish it yourself, that's fine too, just let me know.

[FixeQD]

Alr, if u want u can take the branch, good luck 👍

Hand-over.

[troglobit]

Alr, if u want u can take the branch, good luck 👍

There, just waiting for regression tests before merging. Thank you again for your contribution, very appreciated! 🙏 🙇‍♂️

[troglobit]

Only one flaky test that failed in the gcc job, test fixed on devman branch in commit 8138b40. Merging.