Conversation
Keep host-visible and managed allocations on shared host/device backing, preserve queue-compatible staging, and synchronize managed globals through stream timelines. Propagate kernel resource and native runtime parameter metadata from HSACO. Bind device heap, hostcall, and printf services per dispatch through generic HAL host notifications, with immutable patch lists validated and prepared outside the dispatch hot path. Retain runtime resources with queued work and preserve runtime parameter metadata in replay streams. Reject buffered-printf graph instantiation until reusable graph command buffers can provide race-free per-launch FIFO bindings.
zjgarvey
left a comment
There was a problem hiding this comment.
Review — host-visible memory + runtime services
Strong, cleanly-layered change: the new HAL seam stays provider-neutral, the dispatch borrow/clone contract is honored at every retention boundary, the graph buffered-printf rejection is a precise enforced gate, and the printf grammar parser is well-defended. Two memory-safety issues to fix before merge, and the security-critical device-data machinery ships largely untested. Findings terse; happy to expand any.
Must fix
rocm_hostcall.c:694— the packet consumer's only host-side array bound isindex_mask, re-read each iteration from the device-writable shared buffer (no host-private copy); a device-side OOB write to that field → host OOB read (→ stdout) + OOB atomic write. Keeppacket_count-1host-private and mask with that.registry.c:115,139,1037+module.c:1039— the four managed-global transfers bypass the newdirect_transfer_mutexthat serializes every other direct transfer, sohipDeviceSynchronize/hipModuleGetGlobalcan race a concurrenthipMemcpy. Take the mutex (ideally via one shared helper).
Should fix
- Test coverage — the printf grammar, packet/message processing, both format-walkers, and
module_runtime_metadataparsing have ~no assertions; add table-driven parser + malformed packet/FIFO tests (that's exactly where both must-fix issues live). The host-notificationwait/wakecontract and the graph printf-rejection are also unpinned. executable.c:1195,1211,1258— runtime-parameter patch validation bounds againstmax()of the candidate layouts, not the actual custom-direct buffer; a reflected export dispatched custom-direct with a short constants buffer + a patch canmemcpypast it. Validate against the resolved destination size (or floor the custom-direct buffer tokernarg_sizefor all exports).module_runtime_metadata.c:819+stream.c:1454— the transient printf FIFO (not HAL-retained) is freed on error paths after the referencing dispatch is already submitted/recorded → device-side UAF. Block on the submitted value before release, or bind it to the command buffer's resource set.rocm_hostcall.c:693— the drain loop follows the devicenextchain with no cycle guard and never checksstop_requested; a cyclic link spins forever and deadlocks teardown. Bound bypacket_countand/or check the stop flag.- Duplication — two ~150-line printf format-walkers (
rocm_hostcall.c:362vsmodule_runtime_metadata.c:296) and the managed-global name/pointer-init helpers (registry.cvsmodule.c, ×3) must stay in lockstep by hand; factor each into one.
Minor — managed-global sync does an unbatched blocking D2H + full-symbol scan on every sync even with zero managed globals (registry.c:1003); async D2H allocates redundant full-size staging (memory.c:79); replay dispatch payload grew ~96B (an always-zero field) with no FILE_VERSION bump (format.h:585); %n truncates vs CLR's skip-and-continue (printf_format.h:164); printf width/precision forwarded unbounded to vfprintf (rocm_hostcall.c:329); unaligned ELF struct reads (module.c:431); uncited device-libs heap-header offsets (context.c:12); two opposite vtable-optionality conventions land in one PR; unused_heap_v1 name now contradicts its "patchable" comment.
Strengths — clean provider-neutral layering with no leakage either direction; executable_metadata/HSACO parsing is fully overflow-checked and validated byte-for-byte; ABI matches ROCm CLR precisely; and the one positional vtable initializer that would have shifted was caught and fixed.
Serialize context-wide direct transfers and consolidate managed-global publication so host-visible allocations and module registrations remain coherent under concurrency. Make module registration transactional and remove redundant transfer staging. Centralize printf formatting and hostcall packet traversal with strict bounds, cycle limits, and dispatch-lifetime resource retention. Validate runtime patches against the selected kernel argument layout, reject unsupported graph runtime services, and prevent missed host-notification wakeups. Keep replay streams free of process-local runtime values and resources. Require complete executable vtables with backend-owned unsupported implementations, and cover the new parsing, packet, patching, graph, and notification invariants.
zjgarvey
left a comment
There was a problem hiding this comment.
Re-review — hardening commit
Thorough pass — the earlier findings are resolved and now test-covered: the hostcall index_mask OOB (host-private packet iterator with mask + traversal budget), managed-transfer serialization (direct_transfer.c), the patch validator (bounds against the selected layout now, not max()), the printf-FIFO UAF (FIFO retained in the dispatch resource set), the two duplicated format-walkers (unified into printf_format.c), the cyclic-drain deadlock, %n/width bounds, and the replay payload (always-zero field removed + FILE_VERSION 3→4). Two issues the hardening introduced, plus a few carried forward:
Must fix (new)
local/loaders/{embedded_elf_loader.c:288, system_library_loader.c:398, static_library_loader.c:226, vmvx_module_loader.c:676}— dropping the generic null-guards madefunction_runtime_parameters/runtime_metadata_count/runtime_metadata_recordsmandatory, but the four local CPU vtables don't set them, so those public APIs now NULL-deref on the CPU backend. It's latent in-repo (callers gate onruntime_parameter_count != 0), but it contradicts the "complete vtables" goal. Add the three UNIMPLEMENTED stubs to each.base(or restore the null-guards).
Should fix (new)
pm4_command_buffer.c:3291— the validator refactor replaced the NULL-safe kernarg-size helper with a directdescriptor->kernarg_layout->kernarg_byte_length;kernarg_layoutis NULL forcustom_direct_onlyexports, and PM4 (unlike AQL/host_queue) doesn't reject them, so acustom_direct_onlyexport carrying runtime parameters NULL-derefs. Give PM4 the same rejection, or make the lookup NULL-safe.
Still open (carried forward, low)
registry.c:1000—synchronize_managed_datastill walksregistration->symbols[]/symbol_countwithoutregistry->mutex(realloc'd under it); themanaged_symbol_count == 0gate only covers the empty case, whilequery_variable/_lookuptake the lock for the same arrays.module.c:1239— the managed-global getter still does a blocking H2D on everyhipModuleGetGlobal(and now also contendsdirect_transfer_mutex); publish once at load and have the getter return the address.rocm_hostcall.c:238unbounded per-message accumulation (host-DoS, no OOB — add a per-message/table byte ceiling); no packet drain at teardown; anddirect_transfer.c's "context-wide" comment overclaims — the cross-context/D2DhipMemcpypaths (api.c:5568,5637,5727,5737) still bypass the mutex.
Nits — the reserved0 replay reader-guard (execute.c:2549) has no test; printf_parse_metadata_record is declared in printf_format.h but defined in module_runtime_metadata.c; WakeReleasesBlockedWait asserts an exact value after a single wait, so a spurious HSA wake would flake it.
Retain runtime argument resources until deferred dispatches complete and avoid clearing newly allocated host-visible storage. Move executable-global discovery into the HAL metadata contract so the AMDGPU loader owns HSACO parsing and streaming module setup consumes backend-neutral declarations. Split runtime-service code into scoped components, keep cache probes inline, remove dead physical-device state, and document notification wake semantics.
zjgarvey
left a comment
There was a problem hiding this comment.
Re-review
This closes the whole outstanding list — the PM4 kernarg_layout deref, the CPU-loader vtable gaps, and every carried-forward low (the managed-sync registry-array lock, the per-hipModuleGetGlobal transfer, unbounded printf accumulation, the teardown drain) — and the executable-global-discovery move + the runtime-service component split are clean and behavior-preserving. Three new issues this round block it, though; two are memory-safety regressions in the new code:
Must fix (new)
deferred_command_buffer.c:737— the new retention insert readscmd->runtime_parameters.count, butcmdis arena-allocated unzeroed andiree_hal_dispatch_config_clonewrites that field only whensource.runtime_parameters != NULL, so on the common (no-patch) path it's stale arena bytes. A reused block from a prior runtime-params dispatch then drivesresource_retainover freedpatches[i].resourcepointers → use-after-free / heap corruption (ASAN-invisible; the siblinghost_queue_pending_payload.cpath is safe because its record is memset-zeroed). Guard the insert oncmd->config.runtime_parameters, or zeroruntime_parameter_storage->countin the clone's NULL branch.rocm_hostcall_message.c:212,231—handle_printfdrops the status frommessage_allocate/message_appendwithoutiree_status_ignore(). The new byte caps make those returnRESOURCE_EXHAUSTEDon device input, so the cap itself leaks host memory unboundedly, andRejectsMessagesOverAllocationLimitfailsrocm_hostcall_testunder--config=asan(240 B leaked). Ignore the status at both sites, as the format-error path at:268already does.metal/executable.m:614— the metal executable vtable still omitsfunction_runtime_parameters/runtime_metadata_count/runtime_metadata_records(the generic wrappers are guard-free), so those public APIs NULL-deref on the Metal backend; the CPU-loader fix and this round'sglobal_*addition both skipped it. Add the three UNIMPLEMENTED stubs. (Latent — Metal isn't built in Linux CI.)
Should fix
registry.c:997— closing the managed-sync array race by holdingregistry->mutexacross the blocking, infinite-timeout D2H serializes every module register/unregister/lookup behind those device copies. Snapshot the managed set under the lock and transfer after releasing it.
Initialize cloned dispatch runtime parameter storage unconditionally so retained commands cannot observe stale arena state. Add coverage for both absent and present patch lists. Consume hostcall allocation failures, complete the Metal executable runtime metadata vtable, and snapshot managed buffers while holding the registry lock before performing blocking transfers.
zjgarvey
left a comment
There was a problem hiding this comment.
This clears all my focused review rounds with my agents and I did a pass myself in a few places.
I'm sure you'll want Ben to review, but I think it's at least safe to undraft.
benvanik
left a comment
There was a problem hiding this comment.
The HIP printf behavior is needed, and several pieces here—particularly the
HSACO parsing and printf format/packet work—are useful foundations. The current
integration cannot land, however, because it places an exceptional HIP runtime
service on ordinary HAL dispatch paths.
The generic runtime-parameter transport changes the representation of every
dispatch:
iree_hal_dispatch_config_t: 64 → 72 bytes- deferred dispatch record: 136 → 280 bytes
- PM4 dispatch record: 88 → 224 bytes
- AMDGPU pending operation: 320 → 464 bytes
The fixed 136-byte patch list is copied or zeroed at retention boundaries even
when no service exists, and the pending-operation union growth affects
non-dispatch operations as well. It also adds validation, retention, and
materialization branches throughout generic, AQL, and PM4 paths. Non-HIP
launches and ordinary HIP launches must not pay these costs.
direct_transfer_mutex must also be removed. It holds a context-wide lock
across chunked synchronous transfers and infinite waits without identifying a
shared resource that requires this serialization. Independent streams can
therefore block behind hundreds of submit/wait cycles. Any backend resource
requiring synchronization must own that synchronization itself.
The required integration boundary is:
- The hosting layer must explicitly opt into HIP runtime services. Loom and
other hosts must not link, initialize, inspect, or pay for them. - Within an opted-in HIP host, the zero-service dispatch path retains the
current dispatch configuration, deferred record, PM4 record,
pending-operation layout, and control flow. - HIP/AMDGPU runtime-service requirements are resolved during module loading
into immutable cold metadata. - Service-bearing kernels take a separate HIP-only direct-dispatch path using
a complete native kernarg image. - Context-stable hostcall and heap resources are initialized only for the
service path. - Buffered printf, when supported, owns a distinct FIFO per launch and retains
it through dispatch completion and ordered draining. - Runtime-service graph nodes remain an explicit unsupported case until their
per-launch ownership model exists. - No generic patch lists, PM4 runtime fields, replay reconstruction, or
capability probing viaUNIMPLEMENTEDstatuses are involved.
This PR currently has no real HIP-kernel end-to-end coverage for either default
hostcall printf or buffered printf. If it claims general printf support, both
modes need production-path tests, including return values, formatting,
repeated launches, lifetime during teardown, and operation with ordinary AQL
and PM4-configured streams.
Please do not extend the generic patch transport or transfer mutex further. The
useful parsing and service components can be carried into a cold-path
integration, but the present dispatch architecture needs replacement rather
than additional hardening.
I am starting a separate integration branch from current main that will
selectively carry forward the useful pieces and attempt the cold-path design
above. That work is evidence-driven and has no promised completion date; I
will link it here only once real HIP kernels exercise the vertical slice and
the ordinary dispatch path is verified unchanged.
|
FWIW: https://gist.github.com/AWoloszyn/b75d2da49a48ef932ab0250c98a20cd2
So I didn't want to enable a bad slow signal until we could get MOST Of it passing |
Keep host-visible and managed allocations on shared host/device backing, preserve queue-compatible staging, and synchronize managed globals through stream timelines.
Propagate kernel resource and native runtime parameter metadata from HSACO. Bind device heap, hostcall, and printf services per dispatch through generic HAL host notifications, with immutable patch lists validated and prepared outside the dispatch hot path.
Retain runtime resources with queued work and preserve runtime parameter metadata in replay streams. Reject buffered-printf graph instantiation until reusable graph command buffers can provide race-free per-launch FIFO bindings.