fix(moe): pad trtllm-gen route map by one element to avoid OOB read - #4237
Conversation
Routed batched-GEMM kernels read one int32 past the end of ptrRouteMap from the last batch-dim CTA on every launch. The value is never consumed, so results stay correct, but the read faults with an illegal address whenever the allocation happens to end at a mapped-region boundary -- which is why it surfaces as flaky MoE autotune and inference crashes. The real fix is kernel-side and is being handled separately. Pad the allocation by one element so the already-shipped cubins stay in bounds; the +1 can be dropped once regenerated cubins land. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Enwei Zhu <21126786+syuoni@users.noreply.github.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe MoE routing workspace allocation for ChangesRouting workspace padding
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/bot run tests/moe |
|
Per our discussion, let's take this hot fix for now. The proper fix is expected to take longer to land, and we can replace this workaround once it's ready. |
|
cc @rosenrodt for viz |
|
Thanks for the fix. Will notify here when cubin fix lands |
samuellees
left a comment
There was a problem hiding this comment.
LGTM. For CI fails on GB300, it's about AllReduce and not relative with this PR. And this is not relative with 5090.
… to avoid OOB read) (#4288) …#4237) ## 📌 Description The routed dynamic-batch batched-GEMM kernels (`Bmm_*_dynB` with `-routeAct`) read one `int32` past the end of `ptrRouteMap`, from the last batch-dim CTA, on every launch. The read is speculative — its value is never consumed, so outputs are always bitwise correct — but it faults with `CUDA_ERROR_ILLEGAL_ADDRESS` whenever the allocation happens to end at a mapped-region boundary. That allocator-placement dependence is what makes it surface as *flaky* MoE autotune and inference crashes. Root cause is kernel-side: an off-by-one clamp in the hoisted load-task initializer (`WarpGrpThreadIdx` is clamped to the load-group size instead of size − 1), so off-group threads compute row `tileN` of a `tileN`-row tile, i.e. index `(ctaIdxY + 1) * tileN`. For the last CTA that is exactly one element past the shape documented in `KernelParamsDecl.h` (`[sum(divUpMul(N[bi], tileN) for bi in B)]`) — which is what we allocate. It has been reported to the kernel owners and is being fixed there. This PR is the integration-side workaround: allocate the route map with one extra element, so the already-shipped prebuilt cubins stay in bounds. The overrun is provably always exactly one `int32` from one CTA, so `+1` is sufficient by construction, not a heuristic. The pad slot's contents are irrelevant since the value is never used. Two allocation sites, both `permuted_idx_to_token_idx` (→ `routeMap`). Marked `WAR` + `TODO` so the `+1` can be dropped once regenerated cubins land. ## 🔍 Related Issues Likely explains #3530 and #3168 (and possibly #4012, #2776) — all report intermittent NVFP4 MoE autotune crashes, and #3168 additionally reports silent garbage output, which is the expected signature of an OOB read that usually lands on mapped memory. ## 🚀 Pull Request Checklist ### ✅ Pre-commit Checks - [x] I have installed `pre-commit` by running `pip install pre-commit` (or used your preferred method). - [x] I have installed the hooks with `pre-commit install`. - [x] I have run the hooks manually with `pre-commit run --all-files` and fixed any reported issues. ## 🧪 Tests - [ ] Tests have been added or updated as needed. - [x] All tests are passing (`unittest`, etc.). No test added — the failure is allocator-placement dependent and does not reproduce deterministically through the public API. Validation was done on `v0.6.15.post1` (B200, SM100, Kimi-K2.5 NVFP4 TP4): | Check | stock | route map +1 | |---|---|---| | `compute-sanitizer` memcheck, M=2 bucket × 4 ranks | 40 × `Invalid __global__ read of size 4` | **0 violations** (816 profiles/rank) | | Guard-page probe (route map placed at the tail of a VMM mapping) | faults on every launch | **clean** | | Full-model TP4 autotune crash loop | crashed by attempt 2 in 3/3 loops | **6/6 clean** | The guard-page probe is the load-bearing one: it removes all dependence on allocator placement, so it is deterministic in both directions. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved stability for fused mixture-of-experts routing operations by preventing out-of-bounds memory access in supported execution paths. * Applied the safeguard to both standard and FP4 block-scale routing workflows. <!-- end of auto-generated comment: release notes by coderabbit.ai --> (cherry picked from commit 8b6af1a) <!-- .github/pull_request_template.md --> ## 📌 Description <!-- What does this PR do? Briefly describe the changes and why they’re needed. --> ## 🔍 Related Issues <!-- Link any related issues here --> ## 🚀 Pull Request Checklist Thank you for contributing to FlashInfer! Before we review your pull request, please make sure the following items are complete. ### ✅ Pre-commit Checks - [ ] I have installed `pre-commit` by running `pip install pre-commit` (or used your preferred method). - [ ] I have installed the hooks with `pre-commit install`. - [ ] I have run the hooks manually with `pre-commit run --all-files` and fixed any reported issues. > If you are unsure about how to set up `pre-commit`, see [the pre-commit documentation](https://pre-commit.com/). ## 🧪 Tests - [ ] Tests have been added or updated as needed. - [ ] All tests are passing (`unittest`, etc.). ## Reviewer Notes <!-- Optional: anything you'd like reviewers to focus on, concerns, etc. --> Signed-off-by: Enwei Zhu <21126786+syuoni@users.noreply.github.com> Co-authored-by: Enwei Zhu <21126786+syuoni@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
📌 Description
The routed dynamic-batch batched-GEMM kernels (
Bmm_*_dynBwith-routeAct)read one
int32past the end ofptrRouteMap, from the last batch-dim CTA, onevery launch. The read is speculative — its value is never consumed, so outputs
are always bitwise correct — but it faults with
CUDA_ERROR_ILLEGAL_ADDRESSwhenever the allocation happens to end at a mapped-region boundary. That
allocator-placement dependence is what makes it surface as flaky MoE autotune
and inference crashes.
Root cause is kernel-side: an off-by-one clamp in the hoisted load-task
initializer (
WarpGrpThreadIdxis clamped to the load-group size instead ofsize − 1), so off-group threads compute row
tileNof atileN-row tile, i.e.index
(ctaIdxY + 1) * tileN. For the last CTA that is exactly one element pastthe shape documented in
KernelParamsDecl.h(
[sum(divUpMul(N[bi], tileN) for bi in B)]) — which is what we allocate. It hasbeen reported to the kernel owners and is being fixed there.
This PR is the integration-side workaround: allocate the route map with one
extra element, so the already-shipped prebuilt cubins stay in bounds. The
overrun is provably always exactly one
int32from one CTA, so+1issufficient by construction, not a heuristic. The pad slot's contents are
irrelevant since the value is never used.
Two allocation sites, both
permuted_idx_to_token_idx(→routeMap). MarkedWAR+TODOso the+1can be dropped once regenerated cubins land.🔍 Related Issues
Likely explains #3530 and #3168 (and possibly #4012, #2776) — all report
intermittent NVFP4 MoE autotune crashes, and #3168 additionally reports silent
garbage output, which is the expected signature of an OOB read that usually
lands on mapped memory.
🚀 Pull Request Checklist
✅ Pre-commit Checks
pre-commitby runningpip install pre-commit(or used your preferred method).pre-commit install.pre-commit run --all-filesand fixed any reported issues.🧪 Tests
unittest, etc.).No test added — the failure is allocator-placement dependent and does not
reproduce deterministically through the public API. Validation was done on
v0.6.15.post1(B200, SM100, Kimi-K2.5 NVFP4 TP4):compute-sanitizermemcheck, M=2 bucket × 4 ranksInvalid __global__ read of size 4The guard-page probe is the load-bearing one: it removes all dependence on
allocator placement, so it is deterministic in both directions.
Summary by CodeRabbit