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

Iterate encrypted clones at zvol_create_minor #12471

Merged
merged 1 commit into from
Sep 13, 2021

Conversation

lundman
Copy link
Contributor

@lundman lundman commented Aug 11, 2021

Create device nodes for encrypted cloned zvols.

Userland figures out which encryption-root keys are required to load,
and issues ZFS_IOC_LOAD_KEY. The tail section of spa_keystore_load_wkey()
will call zvol_create_minors() on the encryption-root object.

Any clones of the encrypted zvol will not be plumbed. This commits
adds additional logic to detect if zvol has clones, and is encrypted,
then adds these to the list of zvols to call zvol_create_minors() on.

Motivation and Context

The old incorrect behavior is easily tested by;

zfs create -V1G -o encryption=on -o keyformat=passphrase BOOM/vol1
zfs snap BOOM/vol1@snap
zfs clone BOOM/vol1@snap BOOM/vol2
zpool export -a
zpool import -l

and on import, it will plumb devices for vol1 only, vol2 is left "unplumbed".

This fixes openzfsonosx/openzfs#99 and possibly related issue #10603

Testing for that "plumbing" succeeds is not an easy task - on macOS we would iterate either diskutil list or ioreg for the existence of the created device nodes. Linux (I am guessing) udev is checked? FreeBSD, /dev/zvol/? So each platform would have its own test needed for a tester.

Description

How Has This Been Tested?

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:

@lundman
Copy link
Contributor Author

lundman commented Sep 7, 2021

Come on, this fixes a Linux problem too, where's my parade?! :)

i mean.. "ping" ...

Userland figures out which encryption-root keys are required to load,
and issues ZFS_IOC_LOAD_KEY.
The tail section of spa_keystore_load_wkey() will call
zvol_create_minors() on the encryption-root object.

Any clones of the encrypted zvol will not be plumbed. This commits
adds additional logic to detect if zvol has clones, and is encrypted,
then adds these to the list of zvols to call zvol_create_minors() on.

Signed-off-by: Jorgen Lundman <[email protected]>
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.

Thanks for the reminder (and fix!) This looks good to me.

My one suggestion would be to add the test case you provided with the original PR. It looks like it would be easy enough to either extend cli_root/zpool_import/zpool_import_encrypted_load.ksh, or to create a new ./cli_root/zpool_import/zpool_import_encrypted_vol.ksh. The only mildly tricky bit should be checking that the volume appears in the right place for each platform. There are some existing tests which do this like zpool_create_014_neg.ksh which you could borrow the right bits from.

@behlendorf behlendorf added Component: ZVOL ZFS Volumes Status: Code Review Needed Ready for review and testing labels Sep 7, 2021
@behlendorf behlendorf requested a review from a user September 7, 2021 17:44
@lundman
Copy link
Contributor Author

lundman commented Sep 8, 2021

Did not do a test, just mentioned it could do with a test, but it would be hard to do.

One of those dream properties, what if we had:

# zfs get all BOOM/volume

BOOM/volume      type                   volume                 -
BOOM/volume      creation               Wed Sep  8 10:04 2021  -
BOOM/volume      used                   1.03G                  -
BOOM/volume      available              95.3G                  -
...
BOOM/volume      device_name            /dev/disk3             -

It'd even be easier to do after the zvol symlinks land, since they'd exist if volumes got plumbed.

@behlendorf
Copy link
Contributor

It'd even be easier to do after the zvol symlinks land, since they'd exist if volumes got plumbed.

Fair enough. With this fix this is the kind of test which I'd expect should work today on Linux and FreeBSD. But I don't think we need to hold up merging this for it. @mmaybee would you mind giving this a review so we can move it forward.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Sep 10, 2021
@behlendorf behlendorf merged commit 7443299 into openzfs:master Sep 13, 2021
@lundman lundman mentioned this pull request Sep 14, 2021
13 tasks
rincebrain pushed a commit to rincebrain/zfs that referenced this pull request Sep 22, 2021
Userland figures out which encryption-root keys are required to load,
and issues ZFS_IOC_LOAD_KEY.
The tail section of spa_keystore_load_wkey() will call
zvol_create_minors() on the encryption-root object.

Any clones of the encrypted zvol will not be plumbed. This commits
adds additional logic to detect if zvol has clones, and is encrypted,
then adds these to the list of zvols to call zvol_create_minors() on.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Jorgen Lundman <[email protected]>
Closes openzfs#12471
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 6, 2021
Userland figures out which encryption-root keys are required to load,
and issues ZFS_IOC_LOAD_KEY.
The tail section of spa_keystore_load_wkey() will call
zvol_create_minors() on the encryption-root object.

Any clones of the encrypted zvol will not be plumbed. This commits
adds additional logic to detect if zvol has clones, and is encrypted,
then adds these to the list of zvols to call zvol_create_minors() on.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Jorgen Lundman <[email protected]>
Closes openzfs#12471
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 6, 2021
Userland figures out which encryption-root keys are required to load,
and issues ZFS_IOC_LOAD_KEY.
The tail section of spa_keystore_load_wkey() will call
zvol_create_minors() on the encryption-root object.

Any clones of the encrypted zvol will not be plumbed. This commits
adds additional logic to detect if zvol has clones, and is encrypted,
then adds these to the list of zvols to call zvol_create_minors() on.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Jorgen Lundman <[email protected]>
Closes openzfs#12471
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: ZVOL ZFS Volumes Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cloned encrypted zvol does not get create_minor.
3 participants