Skip to content

Commit 6727436

Browse files
Validate and lock SharedBuffer allocations (#24463)
Summary: ## TL;DR Serialize the QNN shared-buffer manager's bookkeeping, preserve zero-byte allocations, and return `nullptr` for invalid requests or recoverable bookkeeping failures. A duplicate pointer returned by the RPC allocator must leave the existing live allocation intact. Host regressions cover these contracts, and the existing OSS QNN Buck job now executes the suite instead of only building it. ## Context `QnnExecuTorchAllocCustomMem` allocates RPC-backed memory through the process-wide `SharedBuffer` singleton. Clients can associate tensor addresses with an allocation using `QnnExecuTorchAddCustomMemTensorAddr`; `QnnManager::RegisterMem` then selects direct ION registration or custom-memory registration during execution. `QnnExecuTorchFreeCustomMem` frees the original RPC pointer and removes the allocation and its tensor aliases. The manager maintains four maps: aligned pointer to original RPC pointer, aligned pointer to allocated size, tensor address to custom-memory base, and custom-memory base to tensor addresses. These maps describe ownership and registration metadata; they are not owners of the client's execution lifetime. The client must retain an allocation and its aliases until every registration and execution using them has finished. ```text client allocation client execution QnnExecuTorchAllocCustomMem QnnManager::RegisterMem -> SharedBuffer::AllocMem -> alias lookup -> RPC allocation [bytes + alignment] -> RegisterCustomMem / RegisterIonMem -> aligned address + bookkeeping -> QNN execution client teardown, after registration and execution have finished QnnExecuTorchFreeCustomMem -> FreeMem -> RPC free + removal of all aliases ``` This is allocator hardening for every QNN custom-memory client. It does not enable shared I/O, change exported model contents, or establish safe concurrent execution and freeing of the same allocation. ## Problem Map reads and writes previously had no common mutex. Calls on independent threads could mutate the same containers while another thread looked up registration metadata. Even locking each lookup separately does not make a series of size and file-descriptor reads one consistent observation. Allocation bookkeeping also had failure cases. The RPC allocator takes a signed 32-bit byte count, but the requested size and alignment are `size_t`. For example, `bytes = INT32_MAX - 63` and `alignment = 64` require `2,147,483,648` bytes, which exceeds its limit by one byte. Alignment zero and non-power-of-two alignment are invalid. Zero **bytes**, however, were historically valid: the alignment padding still makes the RPC request positive, and rejecting that request breaks callers with empty I/O tensors. The duplicate-address failure is distinct from allocation failure. If an allocator returns the same still-live pointer twice, failure to insert the second mapping must not free that pointer or overwrite the first allocation's size. A successful RPC allocation can also be followed by `std::bad_alloc` from either map insertion; that must release the new allocation and return `nullptr` when the standard library supports throwing allocation failures. ```text duplicate RPC address P before: existing P -> first size second allocation returns P insertion fails -> free(P) -> first allocation now refers to freed memory after: existing P -> first size second allocation returns P reject with nullptr -> no free, no map changes first allocation's eventual FreeMem(P) frees it once ``` ## Solution One `memory_mutex_` protects allocation, freeing, alias bookkeeping and lookup operations. `GetAllocationInfo` obtains the allocated size and RPC file descriptor under one lock and returns `{0, -1}` for an allocation that is no longer recorded. `RegisterCustomMem` consumes that snapshot instead of taking independent size and descriptor snapshots. This makes the snapshot consistent; it does not retain memory after the lock is released. The public free API documents the client's lifetime obligation explicitly. Validate alignment and require `bytes <= INT32_MAX - alignment` before calling RPC. This avoids overflowing the sum and accepts the exact limit `bytes + alignment == INT32_MAX`. `AllocMem(0, alignment)` allocates only the padding. Reject a duplicate aligned address before taking ownership of it, because freeing it would destroy the existing live allocation. For a new address, insert the original-pointer and size mappings while holding the mutex. If an insertion throws `std::bad_alloc`, erase the partial pointer mapping, free the new RPC allocation, log the failure and return `nullptr`. There is no redundant assignment after insertion. Toolchains without throwing allocation support retain their allocation-failure behavior; the regression that injects throwing `operator new` is enabled only where those exceptions are supported. ```text AllocMem(bytes, alignment), size units: bytes validate -> lock -> RPC allocation -> align address | address already live? -> nullptr; preserve live owner | insert original-pointer mapping | insert allocated-size mapping | return aligned pointer either insertion throws bad_alloc -> erase partial bookkeeping -> RPC free -> nullptr ``` The host tests exercise offset alignment and freeing the original pointer, zero-byte compatibility, invalid requests without an RPC call, the exact signed-32-bit limit, duplicate live addresses, allocation snapshots before and after free, and concurrent bookkeeping on distinct allocations. Where supported, an injected allocation failure exercises each insertion separately and checks cleanup and successful retry. Test teardown uses the configured RPC free callback, so an offset allocator is cleaned up correctly even after a failed assertion. The OSS `test-qnn-buck-build-linux` job already installs the QNN SDK and builds the backend. It now runs `buck2 test //backends/qualcomm/runtime/tests:shared_buffer_test`. The suite uses fake RPC callbacks and needs no device or loaded RPC library; an actual green OSS run remains a separate verification gate. ## Compatibility and evidence limits No exported C function signature changes. Empty tensors keep their previous allocation behavior. Invalid alignment and requests exceeding the signed RPC allocation limit return `nullptr`; duplicate allocator addresses preserve the original allocation. The aggregate lookup is a C++ backend helper, not a change to model serialization. The mutex protects bookkeeping and metadata snapshots, not a registration-to-execution lease. Clients must coordinate `FreeMem` with all users of that allocation. Host tests verify allocator contracts; they do not establish device registration behavior or performance. Concrete results and earlier device evidence are recorded separately in the Test Plan. Authored with AI assistance from Codex and Claude Code. Differential Revision: D128567553 Signed-off-by: Ali Monfared <alimonfared@meta.com>
1 parent bdc7c35 commit 6727436

8 files changed

Lines changed: 405 additions & 14 deletions

File tree

‎.github/workflows/pull.yml‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -986,6 +986,9 @@ jobs:
986986
# Build QNN backend with Buck
987987
buck2 build //backends/qualcomm/...
988988
989+
# Run the host allocator regressions without a device or RPC library.
990+
buck2 test //backends/qualcomm/runtime/tests:shared_buffer_test
991+
989992
test-arm-backend-docgen:
990993
needs: [docker-image, changed-files]
991994
if: |

‎backends/qualcomm/runtime/QnnExecuTorch.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,8 @@ QNN_EXECUTORCH_EXPORT void QnnExecuTorchAddCustomMemTensorAddr(
8989
void* tensor_addr,
9090
void* custom_mem);
9191

92-
/// Free the allocated shared memory.
92+
/// Free the allocated shared memory. The caller must wait for all registration
93+
/// and execution using this allocation or its tensor aliases to finish.
9394
QNN_EXECUTORCH_EXPORT void QnnExecuTorchFreeCustomMem(void* buffer_ptr);
9495

9596
/// Register the QNN backend with the ExecuTorch runtime living in the caller's

‎backends/qualcomm/runtime/QnnManager.cpp‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -214,17 +214,13 @@ Error QnnManager::RegisterCustomMem(
214214
tensor_wrapper, data_ptr, pre_registered_handle);
215215
}
216216

217-
SharedBuffer& shared_buffer_manager = SharedBuffer::GetSharedBufferManager();
218-
219217
size_t tensor_offset = info.pos;
220-
size_t total_custom_mem_size =
221-
shared_buffer_manager.GetAllocatedSize(custom_mem_base);
222-
223-
int32_t mem_fd = shared_buffer_manager.MemToFd(custom_mem_base);
218+
SharedBuffer::AllocationInfo allocation =
219+
SharedBuffer::GetSharedBufferManager().GetAllocationInfo(custom_mem_base);
224220
// Note: If obtaining the file descriptor fails, it may be due to memory not
225221
// being released with QnnExecuTorchFreeCustomMem. In this situation, we could
226222
// consider adding a map to monitor it.
227-
if (mem_fd == -1) {
223+
if (allocation.fd == -1) {
228224
QNN_EXECUTORCH_LOG_WARN(
229225
"Tensor name %s failed to get file descriptor.",
230226
tensor_wrapper->GetName().c_str());
@@ -234,9 +230,9 @@ Error QnnManager::RegisterCustomMem(
234230
ET_CHECK_OR_RETURN_ERROR(
235231
backend_params_ptr_->qnn_mem_manager_ptr_->RegisterCustomMem(
236232
tensor_wrapper,
237-
mem_fd,
233+
allocation.fd,
238234
data_ptr,
239-
total_custom_mem_size,
235+
allocation.allocated_size,
240236
tensor_offset,
241237
info) == Error::Ok,
242238
Internal,

‎backends/qualcomm/runtime/SharedBuffer.cpp‎

Lines changed: 45 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,13 @@
77
*/
88
#include <executorch/backends/qualcomm/runtime/Logging.h>
99
#include <executorch/backends/qualcomm/runtime/SharedBuffer.h>
10+
#include <executorch/runtime/platform/compiler.h>
1011

1112
#include <pal/DynamicLoading.h>
1213

14+
#include <limits>
15+
#include <new>
16+
1317
// Refer to the QNN HTP Shared Buffer Tutorial
1418
// in Qualcomm® AI Engine Direct document
1519
constexpr uint8_t RPCMEM_HEAP_ID_SYSTEM = 25;
@@ -63,6 +67,7 @@ intptr_t alignTo(size_t alignment, intptr_t offset) {
6367
std::mutex SharedBuffer::init_mutex_;
6468

6569
void* SharedBuffer::GetCustomMemBase(void* buf) {
70+
std::lock_guard<std::mutex> lock(memory_mutex_);
6671
auto it = tensor_addr_to_custom_mem_.find(buf);
6772
if (it == tensor_addr_to_custom_mem_.end()) {
6873
return nullptr;
@@ -71,13 +76,23 @@ void* SharedBuffer::GetCustomMemBase(void* buf) {
7176
}
7277

7378
size_t SharedBuffer::GetAllocatedSize(void* buf) {
79+
std::lock_guard<std::mutex> lock(memory_mutex_);
7480
auto it = allocated_size_map_.find(buf);
7581
if (it == allocated_size_map_.end()) {
7682
return 0;
7783
}
7884
return it->second;
7985
}
8086

87+
SharedBuffer::AllocationInfo SharedBuffer::GetAllocationInfo(void* buf) {
88+
std::lock_guard<std::mutex> lock(memory_mutex_);
89+
auto it = allocated_size_map_.find(buf);
90+
if (it == allocated_size_map_.end()) {
91+
return {};
92+
}
93+
return {it->second, rpc_mem_to_fd_(buf)};
94+
}
95+
8196
SharedBuffer& SharedBuffer::GetSharedBufferManager() {
8297
std::lock_guard<std::mutex> lk(init_mutex_);
8398
static SharedBuffer shared_buffer_manager;
@@ -109,6 +124,13 @@ void* SharedBuffer::AllocMem(size_t bytes, size_t alignment) {
109124
QNN_EXECUTORCH_LOG_ERROR("Shared memory not initialized.");
110125
return nullptr;
111126
}
127+
const size_t max_bytes = std::numeric_limits<int32_t>::max();
128+
if (alignment == 0 || (alignment & (alignment - 1)) != 0 ||
129+
alignment > max_bytes || bytes > max_bytes - alignment) {
130+
QNN_EXECUTORCH_LOG_ERROR("Invalid shared memory size or alignment.");
131+
return nullptr;
132+
}
133+
std::lock_guard<std::mutex> lock(memory_mutex_);
112134
// do alignment:
113135
auto allocate_bytes = static_cast<int32_t>(bytes + alignment);
114136
void* buf = rpc_mem_alloc_(
@@ -117,18 +139,34 @@ void* SharedBuffer::AllocMem(size_t bytes, size_t alignment) {
117139
QNN_EXECUTORCH_LOG_WARN("Failed to allocate the tensor by RPC memory.");
118140
return nullptr;
119141
}
142+
// NOLINTNEXTLINE(performance-no-int-to-ptr)
120143
auto aligned_buf = reinterpret_cast<void*>(
121144
alignTo(alignment, reinterpret_cast<intptr_t>(buf)));
122-
bool status = restore_map_.insert({aligned_buf, buf}).second;
123-
allocated_size_map_.insert({aligned_buf, allocate_bytes});
124-
if (!status) {
125-
QNN_EXECUTORCH_LOG_ERROR("Failed to allocate the tensor by RPC memory.");
145+
if (restore_map_.count(aligned_buf) != 0) {
146+
// The RPC allocator returned memory that is still live, so freeing buf
147+
// would release the existing allocation.
148+
QNN_EXECUTORCH_LOG_ERROR(
149+
"RPC memory %p is already allocated.", aligned_buf);
150+
return nullptr;
151+
}
152+
#if ET_HAS_EXCEPTIONS
153+
try {
154+
#endif
155+
restore_map_.insert({aligned_buf, buf});
156+
allocated_size_map_.insert({aligned_buf, allocate_bytes});
157+
#if ET_HAS_EXCEPTIONS
158+
} catch (const std::bad_alloc&) {
159+
restore_map_.erase(aligned_buf);
126160
rpc_mem_free_(buf);
161+
QNN_EXECUTORCH_LOG_ERROR("Failed to allocate shared memory bookkeeping.");
162+
return nullptr;
127163
}
164+
#endif
128165
return aligned_buf;
129166
}
130167

131168
int32_t SharedBuffer::MemToFd(void* buf) {
169+
std::lock_guard<std::mutex> lock(memory_mutex_);
132170
int32_t memFd = -1;
133171
if (!initialize_) {
134172
QNN_EXECUTORCH_LOG_ERROR("Shared memory not initialized.");
@@ -139,6 +177,7 @@ int32_t SharedBuffer::MemToFd(void* buf) {
139177
}
140178

141179
void SharedBuffer::FreeMem(void* buf) {
180+
std::lock_guard<std::mutex> lock(memory_mutex_);
142181
if (!initialize_) {
143182
QNN_EXECUTORCH_LOG_ERROR("Shared memory not initialized.");
144183
} else if (restore_map_.count(buf) == 0) {
@@ -159,6 +198,7 @@ void SharedBuffer::FreeMem(void* buf) {
159198
}
160199

161200
bool SharedBuffer::IsAllocated(void* buf) {
201+
std::lock_guard<std::mutex> lock(memory_mutex_);
162202
return restore_map_.count(buf) != 0U;
163203
}
164204

@@ -197,6 +237,7 @@ Error SharedBuffer::Load() {
197237
}
198238

199239
void SharedBuffer::AddCusomMemTensorAddr(void* tensor_addr, void* custom_mem) {
240+
std::lock_guard<std::mutex> lock(memory_mutex_);
200241
bool status =
201242
tensor_addr_to_custom_mem_.insert({tensor_addr, custom_mem}).second;
202243
if (!status) {

‎backends/qualcomm/runtime/SharedBuffer.h‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,9 +61,19 @@ class SharedBuffer final {
6161

6262
size_t GetAllocatedSize(void* buf);
6363

64+
struct AllocationInfo {
65+
size_t allocated_size{0};
66+
int32_t fd{-1};
67+
};
68+
// Reads the size and file descriptor of an allocation under one lock, so a
69+
// concurrent FreeMem cannot leave them describing different states. Returns
70+
// {0, -1} if buf is not allocated.
71+
AllocationInfo GetAllocationInfo(void* buf);
72+
6473
void* GetCustomMemBase(void* buf);
6574

6675
private:
76+
friend class SharedBufferTest;
6777
SharedBuffer() = default;
6878

6979
// dlopen RPCMem library and dlysm required functions
@@ -88,6 +98,7 @@ class SharedBuffer final {
8898
// remain linked to this custom memory.
8999
std::unordered_map<void*, std::unordered_set<void*>>
90100
custom_mem_to_tensor_addr_;
101+
std::mutex memory_mutex_;
91102
std::atomic_bool initialize_{false};
92103
static std::mutex init_mutex_;
93104
};
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
load("@fbcode_macros//build_defs:build_file_migration.bzl", "fbcode_target", "non_fbcode_target")
2+
load(":targets.bzl", "define_common_targets")
3+
4+
oncall("executorch")
5+
6+
non_fbcode_target(_kind = define_common_targets)
7+
fbcode_target(_kind = define_common_targets)

0 commit comments

Comments
 (0)