RFC: Don't panic on unencrypted block in encrypted dataset.

[chrisperedun]

Motivation and Context

While #15465 closes the situation of block cloning creating unencrypted records in encrypted datasets, any existing data that was cloned prior to this still causes panic on read. Setting zfs_recover=1 bypasses the panic but is a bit of a blunt instrument as it avoids all panics.

Since the code paths here already call spa_log_error and return EIO for the read, the actual unencrypted data shouldn't ever be sent upstream, so this shouldn't introduce a vulnerable scenario where deliberately injected data would be returned. We can perhaps be louder about the alert, but this at least prevents users from having accidentally populated an encrypted dataset with files that could crash their system.

Description

Removed the call to zfs_panic_recover at dbuf.c#L1637 and dmu_send.c#L1127

How Has This Been Tested?

Copied a test file from unencrypted to encrypted dataset using cp, confirmed that reading with dd results in a crash, made change, repeated process and received no crash on read, with application receiving an I/O error.

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:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• 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.
• All commit messages are properly formatted and contain Signed-off-by.

[amotin]

While formally it is a pool corruption and should not happen, I agree that it makes no sense to crash the system on it. As these are reads, the errors there should be handled clean and unlike some space map corruptions, for example, should not cause more problems if system is not crashed. May be we could leave some ASSERT() there to ease the debugging of debug builds, but we should not panic production boxes on this.

Looking through the other zfs_panic_recover() use cases I see couple more questionable ones. I wonder if we should introduce some printf()-like variation of ASSERT() to report arbitrary panic messages on debug builds, or just use PANIC() wrapped in ZFS_DEBUG.

[behlendorf]

Agreed we should gracefully handle these by default. Can you just address the two style warnings and force update the PR. You can run make checkstyle locally to make sure it's happy.

error: commit subject over 72 characters
error: missing "Signed-off-by"
error: commit message body contains line over 72 characters

[chrisperedun]

Style warnings addressed, thank you.