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

ZIL: Do not encrypt block pointers in lr_clone_range_t #15543

Closed
wants to merge 2 commits into from
Closed

ZIL: Do not encrypt block pointers in lr_clone_range_t #15543

wants to merge 2 commits into from

Conversation

amotin
Copy link
Member

@amotin amotin commented Nov 19, 2023 •
edited
Loading

In case of crash cloned blocks need to be claimed on pool import. It is only possible if they (lr_bps) and their count (lr_nbps) are not encrypted but only authenticated, similar to block pointer in lr_write_t. Few other fields can be and are still encrypted.

This should fix ZIL corruption causing panic on ZIL claim after crash when block cloning is actively used on encrypted dataset.

Fixes: #15513

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:

@amotin amotin added the Status: Code Review Needed Ready for review and testing label Nov 19, 2023
This was referenced Nov 19, 2023
Closed
Merged
@amotin
Copy link
Member Author

amotin commented Nov 20, 2023

@tcaputi Could you please take a look?

@tcaputi
Copy link
Contributor

tcaputi commented Nov 20, 2023

This looks good to me. Should probably get one other person to review it who has worked on zfs in the last 3 years though :P

@Majiir Majiir mentioned this pull request Nov 21, 2023
Merged
20 tasks
@amotin amotin requested a review from behlendorf November 21, 2023 16:59
ryao
ryao approved these changes Nov 22, 2023
View reviewed changes
Copy link
Contributor

@ryao ryao left a comment

Choose a reason for hiding this comment

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

I feel like the change to TX_WRITE could be broken into a separate cleanup commit. Other than that, this looks fine to me.

module/os/freebsd/zfs/zio_crypt.c Show resolved Hide resolved
@amotin
ZIL: Do not encrypt block pointers in lr_clone_range_t
3a2c1bc 
In case of crash cloned blocks need to be claimed on pool import.
It is only possible if they (lr_bps) and their count (lr_nbps) are
not encrypted but only authenticated, similar to block pointer in
lr_write_t.  Few other fields can be and are still encrypted.

This should fix panic on ZIL claim after crash when block cloning
is actively used.

Signed-off-by:	Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
@amotin
ZIL: Refactor TX_WRITE encryption similar to TX_CLONE_RANGE
761ca54 
It should be purely textual change to make the code more readable.
Should cause no functional difference.

Signed-off-by:	Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Nov 27, 2023
behlendorf pushed a commit that referenced this pull request Nov 27, 2023
@amotin @behlendorf
ZIL: Refactor TX_WRITE encryption similar to TX_CLONE_RANGE
a490875 
It should be purely textual change to make the code more readable.
Should cause no functional difference.

Reviewed-by: Richard Yao <[email protected]>
Reviewed-by: Tom Caputi <[email protected]>
Reviewed-by: Sean Eric Fagan <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Edmund Nadolski <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Sponsored by: iXsystems, Inc.
Closes #15543
Closes #15513
@amotin amotin deleted the brt_enc branch November 27, 2023 18:01
amotin added a commit to amotin/zfs that referenced this pull request Nov 28, 2023
@amotin
ZIL: Do not encrypt block pointers in lr_clone_range_t
8a63cf1 
In case of crash cloned blocks need to be claimed on pool import.
It is only possible if they (lr_bps) and their count (lr_nbps) are
not encrypted but only authenticated, similar to block pointer in
lr_write_t.  Few other fields can be and are still encrypted.

This should fix panic on ZIL claim after crash when block cloning
is actively used.

Reviewed-by: Richard Yao <[email protected]>
Reviewed-by: Tom Caputi <[email protected]>
Reviewed-by: Sean Eric Fagan <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Edmund Nadolski <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Sponsored by: iXsystems, Inc.
Closes openzfs#15543
Closes openzfs#15513
@amotin amotin mentioned this pull request Nov 28, 2023
Merged
13 tasks
behlendorf pushed a commit that referenced this pull request Nov 28, 2023
@amotin @behlendorf
ZIL: Do not encrypt block pointers in lr_clone_range_t
56a2a09 
In case of crash cloned blocks need to be claimed on pool import.
It is only possible if they (lr_bps) and their count (lr_nbps) are
not encrypted but only authenticated, similar to block pointer in
lr_write_t.  Few other fields can be and are still encrypted.

This should fix panic on ZIL claim after crash when block cloning
is actively used.

Reviewed-by: Richard Yao <[email protected]>
Reviewed-by: Tom Caputi <[email protected]>
Reviewed-by: Sean Eric Fagan <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Edmund Nadolski <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Sponsored by: iXsystems, Inc.
Closes #15543
Closes #15513
@amotin amotin mentioned this pull request Dec 4, 2023
Merged
13 tasks
amotin added a commit to amotin/zfs that referenced this pull request Dec 4, 2023
@amotin
zdb: Dump encrypted write and clone ZIL records
fa17cd9 
Block pointers are not encrypted in TX_WRITE and TX_CLONE_RANGE
records, so we can dump them, that may be useful for debugging.

Related to openzfs#15543.

Signed-off-by:	Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
amotin added a commit to amotin/zfs that referenced this pull request Dec 4, 2023
@amotin
zdb: Dump encrypted write and clone ZIL records
4ca1427 
Block pointers are not encrypted in TX_WRITE and TX_CLONE_RANGE
records, so we can dump them, that may be useful for debugging.

Related to openzfs#15543.

Signed-off-by:	Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
amotin added a commit to amotin/zfs that referenced this pull request Dec 4, 2023
@amotin
zdb: Dump encrypted write and clone ZIL records
5d56660 
Block pointers are not encrypted in TX_WRITE and TX_CLONE_RANGE
records, so we can dump them, that may be useful for debugging.

Related to openzfs#15543.

Signed-off-by:	Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
behlendorf pushed a commit that referenced this pull request Dec 6, 2023
@amotin
zdb: Dump encrypted write and clone ZIL records
f9765b1 
Block pointers are not encrypted in TX_WRITE and TX_CLONE_RANGE
records, so we can dump them, that may be useful for debugging.

Related to #15543.
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
Closes #15629
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Dec 12, 2023
@amotin @lundman
ZIL: Do not encrypt block pointers in lr_clone_range_t
770afbb 
In case of crash cloned blocks need to be claimed on pool import.
It is only possible if they (lr_bps) and their count (lr_nbps) are
not encrypted but only authenticated, similar to block pointer in
lr_write_t.  Few other fields can be and are still encrypted.

This should fix panic on ZIL claim after crash when block cloning
is actively used.

Reviewed-by: Richard Yao <[email protected]>
Reviewed-by: Tom Caputi <[email protected]>
Reviewed-by: Sean Eric Fagan <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Edmund Nadolski <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Sponsored by: iXsystems, Inc.
Closes openzfs#15543
Closes openzfs#15513
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Dec 12, 2023
@amotin @lundman
ZIL: Refactor TX_WRITE encryption similar to TX_CLONE_RANGE
c8d5e28 
It should be purely textual change to make the code more readable.
Should cause no functional difference.

Reviewed-by: Richard Yao <[email protected]>
Reviewed-by: Tom Caputi <[email protected]>
Reviewed-by: Sean Eric Fagan <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Edmund Nadolski <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Sponsored by: iXsystems, Inc.
Closes openzfs#15543
Closes openzfs#15513
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Dec 12, 2023
@amotin @lundman
zdb: Dump encrypted write and clone ZIL records
f490f2e 
Block pointers are not encrypted in TX_WRITE and TX_CLONE_RANGE
records, so we can dump them, that may be useful for debugging.

Related to openzfs#15543.
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
Closes openzfs#15629
mmatuska pushed a commit to mmatuska/zfs that referenced this pull request Dec 27, 2023
@amotin @mmatuska
zdb: Dump encrypted write and clone ZIL records
4d74a5d 
Block pointers are not encrypted in TX_WRITE and TX_CLONE_RANGE
records, so we can dump them, that may be useful for debugging.

Related to openzfs#15543.
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
Closes openzfs#15629
behlendorf pushed a commit that referenced this pull request Jan 9, 2024
@amotin @behlendorf
zdb: Dump encrypted write and clone ZIL records
dea2d3c 
Block pointers are not encrypted in TX_WRITE and TX_CLONE_RANGE
records, so we can dump them, that may be useful for debugging.

Related to #15543.
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
Closes #15629
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryao ryao ryao approved these changes

@ednadolski-ix ednadolski-ix ednadolski-ix approved these changes

@kithrup kithrup kithrup approved these changes

@tcaputi tcaputi tcaputi 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
Development

Successfully merging this pull request may close these issues.

Page Fault on Pool Import
6 participants
@amotin @tcaputi @behlendorf @ryao @kithrup @ednadolski-ix