Fix endianness handling in {zfs,zvol}_replay_clone_range() - #18866
Conversation
The lr_bps field in lr_clone_range_t was added for brt_dmu_clone(). It needs to be byteswapped independently. Also, the assertion validating the length of this field is checked before the byteswap, which is another bug. Finally, the claim path has a comment asking if we should byteswap there too. That is unnecessary. arc_buf_fill() will byteswap the entire block. We delete that comment. Reported-by: Grok 4.5 Build Beta Signed-off-by: Richard Yao <richard@ryao.dev>
byteswap_uint64_array(p, sizeof(*p)) only converts the fixed header of a log record. Structured flexible array members such as blkptr_t lr_bps[] stay foreign-endian on opposite-endian ZIL replay (TX_CLONE_RANGE). Add a custom CodeQL query that flags header-only sizeof() byteswaps of types with multi-byte FAMs, while ignoring opaque uint8_t[] payloads and handlers that also swap the FAM. This patch was entirely written and verified by Grok 4.5 Build Beta. The actual check is more general purpose than just checking ZIL, but ZIL is the only place where I expect this sort of bug to appear, so I saw no reason to correct the LLM's proposed commit message beyond adding this addendum. Assisted-by: Grok 4.5 Build Beta Signed-off-by: Richard Yao <richard@ryao.dev>
|
I just pushed a second patch that adds a CodeQL check to catch bugs of this kind. It is somewhat notable because it is the first LLM-generated patch that I feel comfortable submitting in a PR after doing a light review. Being a CodeQL check rather than an actual code change makes it very low risk for us to merge. |
|
There appear to be more endianness bugs in the code, with at a few of them being regressions. I am not sure if I will be able to fix them this week. That said, this specific fix is ready to merge. |
| ASSERT3U(lr->lr_common.lrc_reclen, >=, sizeof (*lr)); | ||
|
|
||
| if (byteswap) | ||
| byteswap_uint64_array(lr, sizeof (*lr)); | ||
|
|
||
| ASSERT3U(lr->lr_common.lrc_reclen, >=, offsetof(lr_clone_range_t, | ||
| lr_bps[lr->lr_nbps])); |
There was a problem hiding this comment.
I'll need a refresher on what should byteswap what, but it seems here you are accessing lrc_reclen both before and after byteswap, which seems wrong.
github-actions
Bot
added
Status: Work in Progress
|
There appear to be many other endianness issues involving assertions in this code. Rather than just fix this, I am marking it as a draft until I have tackled all of them. |
3 participants
Motivation and Context
The lr_bps field in lr_clone_range_t was added for brt_dmu_clone(). It needs to be byteswapped independently. Also, the assertion validating the length of this field is checked before the byteswap, which is another bug. Finally, the claim path has a comment asking if we should byteswap there too. That is unnecessary. arc_buf_fill() will byteswap the entire block.
Description
We add the missing byteswaps and ensure that the assertion that reads a byteswapped field occurs after the byteswap. We also delete the unnecessary comment from the claim path.
Grok 4.5 Build Beta found this bug.
How Has This Been Tested?
The buildbot can test it.
Types of Changes
Checklist
Signed-off-by.