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

Some additional send stream validity checking #6396

Merged
merged 1 commit into from
Jul 26, 2017

Conversation

nedbass
Copy link
Contributor

@nedbass nedbass commented Jul 24, 2017

Description

Check in the DMU whether an object record in a send stream being
received contains an unsupported dnode slot count, and return an
error if it does. Failure to catch an unsupported dnode slot count
would result in a panic when the SPA attempts to increment the
reference count for the large_dnode feature and the pool has the
feature disabled. This is not normally an issue for a well-formed
send stream which would have the DMU_BACKUP_FEATURE_LARGE_DNODE flag
set if it contains large dnodes, so it will be rejected as
unsupported if the required feature is disabled. This change adds a
missing object record field validation.

Add missing stream feature flag checks in
dmu_recv_resume_begin_check().

Consolidate repetitive comment blocks in dmu_recv_begin_check().

Update zstreamdump to print the dnode slot count (dn_slots) for an
object record when running in verbose mode.

Motivation and Context

Kernel-side hardening of backup stream receive code.

How Has This Been Tested?

I generated a backup stream that contained large dnodes but was missing the DMU_BACKUP_FEATURE_LARGE_DNODE flag. I then tried to receive this stream in a pool
created with -o version=28. Without this patch, the node panicked due to a failed
VERIFY in the feature reference count code. With this patch, the stream is safely rejected
by the kernel.

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)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commit messages are properly formatted and contain Signed-off-by.
  • Change has been approved by a ZFS on Linux member.

@ahrens
Copy link
Member

ahrens commented Jul 25, 2017

This change is fine, but I think the kernel also needs to check this in dmu_recv_resume_begin_check(), where it checks the other featureflags.

Check in the DMU whether an object record in a send stream being
received contains an unsupported dnode slot count, and return an
error if it does. Failure to catch an unsupported dnode slot count
would result in a panic when the SPA attempts to increment the
reference count for the large_dnode feature and the pool has the
feature disabled. This is not normally an issue for a well-formed
send stream which would have the DMU_BACKUP_FEATURE_LARGE_DNODE flag
set if it contains large dnodes, so it will be rejected as
unsupported if the required feature is disabled. This change adds a
missing object record field validation.

Add missing stream feature flag checks in
dmu_recv_resume_begin_check().

Consolidate repetitive comment blocks in dmu_recv_begin_check().

Update zstreamdump to print the dnode slot count (dn_slots) for an
object record when running in verbose mode.

Signed-off-by: Ned Bass <bass6@llnl.gov>
@nedbass
Copy link
Contributor Author

nedbass commented Jul 25, 2017

@ahrens I added checks for both large block and large dnode feature flags.

@nedbass nedbass changed the title Validate dnode slot count when receiving object Some additional send stream validity checking Jul 25, 2017
@ahrens
Copy link
Member

ahrens commented Jul 25, 2017

@nedbass Thanks. We have the check for LARGE_BLOCKS there in Delphix but looks like that fix was made as part of a larger restructuring that @pcd1193182 is still working on upstreaming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants