Record NUMA nodes >= 32 in the partition node bitmap - #542
Draft
rootkiller6788 wants to merge 1 commit into
Draft
Conversation
InitNumaTopology builds partition_to_nodes by OR-ing in 1 << node, where node is a size_t. The literal 1 is a 32-bit signed int, so for node >= 31 the shift is undefined behavior and, on x86, corrupts the uint64_t bitmap: node 31 sign-extends to set bits 31..63, and node 32+ has its shift count masked modulo 32, setting a bit belonging to a lower node instead. The resulting nodemask is passed to mbind() for memory binding, so on machines with 32 or more NUMA nodes memory could be bound to the wrong nodes or mbind could fail (fatal under strict binding). Shift in a 64-bit type instead and add a regression test covering a node index of 32.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
InitNumaTopology(tcmalloc/internal/numa.cc) records each detected NUMA node in the per-partitionpartition_to_nodesbitmap with:partition_to_nodes[partition] |= 1 << node;nodeis asize_t, but the literal1is a 32-bit signedint, so fornode >= 31the shift is undefined behavior. On x86 (and every supported LP64 Linux ABI, whereintis 32-bit) the practical result is a corrupted bitmap:node == 31:1 << 31produces a negativeintthat sign-extends to0xffffffff80000000, spuriously setting bits 31..63 of the node bitmap.node >= 32: the shift count is masked modulo 32 (e.g.1 << 32==1 << 0), setting a lower node's bit instead of the intended one.This bitmap is consumed by
SystemAllocator::BindMemory(tcmalloc/internal/system_allocator.h) and passed tombind(2)as the nodemask for NUMA memory binding. On machines with 32 or more NUMA nodes (8-socket EPYC, IBM POWER, etc.) memory can therefore be bound to the wrong nodes — ormbindfails withEINVAL, which is fatal underTCMALLOC_NUMA_AWARE=strict-binding.Fix
Shift in a 64-bit type so the intended bit is set:
Behavior is unchanged for
node < 31.Test
Added
NumaTopologyTest.HighNodeIndex, which initializes a topology with 33 synthetic NUMA nodes (node indices 0..32) and asserts that node 32 is recorded in the partition 0 bitmap. Before this change the test fails:1 << 32evaluates to1on x86, so bit 32 is never set; after the change it passes.Verification
nodein {31, 32, 33, 63}, the old expression set0xffffffff80000000,0x1,0x2, and0xffffffff80000000respectively, while the fixed expression sets exactly the intended single bit.partition_to_nodesis declared asuint64_t partition_to_nodes_[kNumInternalPartitions]in numa.h;uint64_t{1}matches the shift style already used elsewhere in the tree (e.g. tcmalloc/internal/pageflags.cc).