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.

Sign up for GitHub

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

Jump to bottom

module: zfs: receive checks should allow unencrypted child datasets. #13076

Merged
merged 2 commits into from
Feb 9, 2022
Merged

module: zfs: receive checks should allow unencrypted child datasets. #13076

merged 2 commits into from
Feb 9, 2022

Conversation

AttilaFueloep
Copy link
Contributor

Motivation and Context

dmu_recv_begin_check() unconditionally sets the DS_HOLD_FLAG_DECRYPT
flag before calling dsl_dataset_hold_flags(). If the key on the
receiving side isn't loaded or the send stream contains embedded
blocks, the receive check fail for a stream which is perfectly
valid and could be received without any problems. This seems like
a remnant of the initial design, where unencrypted datasets below
encrypted ones weren't allowed.

Description

Add a condition to set DS_HOLD_FLAG_DECRYPT only for encrypted
datasets, modify an existing test to detect this regression and add
a test for raw replication streams.

Closes #13033

How Has This Been Tested?

Verified that the modified regression tests fail on current master and pass with this PR applied.

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:

@AttilaFueloep AttilaFueloep mentioned this pull request Feb 7, 2022
Closed
@gamanakis
Copy link
Contributor

The core tests (checkstyle, zfs-sanity-tests, zfs-tests-functional) are failing because of add15e9. If you rebase to master, this will get them going.

@AttilaFueloep @gamanakis
module: zfs: receive checks should allow unencrypted child datasets.
70a3eba 
dmu_recv_begin_check() unconditionally sets the DS_HOLD_FLAG_DECRYPT
flag before calling dsl_dataset_hold_flags(). If the key on the
receiving side isn't loaded or the send stream contains embedded
blocks, the receive check fails for a stream which is perfectly
valid and could be received without any problem. This seems like
a remnant of the initial design, where unencrypted datasets below
encrypted ones weren't allowed.

Add a condition to set `DS_HOLD_FLAG_DECRYPT` only for encrypted
datasets, modify an existing test to detect this regression and add
a test for raw replication streams.

Co-authored-by: George Amanakis <[email protected]>
Signed-off-by: Attila Fülöp <[email protected]>
@AttilaFueloep
Copy link
Contributor Author

Thanks for the hint, rebased. Fixed two typos in the commit message as well.

behlendorf
behlendorf approved these changes Feb 8, 2022
View reviewed changes
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.

Makes sense, thanks for running this down.

tests/zfs-tests/tests/functional/cli_root/zfs_receive/zfs_receive_-wR-encrypted-mix.ksh Outdated
#
# STRATEGY:
# 1. Create the dataset hierarchy
# 3. Snapshot the dataset hierarchy
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: 2., 3., 4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course.

tests/zfs-tests/tests/functional/cli_root/zfs_receive/zfs_receive_-wR-encrypted-mix.ksh Outdated
# STRATEGY:
# 1. Create the dataset hierarchy
# 3. Snapshot the dataset hierarchy
# 5. Send -Rw the dataset hierarchy and receive uinto a toplevel dataset
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# 5. Send -Rw the dataset hierarchy and receive uinto a toplevel dataset
# 5. Send -Rw the dataset hierarchy and receive into a top-level dataset

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Feb 8, 2022
@AttilaFueloep
Copy link
Contributor Author

Sorry, accidentally omitted the signed-off in the last commit, that's why checkstyle fails. Will fix when squashing.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Feb 9, 2022
@behlendorf
Copy link
Contributor

No problem, if you prefer I can fix is when squashing the commits for the merge,

@behlendorf
Copy link
Contributor

@gamanakis would you mind also reviewing this.

@behlendorf behlendorf merged commit 68ddc06 into openzfs:master Feb 9, 2022
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Feb 15, 2022
@AttilaFueloep @tonyhutter
Receive checks should allow unencrypted child datasets
c1001b6 
dmu_recv_begin_check() unconditionally sets the DS_HOLD_FLAG_DECRYPT
flag before calling dsl_dataset_hold_flags(). If the key on the
receiving side isn't loaded or the send stream contains embedded
blocks, the receive check fails for a stream which is perfectly
valid and could be received without any problem. This seems like
a remnant of the initial design, where unencrypted datasets below
encrypted ones weren't allowed.

Add a condition to set `DS_HOLD_FLAG_DECRYPT` only for encrypted
datasets, modify an existing test to detect this regression and add
a test for raw replication streams.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: George Amanakis <[email protected]>
Co-authored-by: George Amanakis <[email protected]>
Signed-off-by: Attila Fülöp <[email protected]>
Closes openzfs#13033 
Closes openzfs#13076
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Feb 16, 2022
@AttilaFueloep @tonyhutter
Receive checks should allow unencrypted child datasets
abcdb17 
dmu_recv_begin_check() unconditionally sets the DS_HOLD_FLAG_DECRYPT
flag before calling dsl_dataset_hold_flags(). If the key on the
receiving side isn't loaded or the send stream contains embedded
blocks, the receive check fails for a stream which is perfectly
valid and could be received without any problem. This seems like
a remnant of the initial design, where unencrypted datasets below
encrypted ones weren't allowed.

Add a condition to set `DS_HOLD_FLAG_DECRYPT` only for encrypted
datasets, modify an existing test to detect this regression and add
a test for raw replication streams.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: George Amanakis <[email protected]>
Co-authored-by: George Amanakis <[email protected]>
Signed-off-by: Attila Fülöp <[email protected]>
Closes openzfs#13033 
Closes openzfs#13076
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Feb 17, 2022
@AttilaFueloep @tonyhutter
Receive checks should allow unencrypted child datasets
5c19af0 
dmu_recv_begin_check() unconditionally sets the DS_HOLD_FLAG_DECRYPT
flag before calling dsl_dataset_hold_flags(). If the key on the
receiving side isn't loaded or the send stream contains embedded
blocks, the receive check fails for a stream which is perfectly
valid and could be received without any problem. This seems like
a remnant of the initial design, where unencrypted datasets below
encrypted ones weren't allowed.

Add a condition to set `DS_HOLD_FLAG_DECRYPT` only for encrypted
datasets, modify an existing test to detect this regression and add
a test for raw replication streams.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: George Amanakis <[email protected]>
Co-authored-by: George Amanakis <[email protected]>
Signed-off-by: Attila Fülöp <[email protected]>
Closes openzfs#13033 
Closes openzfs#13076
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
@AttilaFueloep @nicman23
Receive checks should allow unencrypted child datasets
9390183 
dmu_recv_begin_check() unconditionally sets the DS_HOLD_FLAG_DECRYPT
flag before calling dsl_dataset_hold_flags(). If the key on the
receiving side isn't loaded or the send stream contains embedded
blocks, the receive check fails for a stream which is perfectly
valid and could be received without any problem. This seems like
a remnant of the initial design, where unencrypted datasets below
encrypted ones weren't allowed.

Add a condition to set `DS_HOLD_FLAG_DECRYPT` only for encrypted
datasets, modify an existing test to detect this regression and add
a test for raw replication streams.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: George Amanakis <[email protected]>
Co-authored-by: George Amanakis <[email protected]>
Signed-off-by: Attila Fülöp <[email protected]>
Closes openzfs#13033 
Closes openzfs#13076
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
@AttilaFueloep @nicman23
Receive checks should allow unencrypted child datasets
a5266c5 
dmu_recv_begin_check() unconditionally sets the DS_HOLD_FLAG_DECRYPT
flag before calling dsl_dataset_hold_flags(). If the key on the
receiving side isn't loaded or the send stream contains embedded
blocks, the receive check fails for a stream which is perfectly
valid and could be received without any problem. This seems like
a remnant of the initial design, where unencrypted datasets below
encrypted ones weren't allowed.

Add a condition to set `DS_HOLD_FLAG_DECRYPT` only for encrypted
datasets, modify an existing test to detect this regression and add
a test for raw replication streams.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: George Amanakis <[email protected]>
Co-authored-by: George Amanakis <[email protected]>
Signed-off-by: Attila Fülöp <[email protected]>
Closes openzfs#13033 
Closes openzfs#13076
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gamanakis gamanakis gamanakis approved these changes

@behlendorf behlendorf behlendorf approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
3 participants
@AttilaFueloep @gamanakis @behlendorf