Verify embedded blkptr's in arc_read()

[behlendorf]

Motivation and Context

When attempting to recover a pool it was determined that it
somehow contained at least one damaged embedded block
pointer. Since this blkptr was always accessed during import
it effectively prevented the pool from being imported. By
detecting the damaged embedded block pointer and treating
it as a normal checksum error it was possible to import and
recover the pool.

Description

The block pointer verification check in arc_read() should also
cover embedded block pointers. While highly unlikely, accessing
a damaged block pointer can result in panic. To further harden
the code extend the existing check to include embedded block
pointers and add a comment explaining the rational for this
sanity check.

How Has This Been Tested?

Used to recover a pool which contained a damaged blkptr which
was protected by a valid checksum.

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.

[behlendorf]

cc: @tcaputi would you mind reviewing this.

[ahrens]

This seems fine. However, I took a look at zfs_blkptr_verify() and I don't understand this code:

	 * Do not verify individual DVAs if the config is not trusted. This
	 * will be done once the zio is executed in vdev_mirror_map_alloc.
	 */
	if (!spa->spa_trust_config)
		return (B_TRUE);

Shouldn't it be returning errors == 0, so that we are checking the non-pool-specific parts of the BP?

[behlendorf]

Shouldn't it be returning errors == 0, so that we are checking the non-pool-specific parts of the BP?

Good catch. Yes, it looks like this case was missed when commit bc67cba updated zfs_blkptr_verify to return if there was an error or not. I've included a fix for this in the updated PR.

[tonynguien]

Looks good to me.