Fix int32 overflow in softmax_warp_forward offset arithmetic - #32330
Fix int32 overflow in softmax_warp_forward offset arithmetic#32330DKAIN-py wants to merge 1 commit into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Widens CUDA Softmax kernel offset arithmetic to prevent large-tensor out-of-bounds memory access.
Changes:
- Uses 64-bit offsets in both warpwise Softmax implementations.
- Documents the overflow rationale.
Suppressed comments (1)
onnxruntime/core/providers/cuda/math/softmax_warpwise_impl.cuh:185
blockIdx.xis unsigned, so the old multiplication was evaluated in 32-bit unsigned arithmetic. It remains correct pastINT32_MAXand wraps only pastUINT32_MAX; the current explanation gives the wrong failure boundary and incorrectly describes this as identical to the signedfirst_batchcase. Please document the unsigned threshold while retaining the cast.
// blockIdx.x and stride are both int32; on large inputs their product can
// exceed INT32_MAX and silently wrap before it's added to src/dst, causing
// out-of-bounds reads/writes. Cast to int64_t before multiplying so the
// arithmetic happens in 64-bit.
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // exceed INT32_MAX and silently wrap before it's added to src/dst, causing | ||
| // out-of-bounds reads/writes. Cast to int64_t before multiplying so the | ||
| // arithmetic happens in 64-bit. | ||
| const int64_t thread_offset = static_cast<int64_t>(first_batch) * stride + local_idx; |
|
|
@microsoft-github-policy-service agree |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
@microsoft-github-policy-service agree |
first_batch and stride/blockIdx.x are int32; their product can exceed INT32_MAX for large batch counts, silently wrapping before being added to src/dst. This causes out-of-bounds reads/writes, observed as an illegal memory access, silently unwritten output, or occasionally a hang, depending on allocator layout. Fixes both softmax_warp_forward and softmax_warp_forward_resource_efficient, which have the identical pattern. Widened the offset computation to int64_t at the multiply site rather than changing first_batch/stride's types, since those are used elsewhere in signed-subtraction bounds checks that assume int. Verified with a standalone repro: an isolated arithmetic test showing the exact expression wraps to a negative offset, and a full GPU kernel test (~4GB fp16 tensor, ~2.1M rows) showing the unfixed kernel faults with an illegal memory access at the exact predicted boundary row, and the fixed kernel produces correct output across the boundary. Fixes microsoft#32299
first_batch and stride/blockIdx.x are int32; their product can exceed INT32_MAX for large batch counts, silently wrapping before being added to src/dst. This causes out-of-bounds reads/writes, observed as an illegal memory access, silently unwritten output, or occasionally a hang, depending on allocator layout.
Fixes both softmax_warp_forward and softmax_warp_forward_resource_efficient, which have the identical pattern. Widened the offset computation to int64_t at the multiply site rather than changing first_batch/stride's types, since those are used elsewhere in signed-subtraction bounds checks that assume int.
Verified with a standalone repro: an isolated arithmetic test showing the exact expression wraps to a negative offset, and a full GPU kernel test (~4GB fp16 tensor, ~2.1M rows) showing the unfixed kernel faults with an illegal memory access at the exact predicted boundary row, and the fixed kernel produces correct output across the boundary.
Addresses #32299
Good — this is close, but it looks like the template's section headers (
### Description,### Motivation and Context) got left as empty placeholders below your actual content instead of your content going into them. Let's restructure it properly and fold in the test output as evidence. Here's the full corrected PR body:Description
Widens the offset computation in
softmax_warp_forwardandsoftmax_warp_forward_resource_efficient(onnxruntime/core/providers/cuda/math/softmax_warpwise_impl.cuh) fromint32toint64_tat the multiply site.first_batchandstride(andblockIdx.x/stridein the resource-efficient variant) are bothint32. Their product can exceedINT32_MAXfor large batch counts, silently wrapping before being added tosrc/dst. This causes out-of-bounds reads/writes — observed as an illegal memory access, silently unwritten output, or occasionally a hang, depending on allocator layout.Kept
first_batch/stride/batch_sizeasintrather than widening their declared types, sincelocal_batches = batch_size - first_batchrelies on signed subtraction elsewhere in the function and widening those types risked an unrelated regression for no benefit — only the multiplication result needed widening.Motivation and Context
Addresses #32299.
A tensor large enough to trigger this (
batch_size × stride > 2^31) requires several GB of host memory to construct through the standardOpTesterfloat-vector interface, which isn't practical to add as a CI unit test. Instead, I verified the fix two ways:1. Isolated arithmetic test (no GPU): confirms
first_batch * stridecomputed inint32produces a wrapped/negative result for representative large inputs, and that theint64_t-cast version produces the correct value.2. Standalone GPU repro: a minimal extraction of the kernel's offset logic and warp-reduction structure, run against a ~4GB fp16 tensor (~2.1M rows × 1024 elements) straddling the exact overflow boundary (predicted boundary row: 2,097,152, where
first_batch × stridefirst exceedsINT32_MAX).Before the fix, the kernel faults deterministically at the predicted boundary:
After the fix, the same tensor — including the rows immediately straddling the boundary — produces correct output:
Existing
Softmaxop tests (softmax_test.cc) are unaffected by this change — it only touches the offset computation, not the reduction/arithmetic logic.Note on Erf
The linked issue also reports Erf as silently producing wrong output above the same 2³¹-element boundary. I traced Erf's standalone CUDA path end-to-end, unary_elementwise_ops.cc::ComputeInternal (passes Tensor::Shape().Size(), which is int64_t) → Impl_Erf → UnaryElementWiseImpl → the templated _UnaryElementWise kernel in unary_elementwise_impl.cuh — and didn't find the same bug. The kernel's per-thread index is computed as static_cast<int64_t>(NumElementsPerThread) * NumThreadsPerBlock * blockIdx.x + threadIdx.x, which widens to int64_t before any multiplication happens, and the host-side grid-size calculation is already guarded with ORT_ENFORCE(blocksPerGridSize <= INT32_MAX, ...). This path appears correct as-is on main.
This PR only fixes the Softmax case (softmax_warp_forward / softmax_warp_forward_resource_efficient). I don't have a repro for the Erf case described in the issue, it's possible it goes through a fused path (e.g. GELU) or is specific to the ROCm backend (the issue's hardware trace is from ROCm/MI300A) rather than the standalone CUDA unary-elementwise dispatch I checked. Flagging this rather than guessing at a fix for code I couldn't confirm is broken, happy to dig further if a maintainer can point me at the right path, or if the issue author can confirm which path they hit.