Repository navigation
fix(rocr): fix out-of-bounds access in hsa_amd_signal_wait_any - #8564
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
cfreeamd
left a comment
cfreeamd
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd)
Checked hsa_ext_amd.cpp, signal.cpp, and the new signal_wait_multi_test.cpp. The three-bug fix (conds/values misalignment, OOB-on-timeout in wait-any, wrong satisfying_value slot in signal.cpp) is correct and the tests directly reproduce the failure scenarios. One nit and one open question inline.
19c3538 to
b41d7d3
Compare
cfreeamd
left a comment
cfreeamd
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd) — follow-up after author update
Re-reviewed after @atgutier's approval. The three-bug fix (conds/values misalignment, OOB-on-timeout in wait-any, and the wrong satisfying-value slot in signal.cpp) still looks correct. Only lingering item is the CI-coverage question below — non-blocking.
Correction: @atgutier and @cfreeamd were right — the test was not wired up@atgutier asked whether this actually adds the test to our active tests so it's built/run, and I answered "My understanding is yes (at least it is actively run locally)." That answer was wrong. @cfreeamd's follow-up about CI filter groups was pointing at the same real problem. Apologies for the bad information — I've verified it properly and fixed it in 83c0458. What was actually wrong
The fix (83c0458)Reimplemented the tests as a
The orphaned Also addressed @cfreeamd's nit — the intentionally-null Verified on hardware (gfx1201)Built Against a pre-fix runtime, each test run in isolation:
Against a runtime built from this tree: 3/3 pass. Broader sanity Suggested follow-up
cc @atgutier @cfreeamd — since this adds a commit on top of the approved head, this probably needs a fresh look. |
cfreeamd
left a comment
cfreeamd
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd) — follow-up after author update
Both prior points are resolved in 83c0458:
- CI coverage [Question]: confirmed the old
utils_testpath was never built/run anywhere. The three regression tests now live inrocrtst/suites/functional/signal_wait_multi.cc, are registered intest_common/main.cc(rocrtstFunc.Signal_Wait_Any_Nonzero_Index,Signal_Wait_Any_Compacted_Conds_Values,Signal_Wait_All_Satisfying_Values), and are wired intotest_categories.yamlfilter groups — so they now get real rocrtst64 CI coverage. - [Nit]:
signals[1]now carries the explicit comment explaining it is intentionally null to exercise the NULL/invalid-skip and conds/values compaction path.
Thanks for chasing down the dead-CMake investigation.
83c0458 to
687a7fe
Compare
216acc5 to
8a8f052
Compare
@cfreeamd / @shwetagkhatri can one of you stamp this change? |
|
FYI: I hit this bug and mistakenly attributed it to the same cause as #5071. So, the unrelated bug has been fixed and closed, but not the wait_any crash. I thought I'd create a new PR so it didn't get forgotten, but then I found this one instead. Even better! There's a test case attached to my comment in the above issue which might be helpful. HTH! |
|
Claude suggested a follow up on "might be helpful": excuse his robotic enthusiasm... I built current
So roughly 93% of runs abort on Environment: gfx942, ROCm 7.13.0 ( On the reproducer vs. the new unit testsI think they're complementary rather than redundant, and it's worth being explicit
That asymmetry is, I think, why this survived to production. The damage scales with the Either of these would close the gap:
It's HSA-only ( |
7726711 to
e71022e
Compare
|
Just following up here: what is needed to get the approval for this PR to be merged? Do we need to add another test/reproducer @jtb20? |
Sorry for the delay. I was on PTO for a couple of weeks and missed this ping. |
84d82f5 to
54605eb
Compare
satisfying_value_vec was allocated with a fixed size of 1, but core::Signal::WaitMultiple() can write to and return any index up to valid_signals.size()-1 (the wait_on_all=false / "wait any" case returns as soon as the first satisfying signal is found, at whatever index it occurs at). Whenever the satisfying signal was not at index 0, this caused an out-of-bounds write into satisfying_value_vec inside WaitMultiple(), and then hsa_amd_signal_wait_any() unconditionally read back index 0 (satisfying_value_vec.at(0)) regardless of which index actually satisfied, returning a garbage/wrong value to the caller. Also guard the uint32_t(-1) timeout/invalid-signal sentinel returned by WaitMultiple(), which would otherwise index valid_signal_ids out-of-bounds when mapping back to the caller's original array position. Sanity-checked hsa_amd_signal_wait_all's analogous code path; it already sizes its output vector correctly (to valid_signal_count), so no equivalent fix is needed there. Found while prototyping a batched-signal-wait completion mechanism for rocprofiler-sdk's Queue Interposition feature (ROCM-27290), which calls hsa_amd_signal_wait_any with a multi-signal batch where the satisfying index is very commonly > 0. Regression coverage: - Added SignalWaitMultiTest.WaitAnyReportsNonzeroSatisfyingIndex, which uses real host-created signals with the satisfying signal at index 1 and verifies both the returned original index and satisfying value.
…ct violation in signal wait-multiple Two additional, related, pre-existing bugs found by review in the same area as the hsa_amd_signal_wait_any out-of-bounds fix above. 1. hsa_amd_signal_wait_any() and hsa_amd_signal_wait_all() build a compacted valid_signals/valid_signal_ids array by skipping NULL and invalid signals, but were passing the caller's original, unfiltered conds/values pointers straight through to core::Signal::WaitMultiple(), which indexes conds[i]/values[i] using the COMPACTED position. If any signal earlier in the caller's array was NULL/invalid, every subsequent valid signal was evaluated against the wrong condition/ value from the unfiltered array. Fixed by compacting conds/values alongside valid_signals in both functions. 2. core::Signal::WaitMultiple()'s wait-any (wait_on_all=false) implementation violated its own documented contract (signal.h: "if wait_on_all is false ... this will contain only the first satisfying value", implying slot 0) by actually writing satisfying_values[i] at the local satisfying index instead of satisfying_values[0]. This caused a separate real out-of-bounds write at runtime.cpp's AsyncEventsLoop, which correctly follows the documented contract with a 1-element vector, whenever the satisfying signal's index was not 0. Fixed by writing satisfying_values[0] in the wait-any branch (the wait_on_all=true branch, which needs a per-signal value for every signal, is unchanged). This lets hsa_amd_signal_wait_any() be simplified back to a 1-element satisfying_value_vec, reading slot 0 directly; runtime.cpp needed no change since it already assumed the correct (now-honored) contract. Testing: - Code review pass found no remaining blockers/majors. - rocr-runtime + rocprofiler-sdk rebuilt and reinstalled; ctest -R queue_interposition: 14/14 passed (rocprofiler-sdk's batched-wait Queue Interposition waiter, a downstream consumer of hsa_amd_signal_wait_any, see companion PR). - Phase 1 sanity benchmark unchanged (~1.33x vs QI-disabled), as expected since Queue Interposition's own waiter never passes NULL/invalid signals in its batch. - 10/10 repeated tight-loop shutdown-stress runs clean -- this specifically re-exercises the non-zero-satisfying-index scenario bug 2 was about (Queue Interposition's dedicated wake signal is always appended at the end of its wait batch), with no regression. - No existing rocr-runtime test coverage was found exercising either bug case; adding targeted regression coverage is a worthwhile follow-up but out of scope for this fix. Regression coverage: - Added SignalWaitMultiTest.WaitAnyCompactsConditionsAndValues, which places a NULL handle between valid signals with distinct condition/value pairs and verifies the valid signal at index 2 returns value 22. - Added SignalWaitMultiTest.WaitAllReportsSatisfyingValues as a host-only wait-all non-regression check.
Rename first_satysifying_signal_idx to first_satisfying_signal_idx and note that satisfying_values is only partially filled when hsa_amd_signal_wait_all times out, matching the guard already added for wait_any.
The regression tests added with this fix were placed in rocrtst/common/utils_test/, which is dead CMake: nothing add_subdirectory()s it, ROCRTST_UTIL_TEST_NAME is undefined, and force-including that CMakeLists.txt is a hard configure error. The rocrtst CMake project root (suites/test_common) gathers sources with aux_source_directory(), which is not recursive, so common/utils_test could never be compiled into rocrtst64. The tests were therefore never built or run, locally or in CI. Reimplement them as a TestBase-derived suite under suites/functional, which aux_source_directory() does sweep into rocrtst64 - the binary that test/therock/test_rocrtst.py actually invokes in CI. No CMake change is required. Register the tests in suites/test_common/main.cc under the rocrtstFunc naming convention and add them to all four tiers of test_categories.yaml so they carry ctest labels: rocrtstFunc.Signal_Wait_Any_Nonzero_Index rocrtstFunc.Signal_Wait_Any_Compacted_Conds_Values rocrtstFunc.Signal_Wait_All_Satisfying_Values hsa_init()/hsa_shut_down() are now owned by the framework rather than the test, and created signals are destroyed in Close() before TestBase::Close() tears down the runtime. Revert the dead utils_test/CMakeLists.txt entry and delete the orphaned source file. Verified on gfx1201 by swapping only libhsa-runtime64 via LD_LIBRARY_PATH (the binary uses RUNPATH, so the override applies). Against a pre-fix runtime, Signal_Wait_Any_Nonzero_Index fails with satisfying_value 0 instead of 41 and Signal_Wait_Any_Compacted_ Conds_Values segfaults, while Signal_Wait_All_Satisfying_Values passes. Against a runtime built from this tree, all three pass, and rocrtstFunc.Signal_*:rocrtst.Test_Example is 8/8.
54605eb to
eb5b8fc
Compare
The Claude-generated test might be useful to add so this doesn't regress but it shouldn't be a blocker, IMO. This is a serious bug, and we should prioritise getting the fix committed. Thanks! |
JIRA ID : ROCM-27290
## Summary
Fixes an out-of-bounds read/write in `hsa_amd_signal_wait_any` that
occurs whenever the satisfying signal in a multi-signal batch is at any
index other than 0, plus two related condition/value-compaction and
wait-any value-slot contract bugs found by review in the same code path.
## Root Cause
`hsa_amd_signal_wait_any` allocated `satisfying_value_vec` with a fixed
size of `1`:
```cpp
std::vector<hsa_signal_value_t> satisfying_value_vec(1);
uint32_t satisfying_signal_idx =
core::Signal::WaitMultiple(valid_signals.size(), valid_signals.data(), conds, values,
timeout_hint, wait_hint, satisfying_value_vec, false);
...
if (satisfying_value) *satisfying_value = satisfying_value_vec.at(0);
```
But `core::Signal::WaitMultiple()` (called with `wait_on_all=false`,
i.e. "wait for any") writes `satisfying_values[i] = value` and returns
`i` directly, where `i` can be any index in `[0, valid_signals.size())`
— whichever signal is satisfied first. Whenever `i != 0`,
`WaitMultiple()` writes out of bounds into the 1-element vector, and the
subsequent `.at(0)` read (even if the OOB write somehow didn't corrupt
anything) returns the wrong value regardless of which signal actually
satisfied.
This also missed handling `WaitMultiple()`'s `uint32_t(-1)`
timeout/invalid-signal sentinel return, which would separately index
`valid_signal_ids` out of bounds when mapping the result back to the
caller's original array position.
Additionally, two related bugs were found by review in the same area:
1. `hsa_amd_signal_wait_any()`/`hsa_amd_signal_wait_all()` built a
compacted `valid_signals`/`valid_signal_ids` array by skipping
NULL/invalid signals, but passed the caller's original, unfiltered
`conds`/`values` pointers straight through to `WaitMultiple()`, which
indexes them in the COMPACTED position — misaligning every valid signal
after a skipped one.
2. `core::Signal::WaitMultiple()`'s wait-any branch violated its own
documented contract (`satisfying_values` should contain only the first
satisfying value at slot 0) by writing `satisfying_values[i]` at the
local satisfying index instead — a separate real OOB write hit by
`runtime.cpp`'s `AsyncEventsLoop`.
## Fix
- Size `satisfying_value_vec` to `valid_signals.size()` (matching
`WaitMultiple()`'s local index space), not a fixed `1`.
- Return early on the `uint32_t(-1)` sentinel before doing any indexing.
- Read the satisfying value using `WaitMultiple()`'s *local* index into
`satisfying_value_vec`, then map the *index returned to the caller*
through `valid_signal_ids` back to the caller's original array position
— these two index spaces are different and must not be conflated (the
fix separates them explicitly with comments).
- Compact `conds`/`values` alongside `valid_signals` in both
`hsa_amd_signal_wait_any` and `hsa_amd_signal_wait_all`.
- Make `Signal::WaitMultiple()`'s wait-any branch write
`satisfying_values[0]` per its documented contract, letting
`hsa_amd_signal_wait_any()` read a 1-element vector directly.
Sanity-checked `hsa_amd_signal_wait_all`'s analogous code path; it
already sized its output vector correctly (`valid_signal_count`), so no
equivalent fix was needed for that specific issue.
## How this was found
Discovered while prototyping a batched-signal-wait completion mechanism
for rocprofiler-sdk's Queue Interposition feature (tracked separately,
ROCM-27290 investigation), which calls `hsa_amd_signal_wait_any` with a
multi-signal batch where the satisfying index is very commonly greater
than 0 (a dedicated "wake" signal is always appended at the end of the
batch). Any existing caller of `hsa_amd_signal_wait_any` with more than
one signal in its batch is exposed to this bug today, independent of
that follow-on work.
## Testing
- Added a new gtest regression suite, `SignalWaitMultiTest`, in
`rocrtst/common/utils_test/signal_wait_multi_test.cpp`:
- `WaitAnyReportsNonzeroSatisfyingIndex` — regresses the
`satisfying_value_vec` OOB fix (satisfying signal at index 1 of 3).
- `WaitAnyCompactsConditionsAndValues` — regresses the `conds`/`values`
compaction fix (NULL signal interleaved before valid ones with distinct
condition/value pairs).
- `WaitAllReportsSatisfyingValues` — non-regression check for
`hsa_amd_signal_wait_all`.
- Confirmed these fail against the pre-fix library
(`WaitAnyReportsNonzeroSatisfyingIndex` returns `satisfying_value=0`
instead of the expected `41`) and pass 3/3 against a freshly rebuilt
library containing this PR's fix.
- Exercised indirectly via a downstream rocprofiler-sdk prototype that
calls `hsa_amd_signal_wait_any` with multi-signal batches where the
satisfying index is >0 in the common case; before this fix, that
prototype produced incorrect/garbage wake-signal values, consistent with
this bug. After the fix: correct behavior confirmed across `ctest` runs
and repeated stress runs.
## Related
Related to ROCM-27290 (rocprofiler-sdk Queue Interposition performance
investigation) but is a standalone, independently-valid bug fix.
</content>
…8564) JIRA ID : ROCM-27290 ## Summary Fixes an out-of-bounds read/write in `hsa_amd_signal_wait_any` that occurs whenever the satisfying signal in a multi-signal batch is at any index other than 0, plus two related condition/value-compaction and wait-any value-slot contract bugs found by review in the same code path. ## Root Cause `hsa_amd_signal_wait_any` allocated `satisfying_value_vec` with a fixed size of `1`: ```cpp std::vector<hsa_signal_value_t> satisfying_value_vec(1); uint32_t satisfying_signal_idx = core::Signal::WaitMultiple(valid_signals.size(), valid_signals.data(), conds, values, timeout_hint, wait_hint, satisfying_value_vec, false); ... if (satisfying_value) *satisfying_value = satisfying_value_vec.at(0); ``` But `core::Signal::WaitMultiple()` (called with `wait_on_all=false`, i.e. "wait for any") writes `satisfying_values[i] = value` and returns `i` directly, where `i` can be any index in `[0, valid_signals.size())` — whichever signal is satisfied first. Whenever `i != 0`, `WaitMultiple()` writes out of bounds into the 1-element vector, and the subsequent `.at(0)` read (even if the OOB write somehow didn't corrupt anything) returns the wrong value regardless of which signal actually satisfied. This also missed handling `WaitMultiple()`'s `uint32_t(-1)` timeout/invalid-signal sentinel return, which would separately index `valid_signal_ids` out of bounds when mapping the result back to the caller's original array position. Additionally, two related bugs were found by review in the same area: 1. `hsa_amd_signal_wait_any()`/`hsa_amd_signal_wait_all()` built a compacted `valid_signals`/`valid_signal_ids` array by skipping NULL/invalid signals, but passed the caller's original, unfiltered `conds`/`values` pointers straight through to `WaitMultiple()`, which indexes them in the COMPACTED position — misaligning every valid signal after a skipped one. 2. `core::Signal::WaitMultiple()`'s wait-any branch violated its own documented contract (`satisfying_values` should contain only the first satisfying value at slot 0) by writing `satisfying_values[i]` at the local satisfying index instead — a separate real OOB write hit by `runtime.cpp`'s `AsyncEventsLoop`. ## Fix - Size `satisfying_value_vec` to `valid_signals.size()` (matching `WaitMultiple()`'s local index space), not a fixed `1`. - Return early on the `uint32_t(-1)` sentinel before doing any indexing. - Read the satisfying value using `WaitMultiple()`'s *local* index into `satisfying_value_vec`, then map the *index returned to the caller* through `valid_signal_ids` back to the caller's original array position — these two index spaces are different and must not be conflated (the fix separates them explicitly with comments). - Compact `conds`/`values` alongside `valid_signals` in both `hsa_amd_signal_wait_any` and `hsa_amd_signal_wait_all`. - Make `Signal::WaitMultiple()`'s wait-any branch write `satisfying_values[0]` per its documented contract, letting `hsa_amd_signal_wait_any()` read a 1-element vector directly. Sanity-checked `hsa_amd_signal_wait_all`'s analogous code path; it already sized its output vector correctly (`valid_signal_count`), so no equivalent fix was needed for that specific issue. ## How this was found Discovered while prototyping a batched-signal-wait completion mechanism for rocprofiler-sdk's Queue Interposition feature (tracked separately, ROCM-27290 investigation), which calls `hsa_amd_signal_wait_any` with a multi-signal batch where the satisfying index is very commonly greater than 0 (a dedicated "wake" signal is always appended at the end of the batch). Any existing caller of `hsa_amd_signal_wait_any` with more than one signal in its batch is exposed to this bug today, independent of that follow-on work. ## Testing - Added a new gtest regression suite, `SignalWaitMultiTest`, in `rocrtst/common/utils_test/signal_wait_multi_test.cpp`: - `WaitAnyReportsNonzeroSatisfyingIndex` — regresses the `satisfying_value_vec` OOB fix (satisfying signal at index 1 of 3). - `WaitAnyCompactsConditionsAndValues` — regresses the `conds`/`values` compaction fix (NULL signal interleaved before valid ones with distinct condition/value pairs). - `WaitAllReportsSatisfyingValues` — non-regression check for `hsa_amd_signal_wait_all`. - Confirmed these fail against the pre-fix library (`WaitAnyReportsNonzeroSatisfyingIndex` returns `satisfying_value=0` instead of the expected `41`) and pass 3/3 against a freshly rebuilt library containing this PR's fix. - Exercised indirectly via a downstream rocprofiler-sdk prototype that calls `hsa_amd_signal_wait_any` with multi-signal batches where the satisfying index is >0 in the common case; before this fix, that prototype produced incorrect/garbage wake-signal values, consistent with this bug. After the fix: correct behavior confirmed across `ctest` runs and repeated stress runs. ## Related Related to ROCM-27290 (rocprofiler-sdk Queue Interposition performance investigation) but is a standalone, independently-valid bug fix. </content>
TheRock Submodule Bump ActivityNewest first
|
…ks, DDP AC repair, profiler (#1314) First of a series; the following PRs build on the patch framework added here. **What changes** - The backend #835 added as `nemo_automodel` is renamed `automodel`, after the project it runs: `primus/backends/automodel/`, its configs, examples, prepare hook and tests, `AutoModelAdapter` / `AutoModelPretrainTrainer`, and `framework: automodel`. Old configs (`framework: nemo_automodel`), `--backend nemo_automodel` and imports of the four modules #835 published keep working, with a deprecation warning. The upstream package keeps its name, `nemo_automodel`. - `--backend` works. On `main` it never did, under any name: the launch line passes it to the prepare hook and to `train pretrain`, neither declared it, and argparse took it as an abbreviation of `--backend_path`, so the run looked for a checkout named after the backend. The train parsers (`primus/core/launcher/parser.py`) now declare `--backend`, and the AutoModel hook no longer accepts abbreviated options. The other backends' hooks still do; that is left for a separate change. - `third_party/Automodel` moves to NVIDIA-NeMo/Automodel main (`bd1ca5a0`). That pin requires `transformers==5.15.1` and `diffusers>=0.39.0` in the image. - The prepare hook enforces the pin. It used to keep any importable AutoModel; now it keeps one only if it is the commit `primus/_thirdparty.lock` pins and its installed metadata matches, and otherwise installs `third_party/Automodel` over it (ROCm packages held), logging every package it added, removed or moved. `AUTOMODEL_REINSTALL=0` keeps any copy unless `BACKEND_PATH`/`AUTOMODEL_PATH` names one explicitly, and `PRIMUS_SKIP_PIP=1` installs nothing, as in the other backends' hooks. A root container over a host-owned checkout no longer fails the install on git's ownership check. - The same constraints hold diffusers below 0.41, which the pin leaves open: diffusers 0.41.0 imports `torchao.quantization.FqnToConfig` whenever torchao is installed, the image's torchao lacks it, and so no diffusers model would load. An installed copy above the cap counts as stale and is reinstalled under it. - On a ROCm torch the hook removes the CUDA-only `nvidia-cutlass-dsl` wheels the install pulls in (through `quack-kernels`). The diffusion path never imports them, but torch 2.14's Inductor imports `cutlass` whenever it is installed, and its MLIR bindings collide with FlyDSL's, which Primus-Turbo loads, so the first compile aborts. The install removes them before it logs the version changes, and the hook checks again on every launch, so containers set up before this change are repaired too. - The backend gets a patch package: every `patches/*_patches.py` registers through the existing Primus patch framework (`@register_patch`, `PRIMUS_PATCHES`), as the megatron, maxtext and megatron_bridge backends already do. Adding a feature means adding a file. The trainer runs the patches before it builds the AutoModel config, and model-specific patches apply only to their model. - Settings are config fields: top-level `primus_*` sections of the module config, removed before the config reaches AutoModel. No new environment variables. Each patch module declares the keys its feature reads, and the trainer warns about any `primus_*` key no module declared, so a typo no longer turns a feature off silently. - `ddp.activation_checkpointing` now takes effect. AutoModel's parser moves it out of `DDPConfig`, which is the only place the DDP path reads it; the repair copies it back and is a no-op once upstream forwards it. It applies only when the `ddp:` section asks for checkpointing, so FSDP2 runs do not log it as applied. - The Wan preset uses the pinned diffusers' registered `flash` attention backend. Its previous `aiter` value raises at startup on this pin; there is no automatic fallback. - `primus_profiler.enabled: true` runs the diffusion train loop under `torch.profiler`, one trace per rank under `diffusion_profile/` in the experiment directory. On ROCm 10.1 images (`rocm/primus:v26.8`), GPU tracing of FSDP2 runs can hang in rocprofiler-sdk; until an image carries the fix (ROCm/rocm-systems#9984 and ROCm/rocm-systems#8564, in ROCm 10.2), set `ROCPROFILER_QUEUE_INTERPOSITION=0` for profiled runs. - The trainer accepts the keyword arguments `main`'s core runtime passes (`module_name=` and the rest); #835's `__init__(self, backend_args)` raised `TypeError` on that path. **Why the bump.** Typed recipe config and the `optimizer:` schema (NVIDIA-NeMo/Automodel#3122), diffusion context parallelism (NVIDIA-NeMo/Automodel#3157), and per-model `ModelParallelizer` sidecars (NVIDIA-NeMo/Automodel#4032), which let a model repair subclass upstream's parallelization instead of replacing it. **Breaking config schema** (AutoModel rejects the old keys at startup) | old | new | |---|---| | `optim.learning_rate` | `optimizer.lr` | | `optim.optimizer.*` | `optimizer.*` | | `step_scheduler.log_every` | `step_scheduler.log_remote_every_steps` | The in-tree Wan configs are migrated; out-of-tree configs need the same change. **How to review.** The rename is mostly moves. The old name is resolved from one table, `primus/backends/backend_aliases.py`, which the config parser, `prepare_experiment.sh` and the projection CLI read; `primus/backends/nemo_automodel/` keeps the old import paths and registry name. GitHub shows the moved adapter, trainer, `argument_builder.py` and `__init__.py` as new files, since their old paths keep the aliases. To see what changed in them: `git diff origin/main:primus/backends/nemo_automodel/nemo_automodel_adapter.py HEAD:primus/backends/automodel/automodel_adapter.py`, and likewise for the others. Then start at `patches/__init__.py`, `options.py` and the trainer's `init()`, then `distributed/ddp_activation_checkpointing.py`, then the prepare hook. Unlike the other backends, the hook installs from `prepare.py` rather than an `00_install_requirements.sh`, as #835's already did: enforcing the pin needs the checks in Python. **Test plan** - `pytest tests/unit_tests/backends/automodel` — stubbed tests need no AutoModel; contract tests run against the pinned AutoModel when it is importable. The example-config smoke test now covers the `automodel` experiments; an old-style `framework: nemo_automodel` experiment, the old imports and a registry lookup of the old name resolve to the renamed backend, and `train pretrain` takes `--backend` as itself. Every setting the backend reads is declared, and the shipped configs set no other. The tests pass in any order: `main`'s `tests/unit_tests/core/patches` empty the global patch registry without restoring it, so the backend's conftest rebuilds it for each test. - `pytest tests/runner/test_prepare_hooks.py` (not run by CI): the hook does not read `--backend` as `--backend_path`. - Wan and FLUX.1 short training runs through `primus-cli` on 8× MI355X; the profiler writes one trace per rank into the experiment directory; `--backend automodel` and `--backend nemo_automodel` launches train, the old name with one deprecation warning, and so does an old-name config; an old `optim.` key stops the run at startup with AutoModel's migration message.. A misspelled `primus_*` key on the launch line gives one warning naming it; the DDP repair applies on the DDP path and not on FSDP2. - From the unmodified `rocm/primus:v26.8` image: the first launch installs the pin and trains, and with an older AutoModel installed first, the hook replaces it and training passes. With diffusers 0.41.0 installed first, the hook brings it under the cap and a diffusers model imports. --------- Co-authored-by: mvstrauss <mvonstra@amd.com>
JIRA ID : ROCM-27290
Summary
Fixes an out-of-bounds read/write in
hsa_amd_signal_wait_anythat occurs whenever the satisfying signal in a multi-signal batch is at any index other than 0, plus two related condition/value-compaction and wait-any value-slot contract bugs found by review in the same code path.Root Cause
hsa_amd_signal_wait_anyallocatedsatisfying_value_vecwith a fixed size of1:But
core::Signal::WaitMultiple()(called withwait_on_all=false, i.e. "wait for any") writessatisfying_values[i] = valueand returnsidirectly, whereican be any index in[0, valid_signals.size())— whichever signal is satisfied first. Wheneveri != 0,WaitMultiple()writes out of bounds into the 1-element vector, and the subsequent.at(0)read (even if the OOB write somehow didn't corrupt anything) returns the wrong value regardless of which signal actually satisfied.This also missed handling
WaitMultiple()'suint32_t(-1)timeout/invalid-signal sentinel return, which would separately indexvalid_signal_idsout of bounds when mapping the result back to the caller's original array position.Additionally, two related bugs were found by review in the same area:
hsa_amd_signal_wait_any()/hsa_amd_signal_wait_all()built a compactedvalid_signals/valid_signal_idsarray by skipping NULL/invalid signals, but passed the caller's original, unfilteredconds/valuespointers straight through toWaitMultiple(), which indexes them in the COMPACTED position — misaligning every valid signal after a skipped one.core::Signal::WaitMultiple()'s wait-any branch violated its own documented contract (satisfying_valuesshould contain only the first satisfying value at slot 0) by writingsatisfying_values[i]at the local satisfying index instead — a separate real OOB write hit byruntime.cpp'sAsyncEventsLoop.Fix
satisfying_value_vectovalid_signals.size()(matchingWaitMultiple()'s local index space), not a fixed1.uint32_t(-1)sentinel before doing any indexing.WaitMultiple()'s local index intosatisfying_value_vec, then map the index returned to the caller throughvalid_signal_idsback to the caller's original array position — these two index spaces are different and must not be conflated (the fix separates them explicitly with comments).conds/valuesalongsidevalid_signalsin bothhsa_amd_signal_wait_anyandhsa_amd_signal_wait_all.Signal::WaitMultiple()'s wait-any branch writesatisfying_values[0]per its documented contract, lettinghsa_amd_signal_wait_any()read a 1-element vector directly.Sanity-checked
hsa_amd_signal_wait_all's analogous code path; it already sized its output vector correctly (valid_signal_count), so no equivalent fix was needed for that specific issue.How this was found
Discovered while prototyping a batched-signal-wait completion mechanism for rocprofiler-sdk's Queue Interposition feature (tracked separately, ROCM-27290 investigation), which calls
hsa_amd_signal_wait_anywith a multi-signal batch where the satisfying index is very commonly greater than 0 (a dedicated "wake" signal is always appended at the end of the batch). Any existing caller ofhsa_amd_signal_wait_anywith more than one signal in its batch is exposed to this bug today, independent of that follow-on work.Testing
SignalWaitMultiTest, inrocrtst/common/utils_test/signal_wait_multi_test.cpp:WaitAnyReportsNonzeroSatisfyingIndex— regresses thesatisfying_value_vecOOB fix (satisfying signal at index 1 of 3).WaitAnyCompactsConditionsAndValues— regresses theconds/valuescompaction fix (NULL signal interleaved before valid ones with distinct condition/value pairs).WaitAllReportsSatisfyingValues— non-regression check forhsa_amd_signal_wait_all.WaitAnyReportsNonzeroSatisfyingIndexreturnssatisfying_value=0instead of the expected41) and pass 3/3 against a freshly rebuilt library containing this PR's fix.hsa_amd_signal_wait_anywith multi-signal batches where the satisfying index is >0 in the common case; before this fix, that prototype produced incorrect/garbage wake-signal values, consistent with this bug. After the fix: correct behavior confirmed acrossctestruns and repeated stress runs.Related
Related to ROCM-27290 (rocprofiler-sdk Queue Interposition performance investigation) but is a standalone, independently-valid bug fix.