zfs icon indicating copy to clipboard operation
zfs copied to clipboard

initramfs: fix import of additional zpools on boot

Open theQuestionmark opened this issue 3 years ago • 8 comments
trafficstars

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: https://github.com/zfsonlinux/pkg-zfs/issues/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

  • [x] 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:

  • [x] My code follows the OpenZFS code style requirements.
  • [ ] I have updated the documentation accordingly.
  • [x] I have read the contributing document.
  • [ ] I have added tests to cover my changes.
  • [ ] I have run the ZFS Test Suite with this change applied.
  • [x] All commit messages are properly formatted and contain Signed-off-by.

theQuestionmark avatar Jun 09 '22 15:06 theQuestionmark

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

bghira avatar Jun 09 '22 21:06 bghira

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?

bghira avatar Jun 09 '22 21:06 bghira

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

theQuestionmark avatar Jun 10 '22 10:06 theQuestionmark

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.

bghira avatar Jun 10 '22 17:06 bghira

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.

theQuestionmark avatar Jun 11 '22 22:06 theQuestionmark

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

behlendorf avatar Jun 29 '22 23:06 behlendorf

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: https://github.com/theQuestionmark/zfs/commit/bdc327efab64891b3899478cac017327320c0175 How will I get squashed commit to replace the made commits in this PR?

theQuestionmark avatar Jul 17 '22 14:07 theQuestionmark

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.

nabijaczleweli avatar Jul 26 '22 19:07 nabijaczleweli

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!

lifo9 avatar Aug 31 '23 20:08 lifo9