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

Always perform bounds-checking in metaslab_free_concrete #17136

Merged
merged 1 commit into from
Mar 19, 2025

Conversation

asomers
Copy link
Contributor

@asomers asomers commented Mar 11, 2025

The vd->vdev_ms access can overflow due to on-disk corruption, not just due to programming bugs. So it makes sense to check its boundaries even in production builds.

Sponsored by: ConnectWise
Signed-off-by: Alan Somers [email protected]

Motivation and Context

Prevents an out-of-bounds memory access due to on-disk corruption. The out of bounds access usually results in a page fault, but its effects are unpredictable. Better to cleanly panic instead. The original corruption was probably caused by the same underlying cause as #16626, combined with inadequate sanity checking of the block pointers.

Description

Changes in existing ASSERT into a VERIFY

How Has This Been Tested?

Ran the ZFS test suite on FreeBSD

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:

The vd->vdev_ms access can overflow due to on-disk corruption, not just
due to programming bugs.  So it makes sense to check its boundaries even
in production builds.

Sponsored by:	ConnectWise
Signed-off-by:	Alan Somers <[email protected]>
Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

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

I have no objections, but wonder if this is the earliest place we can catch the corruption.

@amotin amotin added the Status: Code Review Needed Ready for review and testing label Mar 12, 2025
@amotin
Copy link
Member

amotin commented Mar 12, 2025

This slightly echoes with #17094.

@asomers
Copy link
Contributor Author

asomers commented Mar 12, 2025

I have no objections, but wonder if this is the earliest place we can catch the corruption.

I hope there's a better place to catch it. I plan to keep working on the problem. But in the meantime I think this change is correct. Maybe after #17094 there will be a better way to handle it.

@amotin amotin added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Mar 19, 2025
@amotin amotin merged commit d033f26 into openzfs:master Mar 19, 2025
22 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants