Skip to content

[MIOpen] Fix use-after-free of kernel_id in InitInvokerFactoryNCHW - #7922

Merged
jdcampbe merged 6 commits into
developfrom
users/jascampb/fix-kernel-name-reference-NCHW
Jun 8, 2026
Merged

jdcampbe merged 6 commits into
developfrom
users/jascampb/fix-kernel-name-reference-NCHW

Conversation

@jdcampbe

@jdcampbe jdcampbe commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Running any NCHW grouped CK xdlops convolution with kernel-name logging enabled (e.g. MIOPEN_PERFORMANCE_LOGS=4) deterministically SIGSEGVs inside
MIOpen's invoker. This was originally surfaced as a CK / large-tensor regression in a perf-eval run, but root-causing showed the fault is host-side in
MIOpen and reproduces independent of data type or tensor size — any NCHW grouped CK conv that runs AddKernelToJsonAccumulator will crash.

This PR fixes the use-after-free so the NCHW invoker matches the by-value capture used by the three sibling invoker factories in the same file.

Technical Details

In projects/miopen/src/ck_impl/implicitgemm_ck_util.hpp, InitInvokerFactoryNCHW captured its const std::string& kernel_id parameter by
pointer
:

result.invoker_factory = [kernel_id_ = &kernel_id, ...]  // dangling once function returns
    return [kernel_id2 = kernel_id_, ...]                // kernel_id2 is const std::string*
        ...
        MIOPEN_LOG_I2("kernel_name = " << kernel_id2);            // prints stale stack address
        AddKernelToJsonAccumulator(*kernel_id2, elapsed, false);  // dereferences dangling ptr -> SIGSEGV

The invoker_factory (and its nested invoker lambda) are stored in the returned ConvSolution and run long after InitInvokerFactoryNCHW has
returned, by which time the referent of the kernel_id parameter has been destroyed. The deref is gated on IsProfilingEnabled() && IsLoggingKernel(), so it fires whenever kernel-name JSON logging is on during execution.

The other three invoker factories in this file (NHWC fwd, NHWC bwd/wrw) already capture kernel_id by value (lines 690, 1138, 1210). An earlier
dereference-error fix corrected the *kernel_id2 deref in some sibling paths but missed this NCHW one.

This 3-line change:

  • Capture kernel_id by value: kernel_id_ = kernel_id (line 981).
  • Move the owned string into the inner lambda: kernel_id2 = std::move(kernel_id_) (line 991).
  • Drop the dereference: AddKernelToJsonAccumulator(kernel_id2, elapsed, false) (line 1079).

As a beneficial side-effect, MIOPEN_LOG_I2("kernel_name = " << kernel_id2) now prints the actual kernel name instead of the stale pointer address.

The remaining &kernel_id capture in FindConvPtrByID (line 165) is a synchronous, non-escaping std::find_if lambda and is safe.

Test Plan

  • Re-run the original reproducer (e.g. MIOpenDriver conv -n 1 -c 512 -H 912 -W 1144 -k 512 -y 3 -x 3 -p 1 -q 1 -u 1 -v 1 -l 1 -j 1 -m conv -g 1 -F 1 -t 1 -iter 10) with MIOPEN_FIND_MODE=1 MIOPEN_FIND_ENFORCE=4 MIOPEN_LOG_LEVEL=7 MIOPEN_PERFORMANCE_LOGS=4 on gfx950 and confirm it no longer
    SIGSEGVs.
  • Re-run the broader perf-eval shape-coverage shard that produced ~69 segfaults on this same code path and confirm those crashes are gone.
  • Spot-check the MIOPEN_LOG_I2("kernel_name = …") log line now prints the kernel name string (e.g.
    DeviceGroupedConvFwdMultipleABD_Xdl_CShuffle<...>) rather than a 0x7fff… stack address.
  • Existing MIOpen gtests covering NCHW grouped CK conv paths should continue to pass.

Test Result

  • Static analysis: confirmed no remaining *kernel_id2 dereferences and no escaping &kernel_id captures in implicitgemm_ck_util.hpp; all four
    invoker factories now capture kernel_id by value.
  • Validated at runtime through a battery of tests.

Risk Assessment

Risk level: Low

  • Impacted components: projects/miopen/src/ck_impl/implicitgemm_ck_util.hpp, specifically the NCHW invoker-factory path used by the
    ConvHipImplicitGemmGroup{Fwd,Bwd,Wrw}Xdlops and ConvHipImplicitGemm3DGroup{Fwd,Bwd,Wrw}Xdlops solvers when the problem is in default (NCHW/NCDHW)
    layout. No CK code, no kernel code, no public API changes. Other MIOpen layouts and solvers are unaffected.
  • Potential side effects:
    • Functional: removes a guaranteed use-after-free; no behavior change on the happy path besides the log line now printing the kernel name instead of
      a stale address.
    • Performance: one extra std::string copy per InitInvokerFactoryNCHW call (a one-time setup cost during solution build), then a std::move into
      the inner lambda — negligible vs the kernel launch and CK arg setup cost. Per-invocation cost on the hot path is unchanged.
    • ABI/API: none.
  • Mitigation steps:
    • Change is minimal (3 lines) and matches the established by-value pattern already in use by the three sibling invoker factories in the same file, so
      review surface is small.
    • The defect is straightforwardly reproducible with MIOPEN_PERFORMANCE_LOGS on any NCHW grouped CK conv, making the fix easy to verify pre-merge.
    • If a regression surfaces, the change is trivially revertible.

Submission Checklist

InitInvokerFactoryNCHW captured the kernel_id string parameter by
pointer (kernel_id_ = &kernel_id) instead of by value. The
invoker_factory and its nested invoker lambda are stored in the
returned ConvSolution and executed long after InitInvokerFactoryNCHW
returns, by which point the referent of the `const std::string&
kernel_id` parameter has been destroyed. At invoke time the dangling
pointer was dereferenced via AddKernelToJsonAccumulator(*kernel_id2,
...) (gated on profiling + IsLoggingKernel), reading a freed
std::string and crashing with SIGSEGV. The accompanying
MIOPEN_LOG_I2("kernel_name = " << kernel_id2) also printed the stale
pointer address instead of the kernel name.

All three sibling invoker factories in this file already capture
kernel_id by value; only the NCHW forward path used a pointer (and the
earlier dereference-error fix corrected the other paths but missed this
one). Capture kernel_id by value here too so the lambda owns the string
for its full lifetime, drop the now-unnecessary dereference, and let the
log print the actual kernel name.

This is reached whenever an NCHW grouped CK xdlops convolution is
executed with kernel logging enabled (e.g. MIOPEN_PERFORMANCE_LOGS),
independent of data type or tensor size.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@BrianHarrisonAMD BrianHarrisonAMD left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Minor comments, but I don't think it's blocking.

@JonathanLichtnerAMD JonathanLichtnerAMD left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Would it be worth adding a test for this, i.e., it could either reproduce the crash or ASAN could detect the issue? I'll leave that as optional but having a test means that ASAN and other sanitizers will validate the code path against future changes.

@yassinsolim yassinsolim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The lifetime fix is correct: kernel_id is now owned by the factory/invoker instead of captured by pointer to a local. Brian’s move-consistency comments seem reasonable but non-blocking to me.

@jdcampbe
jdcampbe merged commit 394ebff into develop Jun 8, 2026
45 checks passed
@jdcampbe
jdcampbe deleted the users/jascampb/fix-kernel-name-reference-NCHW branch June 8, 2026 16:14
JonathanLichtnerAMD added a commit that referenced this pull request Jul 15, 2026
JIRA ID: ALMIOPEN-2312

## Motivation

`projects/miopen/CHANGELOG.md`'s "MIOpen 3.5.2 for ROCm 7.14.0" section
only had 4 entries, but 68 commits touched `projects/miopen` between the
`therock-7.13` tag and `release/therock-7.14`. Auditing that commit
range (via `git log --oneline
therock-7.13..remotes/origin/release/therock-7.14 -- projects/miopen`)
found 18 additional user-facing changes — new gfx950/gfx1250
architecture support, SystemDB refreshes, AI-heuristic improvements,
several correctness/crash bug fixes, and OpenCL solver removals — that
were never documented. This PR backfills those entries so the 7.14.0
changelog accurately reflects what shipped.

## Technical Details

Added 24 lines to the existing "MIOpen 3.5.2 for ROCm 7.14.0" section of
`projects/miopen/CHANGELOG.md`, merging new bullets into the existing
`Added`/`Changed`/`Resolved Issues` sub-sections (no new entries were
duplicated or reworded) and adding a new `Removed` sub-section for
solver removals. No existing content was altered.

Each new bullet was traced to a specific commit + PR:

| Entry | Commit | PR |
|---|---|---|
| Added gfx950 (MI350X/MI355X) SystemDB | `60d8e1e5175` | #8081 |
| Added initial gfx1250 architecture support | `d3f057b2f22` | #7587 |
| Added JSON-formatted performance logs (`MIOPEN_PERFORMANCE_LOGS`) |
`ffbde871260` | #5736 |
| Refreshed gfx942 SystemDB | `faf754a9ef6` | #8174 |
| Refined 2D AI heuristics (Tunanet/KTN) for gfx942 & gfx950 |
`c24ad415884` | #7109 |
| Fixed thread-safety issue in AI-heuristic model caches | `cd957402309`
| #9149 (cherry-pick of #9131) |
| Fixed CK grouped-conv solvers not always registered in host |
`c971d54d7ae` | #9105 |
| Fixed legacy CK buffer resource descriptor (gfx1250) | `1dee011b8b0` |
#8758 |
| Removed stale gfx942 SystemDB naive-solver entries | `f42668f6316` |
#7028 |
| Fixed softmax NaN when `beta=0` | `fb8157c8512` | #8426 |
| Fixed BN backward spatial variant 1 vectorized memory access bug |
`1351a7e8b10` | #6765 |
| Fixed use-after-free of `kernel_id` in `InitInvokerFactoryNCHW` |
`394ebffb50d` | #7922 |
| Fixed SoftMarginLoss forward reduce-kernel kernarg corruption |
`1e65a31570f` | #8065, #8135 |
| Fixed build with `MIOPEN_ENABLE_AI_KERNEL_TUNING=OFF` | `e6288c3ca61`
| #6800 |
| Fixed install RPATH for MIOpenDriver / CK backend libs | `ef8a4cf2f93`
| #7485 |
| Fixed dangling lambda causing log-level 6 segfault | `190f9791737` |
#6953 |
| Enabled Windows long-path support for `addkernels` | `1f9d69724cf` |
#7387 |
| Removed OCL solver `ConvOclDirectFwd1x1` | `126c720cadd` | #6491 |
| Removed OCL solver `conv_ocl_dir2Dfwdgen` | `2d499ddf709` | #6633 |
| Removed OCL solvers `ConvOclBwdWrW1x1`, `ConvOclBwdWrW2` |
`b5d43a33dc6` | #6859 |
| Removed OCL solvers `OpTensorLeadingOnes`, `Op5dTensorGeneric` |
`8642c83b7de` | #7347 |

The remaining ~40 commits in the 68-commit range were deliberately
excluded (tests/CI/build/docs/internal refactors/1:1 OCL→HIP swaps with
no behavior change, per each commit's own stated scope) and are not
represented here.

## Risk Assessment

**Risk Level: 🟢 Low**

This is a documentation-only change to a single Markdown file. No code,
build configuration, tests, or public API surface is touched.

### Impacted Components

- `projects/miopen/CHANGELOG.md` only.

### Potential Side Effects

None expected. This does not change any runtime behavior; it only
documents changes that already shipped on `release/therock-7.14`.

### Mitigation Steps

None required beyond normal review — a reviewer familiar with the 7.14
release can spot-check a few entries against the linked commit hashes/PR
numbers for accuracy.

## Test Plan

Not applicable (no code changes). Verification was done by
cross-referencing every added bullet against a specific commit hash and
PR number pulled from `git log`, and by diffing the edited section
against `git show
remotes/origin/release/therock-7.14:projects/miopen/CHANGELOG.md` to
confirm no existing entry was duplicated or altered.

## Test Result

All 21 new bullets map 1:1 to a real commit/PR in the
`therock-7.13..release/therock-7.14` range for `projects/miopen`. No
pre-existing changelog text was modified — `git diff` shows a pure
addition (24 insertions, 0 deletions). Two borderline commits
(`a67834413e8` "Standalone build", `7c7cd5774a9` "enable warnings about
deprecated API/features") were deliberately left out pending author
judgment on whether they warrant an entry.
pschang-phy pushed a commit that referenced this pull request Jul 17, 2026
JIRA ID: ALMIOPEN-2312

## Motivation

`projects/miopen/CHANGELOG.md`'s "MIOpen 3.5.2 for ROCm 7.14.0" section
only had 4 entries, but 68 commits touched `projects/miopen` between the
`therock-7.13` tag and `release/therock-7.14`. Auditing that commit
range (via `git log --oneline
therock-7.13..remotes/origin/release/therock-7.14 -- projects/miopen`)
found 18 additional user-facing changes — new gfx950/gfx1250
architecture support, SystemDB refreshes, AI-heuristic improvements,
several correctness/crash bug fixes, and OpenCL solver removals — that
were never documented. This PR backfills those entries so the 7.14.0
changelog accurately reflects what shipped.

## Technical Details

Added 24 lines to the existing "MIOpen 3.5.2 for ROCm 7.14.0" section of
`projects/miopen/CHANGELOG.md`, merging new bullets into the existing
`Added`/`Changed`/`Resolved Issues` sub-sections (no new entries were
duplicated or reworded) and adding a new `Removed` sub-section for
solver removals. No existing content was altered.

Each new bullet was traced to a specific commit + PR:

| Entry | Commit | PR |
|---|---|---|
| Added gfx950 (MI350X/MI355X) SystemDB | `60d8e1e5175` | #8081 |
| Added initial gfx1250 architecture support | `d3f057b2f22` | #7587 |
| Added JSON-formatted performance logs (`MIOPEN_PERFORMANCE_LOGS`) |
`ffbde871260` | #5736 |
| Refreshed gfx942 SystemDB | `faf754a9ef6` | #8174 |
| Refined 2D AI heuristics (Tunanet/KTN) for gfx942 & gfx950 |
`c24ad415884` | #7109 |
| Fixed thread-safety issue in AI-heuristic model caches | `cd957402309`
| #9149 (cherry-pick of #9131) |
| Fixed CK grouped-conv solvers not always registered in host |
`c971d54d7ae` | #9105 |
| Fixed legacy CK buffer resource descriptor (gfx1250) | `1dee011b8b0` |
#8758 |
| Removed stale gfx942 SystemDB naive-solver entries | `f42668f6316` |
#7028 |
| Fixed softmax NaN when `beta=0` | `fb8157c8512` | #8426 |
| Fixed BN backward spatial variant 1 vectorized memory access bug |
`1351a7e8b10` | #6765 |
| Fixed use-after-free of `kernel_id` in `InitInvokerFactoryNCHW` |
`394ebffb50d` | #7922 |
| Fixed SoftMarginLoss forward reduce-kernel kernarg corruption |
`1e65a31570f` | #8065, #8135 |
| Fixed build with `MIOPEN_ENABLE_AI_KERNEL_TUNING=OFF` | `e6288c3ca61`
| #6800 |
| Fixed install RPATH for MIOpenDriver / CK backend libs | `ef8a4cf2f93`
| #7485 |
| Fixed dangling lambda causing log-level 6 segfault | `190f9791737` |
#6953 |
| Enabled Windows long-path support for `addkernels` | `1f9d69724cf` |
#7387 |
| Removed OCL solver `ConvOclDirectFwd1x1` | `126c720cadd` | #6491 |
| Removed OCL solver `conv_ocl_dir2Dfwdgen` | `2d499ddf709` | #6633 |
| Removed OCL solvers `ConvOclBwdWrW1x1`, `ConvOclBwdWrW2` |
`b5d43a33dc6` | #6859 |
| Removed OCL solvers `OpTensorLeadingOnes`, `Op5dTensorGeneric` |
`8642c83b7de` | #7347 |

The remaining ~40 commits in the 68-commit range were deliberately
excluded (tests/CI/build/docs/internal refactors/1:1 OCL→HIP swaps with
no behavior change, per each commit's own stated scope) and are not
represented here.

## Risk Assessment

**Risk Level: 🟢 Low**

This is a documentation-only change to a single Markdown file. No code,
build configuration, tests, or public API surface is touched.

### Impacted Components

- `projects/miopen/CHANGELOG.md` only.

### Potential Side Effects

None expected. This does not change any runtime behavior; it only
documents changes that already shipped on `release/therock-7.14`.

### Mitigation Steps

None required beyond normal review — a reviewer familiar with the 7.14
release can spot-check a few entries against the linked commit hashes/PR
numbers for accuracy.

## Test Plan

Not applicable (no code changes). Verification was done by
cross-referencing every added bullet against a specific commit hash and
PR number pulled from `git log`, and by diffing the edited section
against `git show
remotes/origin/release/therock-7.14:projects/miopen/CHANGELOG.md` to
confirm no existing entry was duplicated or altered.

## Test Result

All 21 new bullets map 1:1 to a real commit/PR in the
`therock-7.13..release/therock-7.14` range for `projects/miopen`. No
pre-existing changelog text was modified — `git diff` shows a pure
addition (24 insertions, 0 deletions). Two borderline commits
(`a67834413e8` "Standalone build", `7c7cd5774a9` "enable warnings about
deprecated API/features") were deliberately left out pending author
judgment on whether they warrant an entry.
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.

6 participants