Skip to content

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
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Apr 3, 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
Reviewed by: Alek Pinchuk <[email protected]>
Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by:	Alan Somers <[email protected]>
Closes openzfs#17136
fuporovvStack pushed a commit to fuporovvStack/zfs that referenced this pull request Apr 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
Reviewed by: Alek Pinchuk <[email protected]>
Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by:	Alan Somers <[email protected]>
Closes openzfs#17136
fuporovvStack pushed a commit to fuporovvStack/zfs that referenced this pull request Apr 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
Reviewed by: Alek Pinchuk <[email protected]>
Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by:	Alan Somers <[email protected]>
Closes openzfs#17136
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Apr 16, 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
Reviewed by: Alek Pinchuk <[email protected]>
Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by:	Alan Somers <[email protected]>
Closes openzfs#17136
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