Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

initramfs: fix import of additional zpools on boot #13543

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

theQuestionmark
Copy link

Motivation and Context

Currently all zpools defined in ZFS_INITRD_ADDITIONAL_DATASETS
aren't imported correctly if they are on an other zpool than
the root-pool. The mount fails hence the zpool is not known.
Initramfs asks know for the second decryption-key on boot for
second zpool.

Fixes the following issue:
zfsonlinux/pkg-zfs#237

Description

Adding a for-loop after the rpool gets imported, execs only if ZFS_INITRD_ADDITIONAL_DATASETS is set in /etc/default/zfs.
The import is only done for zpools which are under the root-pool-base.
The script is going to import the additional zpools but does not succeed
because the zpool isn't imported when the mount is going to happen.

How Has This Been Tested?

Tested on Debian 11 with kernel 5.10.0-14-amd64 and with the following zpools:

NAME     SIZE  ALLOC   FREE  CKPOINT  EXPANDSZ   FRAG    CAP  DEDUP    HEALTH  ALTROOT
bpool    960M  84.7M   875M        -         -     0%     8%  1.00x    ONLINE  -
rpool   29.5G  1.11G  28.4G        -         -     1%     3%  1.00x    ONLINE  -
vmpool    18G  1.56M  18.0G        -         -     0%     0%  1.00x    ONLINE  -

Where the rpool and the vmpool are encrypted with zfs native encryption.
Now the zfsunlock-script askes also for the key of the vmpool and it's mounted according the defined mountpoint.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Jun 9, 2022
@bghira
Copy link

bghira commented Jun 9, 2022

it would be best if there were an option to reuse the same key for all pools that we try to import.

@bghira
Copy link

bghira commented Jun 9, 2022

if the key for the 2nd pool is set as a file on the first pool, does keylocation need to be prefixed via /sysroot/key/path or will it work with the same fully qualified path that works in multiuser target?

@theQuestionmark
Copy link
Author

if the key for the 2nd pool is set as a file on the first pool, does keylocation need to be prefixed via /sysroot/key/path or will it work with the same fully qualified path that works in multiuser target?

Just tested it, if your key is /vmpoolkey in multiuser-target, your zfs keylocation must be prefixed with /root. e.g. file:///root/vmpoolkey

Unlocking then works with only the passphrase provided for rpool.


root@this-host:~# ls -la /vmpoolkey
-r-------- 1 root root 32 Jun 10 12:06 /vmpoolkey
root@this-host:~# zfs get keylocation vmpool
NAME    PROPERTY     VALUE                   SOURCE
vmpool  keylocation  file:///root/vmpoolkey  local

@bghira
Copy link

bghira commented Jun 10, 2022

so there's two initramfs subsystems and one is the initramfs-tools which you've pushed this for and the other is dracut. it would be most excellent to keep them at feature-parity and include the fix in both environments, where applicable.

i would still prefer if we use zfs load-key -L <location> to load the key from the correct location without having to add an arbitrary prefix into the keylocation property. I have Gentoo and Ubuntu both installed side by side, and one uses /sysroot and the other uses /root because the former uses Dracut and the latter uses initramfs-tools. you can not put the /sysroot/path/to/key into keylocation because Ubuntu then cannot find it.

@theQuestionmark
Copy link
Author

theQuestionmark commented Jun 11, 2022

so there's two initramfs subsystems and one is the initramfs-tools which you've pushed this for and the other is dracut. it would be most excellent to keep them at feature-parity and include the fix in both environments, where applicable.

i would still prefer if we use zfs load-key -L <location> to load the key from the correct location without having to add an arbitrary prefix into the keylocation property. I have Gentoo and Ubuntu both installed side by side, and one uses /sysroot and the other uses /root because the former uses Dracut and the latter uses initramfs-tools. you can not put the /sysroot/path/to/key into keylocation because Ubuntu then cannot find it.

Thank you for the explanation, didn't know that!
I agree, that we should fix this in both. But it could take me a couple of days to fully understand the inner workings of the dracut scripts.
I'm kinda new to the initramfs-tools/dracut environments.

It would be more user friendly if the user could ignore the prefix at all when setting the keylocation and the scripts add the prefix according to the environment. I would try to implement this as well.

@behlendorf
Copy link
Contributor

If this PR is ready for review would you mind rebasing it on current master branch, squashing the commits, and force updating the PR.

Currently all zpools defined in ZFS_INITRD_ADDITIONAL_DATASETS
aren't imported correctly if they are on an other zpool than
the root-pool. The mount fails hence the zpool is not known.
Initramfs asks know for the second decryption-key on boot for
second zpool.

Signed-off-by: theQuestionmark <[email protected]>
Adding function for honoring the
ZFS_INITRD_ADDITIONAL_DATASETS variable.

Signed-off-by: theQuestionmark <[email protected]>
Change /etc/ to sysconfdir-variable and add default-config
to dracut initrd-image

Signed-off-by: theQuestionmark <[email protected]>
@theQuestionmark theQuestionmark force-pushed the fix-encrypted-zpool-import-initramfs branch from d1c9527 to 9f2fd46 Compare July 17, 2022 14:24
@theQuestionmark
Copy link
Author

If this PR is ready for review would you mind rebasing it on current master branch, squashing the commits, and force updating the PR.

I rebased to the master-branch, squashed the commits and from the master i tried to force update this branch.
I'm stuck somehow, sry I'm not a dev.

The squashed commit is here: theQuestionmark@bdc327e
How will I get squashed commit to replace the made commits in this PR?

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

@nabijaczleweli would you mind looking at this.

@@ -75,6 +93,14 @@ check() {
# shellcheck disable=SC2154
if [ -n "$hostonly" ]; then

if [ -f @sysconfdir@/default/zfs ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Nowhere else in the dracut config do we source @sysconfdir@/default/zfs for environment variables. If we need to be able to specify this we should do it in a way which is consistent the the design described in man/man7/dracut.zfs.7.

@nabijaczleweli
Copy link
Contributor

nabijaczleweli commented Jul 26, 2022

The i-t bit makes sense, even if it's only preferable to have it in the part where we already process ZFS_INITRD_ADDITIONAL_DATASETS:

	for fs in $ZFS_INITRD_ADDITIONAL_DATASETS; do
		import_pool "${fs%%/*}"
		mount_fs "$fs"
	done

Although, notably, ZFS_INITRD_ADDITIONAL_DATASETS is entirely undocumented, so having an empty stub like

diff --git a/etc/default/zfs.in b/etc/default/zfs.in
index ae813e9de..8db0dc451 100644
--- a/etc/default/zfs.in
+++ b/etc/default/zfs.in
@@ -1,7 +1,7 @@
 # OpenZFS userland configuration.
 # shellcheck disable=SC2154
 
-# NOTE: This file is intended for sysv init and initramfs.
+# NOTE: This file is intended for SysV init and initramfs-tools.
 # Changing some of these settings may not make any difference on
 # systemd-based setup, e.g. setting ZFS_MOUNT=no will not prevent systemd
 # from launching zfs-mount.service during boot.
@@ -109,3 +109,7 @@ ZFS_DKMS_DISABLE_STRIP='no'
 # Optional arguments for the ZFS Event Daemon (ZED).
 # See zed(8) for more information on available options.
 #ZED_ARGS="-M"
+
+# Datasets to mount (and pools to import) after the children of the boot dataset
+# in the initrd
+#ZFS_INITRD_ADDITIONAL_DATASETS=

would be great if we're already touching it.

Those two bits would be fine to include.

I have no polite words to say about the dracut parts, and I'm not sure what OP is trying to do there. Get rid of them, a sensible implementation that addresses this can probably be invented if there's demand.

@lifo9
Copy link

lifo9 commented Aug 31, 2023

Hi,

Thank you, @theQuestionmark!
Your PR helped me fix the same issue I had!

Could you please review the remaining comments so it can be merged?

Also, not directly related to this PR, but how do you specify additional pools in ZFS_INITRD_ADDITIONAL_DATASETS?
I have a natively encrypted pool tank, with storage dataset, so I added tank/storage.
But it didn't work - I received an error that it couldn't be found.
I checked /etc/zfs/zpool.cache generated in initramfs and it didn't contain tank, only rpool.
So, I double-checked /etc/zfs/zpool.cache in rootfs and it had both rpool and tank datasets.
I re-generated initramfs and double-checked the generated .img - it had the correct content.
I then rebooted and got the same error, /etc/zfs/zpool.cache in initramfs didn't contain the tank pool.

What helped me was modifying /usr/share/initramfs-tools/hooks/zfs - adding copy_file cache in "/etc/zfs/zpool.cache" "/etc/zfs/zpool.cache-initrd" and setting ZPOOL_CACHE="/etc/zfs/zpool.cache-initrd" in /etc/default/zfs.
See https://github.com/lifo9/enkidu/blob/master/ansible/03_storage/site.yaml

It seems the original /etc/zfs/zpool.cache gets somewhat overwritten.
Have you, by any chance, happened to stumble upon something similar?

Many thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Code Review Needed Ready for review and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants