zbookmark_compare: avoid negative shift in block span calculation

[robn]

[Sponsors: TrueNAS]

Motivation and Context

Fixes #14777

Description

BP_SPANB subtracts SPA_BLKPTRSHIFT from the indirect block shift before shifting it. However, 0 is a legal value for ibs here, and the math is all unsigned, so the subtract can underflow, which is UB, and even if the compiler does resolve it to something negative, a negative shift is also UB.

Its unclear if this can be a correctness issue because it (hopefully!) only impacts the sort order. It's definitely not good though, and UBSAN has been seen to complain about.

This commit fixes it by only calling BP_SPANB if the ibs not zero, instead just using the block id, which is the intent. The asserts are extended to ensure that if it is non-zero, it is in a safe range.

How Has This Been Tested?

ZTS run completed and passed, so that's nice. However! #14777 mentions scrub with L2ARC, which we don't have a test for (unless its hidden down the back somewhere and I missed it). But it also might be coincidental. The examples all show a negative shifts that aren't 0 - SPA_BLKPTRSHIFT == -7, so either that's the UB at play, or its real and different things are calling zbookmark_compare() with weird small values for ibs. I suspect the former, but I don't really know this code path at all.

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)
• Quality assurance (non-breaking change which makes the code more robust against bugs)
• 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]

I feel like there is something weird going on. The existing IMPLY()'es should be sufficient to verify the ibs'es if the level is not 0. And even if the subtraction result end up negative for the level 0, then zero level multiplication should make it irrelevant. If in some way (due to non-debug build) we miss a case when ibs is zero for non-zero level, then you are just burying the problem deeper, not solving it.