Skip to content

Move HRX pin: hrx-system to 2ab311496 (engine#420 length-less ELF readability fix) - #443

Merged
bong-water-water-bong merged 4 commits into
mainfrom
t420/pin-hrx-420
Oct 18, 2026
Merged

bong-water-water-bong merged 4 commits into
mainfrom
t420/pin-hrx-420

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Move HRX pin: third_party/hrx-system d3c731e1 -> 2ab311496, which is 1bit-MONSTER/hrx-system#7 on today's pin line (1bit/e3e6a976-pin-6).

Fixes #420. That issue's crash is not in the AMDGPU HAL on this line: hsaco_metadata.c already bounds-checks every read against data_length. The surviving defect is the length-less path in libhrx/src/binding/common/fat_binary.c: hipModuleLoadData carries no image length, so the extent is derived from the ELF header's own table geometry and was then proved with mincore(). mincore() reports that a mapping covers a page, not that the page is readable, so a header whose e_shoff/e_phoff points into a PROT_NONE guard page passed the proof and the table read faulted in iree_hal_streaming_fat_binary_describe_amdgpu_elf.

The fork PR replaces the proof with process_vm_readv() over one byte per spanned page, which goes through the same access checks a real read does and fails with EFAULT on the first unreadable page, so the span is rejected with INVALID_ARGUMENT before any table is dereferenced. Only the length-less branch changes; loads that carry a length, which is what the HRX ggml backend does, never reach it. The fork PR also adds the regression test RejectsLengthlessElfSpanningUnreadablePage and reports the crafted-header repro: SIGSEGV/exit 139 before, HIP error 200/exit 0 after, with a real gfx1151 code object still loading through hipModuleLoadData.

config/hrx-long-prompt-validated-pin.config moves with the pin, as tools/check_pins.py requires for an hrx-system pin move.

Verification at this pin:

  • tools/check_pins.py origin/main: ok third_party/hrx-system: d3c731e1d -> 2ab311496 is ahead.
  • Engine built with -DONEBIT_HRX=ON against this pin; tests/hrx_long_prompt.py (Qwen3-0.6B multi-ubatch retrieval, needle 7341) PASS twice on strixhalo gfx1151: PASS 7082-token prompt: expected 7341, got '7341'.
  • The fork PR's unit cover (fat_binary_test, 16/16 with the new case) and its crafted-header repro are in [HIP] Prove length-less ELF extents readable, not just mapped (engine#420) hrx-system#7.

Merge order: 1bit-MONSTER/hrx-system#7 first, then this pin move. Both are linked from #420.

🤖 Generated with Claude Code

…dability fix)

2ab311496 is 1bit-MONSTER/hrx-system PR #7 on today's pin line (1bit/e3e6a976-pin-6):
the length-less hipModuleLoadData path derives its extent from the ELF header and
checked the span with mincore(), which reports a mapped-but-unreadable (PROT_NONE)
page as present, so a header whose e_shoff points into one passed the check and the
table read faulted. The probe is now process_vm_readv(), one byte per spanned page,
so unreadable spans are rejected with INVALID_ARGUMENT before any table is read.
The changed path is only the length-less branch; loads that carry a length (which is
what the HRX ggml backend does) never reach it.

config/hrx-long-prompt-validated-pin.config moves with it, as check_pins.py requires
for an hrx-system pin move.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit 929555c)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Pin Update

The HRX pin has been updated from d3c731e1 to 2ab311496, which includes a fix for engine#420. This change addresses a crash caused by an incorrect ELF extent readability check in the hipModuleLoadData path. The new implementation uses process_vm_readv() to properly validate page readability, preventing faults when section table offsets point into PROT_NONE guard pages.

2ab3114966a4da12b5828e27f6cf705a5ebb7ee0
Hold Update

The HRX system hold has been updated to reference the new commit 2ab311496, which includes the fix for engine#420. This ensures that the long-prompt validation test passes and that the correct version of the HRX system is used for further development and testing.

# 2026-10-18, engine#420: the hold names 2ab311496, which is d3c731e1 plus hrx-system#7 -
# the length-less hipModuleLoadData path proves its derived ELF extent readable with
# process_vm_readv() instead of mincore(), so a section table offset pointing into a PROT_NONE
# page is refused instead of faulting. Nothing else moves: same pin line, same AMD core, the
# #1535 question stays where it was, and the long-prompt check passed at this commit
# (tests/hrx_long_prompt.py PASS twice, 7082-token prompt, needle 7341).
2ab3114966a4da12b5828e27f6cf705a5ebb7ee0

…es with the gitlink

The guard compares third_party/hrx-system with this file, and the pin move needs both. The
commit is the same held line (d3c731e1 + hrx-system#7), so the durable-hold reasoning and the
ROCm/hrx-system#1535 question are unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit 929555c

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

test-backend-ops -b HRX0 on the engine built at this pin (hrx-system 2ab3114966, -DONEBIT_HRX=ON), strixhalo gfx1151, one GPU slot, IREE_HAL_AMDGPU_LIBHSA_PATH from TheRock:

1141/1141 tests passed
2/2 backends passed

rc=0, Tctl 76 -> 74 C, log ~/wt/t420-tbo.log on strixhalo. That is the same pass count the 2026-10-17 bump test recorded at the parent pin (d3c731e1), so the fix changes no backend behaviour. The long-prompt gate also passed at this pin: PASS 7082-token prompt: expected 7341, got '7341', twice.

Also fixed in this PR: the hrx-system gitlink agrees with the hold file check, which was the first CI result on the branch - config/hrx-system-hold.config names the pinned commit too and now names 2ab311496 (same held line, so the hold reasoning is unchanged).

@bong-water-water-bong
bong-water-water-bong merged commit 8b6a0f4 into main Oct 18, 2026
9 of 10 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the t420/pin-hrx-420 branch October 18, 2026 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HRX: crafted ELF header with small e_shoff still crashes inside the AMDGPU HAL executable loader

1 participant