Skip to content

Conversation

@ahesford
Copy link
Member

By default, check now always returns 255 to prevent automatic installation of the dracut module; it must be explicitly requested or pulled in as a dependency.

install now checks the exit codes of all critical installation commands and, for files that absolutely must exist in the initramfs, failure to install will trigger a dracut failure. For a few optional files, warnings are emitted but installation continues. We can argue about the specific "essential" executables, but most are required in the early parts of zfsbootmenu.sh that will get to the point of automatically booting a kernel and, for some others like grep, they should definitely be in the image to ensure that the emergency shell has important functionality in the event the menuing system cannot be displayed or some other critical error occurs. (These commands should be sufficient to at least set a bootfs or, for those with a deeper understanding, drive the ZBM kexec sequence manually---this has saved me a few times!)

When we merge skim_support, fzf will have to be special-cased to fallback to sk and, if that isn't available, emit the warning.

I'm marking WIP because this is totally untested but looks functional and because I want to get some feedback before we really dive in here.

NB: I've dropped the /usr/bin prefix from the ZFS executables. I'm not sure why they were there in the first place, but it's another point that needs testing.

@ahesford ahesford requested a review from zdykstra August 21, 2020 20:42
Copy link
Member

@zdykstra zdykstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the exception of what should be considered required and optional, I like this. It does highlight other bits of hardening we should/could do - but aren't strictly in the scope of this PR.

@ahesford
Copy link
Member Author

Still untested, but ready for testing. I put the sk fallback install in here even though we still need the additional logic from skim_support; there were going to be merge conflicts no matter what.

I added blkid as a hard requirement. It seems to be in there as a consequence of udev, but since we call it explicitly in the check for resume images, we should explicitly require it.

@zdykstra
Copy link
Member

I added blkid as a hard requirement. It seems to be in there as a consequence of udev, but since we call it explicitly in the check for resume images, we should explicitly require it.

Good catch on blkid !

@zdykstra
Copy link
Member

I've tested creating a few different initramfs's that check the major code paths in module-setup.sh. A full clone test in the initramfs works as well.

@zdykstra zdykstra merged commit 2225fea into master Aug 22, 2020
@zdykstra zdykstra deleted the failsafe branch August 22, 2020 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants