unmap ring when handle count reaches zero#5308
Draft
mikeagun wants to merge 5 commits into
Draft
Conversation
F-002 (Critical): ebpf_free_ring_buffer_memory frees MDLs without calling MmUnmapLockedPages for outstanding user-mode mappings, leaving orphaned VA-to-PA mappings that can reference freed physical pages. Fix: In the free path, if user mapping state is 'fully mapped', attempt MmUnmapLockedPages in same-process context. For cross-process context (typical during process teardown on a worker thread where KeStackAttachProcess is unsafe), log a warning and rely on OS process address space teardown for VA cleanup. F-003 (High): user_process, user_consumer_address, user_producer_address fields are checked and updated with plain loads/stores — concurrent map and unmap calls can race, causing leaked process refs, incorrect process ownership, or double-unmap. Fix: Add volatile LONG user_mapping_state to ebpf_ring_descriptor_t with three-state CAS transitions (0=unmapped, 2=mapping-in-progress, 1=fully mapped). Map claims the slot with CAS 0->2, populates all fields, then transitions to 1. Unmap transitions 1->0 atomically. Free uses InterlockedExchange and only acts on state==1 (fully mapped). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add usersim CAS logic to ebpf_ring_map_user/ebpf_ring_unmap_user/ ebpf_free_ring_buffer_memory mirroring the kernel F-003 fix. This enables unit tests to validate double-map rejection. Unit test (ring_buffer_double_map_rejected): Verify second map on same ring returns EBPF_INVALID_ARGUMENT, and remap after unmap works. Kernel tests (api_test.cpp): - ring_buffer_double_map_rejected: Same double-map rejection via IOCTL - ring_buffer_destroy_with_outstanding_mapping: F-002 — close map fd without unmapping, verify no crash - ring_buffer_concurrent_map_unmap: F-003 stress — two threads race map vs unmap on same ring for 200 iterations Fix existing perf_event_array_output_capture test to reuse the first map result instead of calling ebpf_map_query_buffer twice (which now correctly fails with the CAS double-map check). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The 3-state CAS (0=unmapped, 1=mapped, 2=mapping) had a race in the unmap path: CAS(1->0) allowed a new map (0->2->1) and another unmap (1->0) to overlap with the first unmap's cleanup, causing ObDereferenceObject(NULL) BSOD (bugcheck 0x50). Add state 3 (unmapping-in-progress): unmap now uses CAS(1->3), performs cleanup, then Exchange(0). This blocks new map operations while cleanup runs, eliminating the race. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
User-mode ebpf_free_ring_buffer_memory reset user_mapping_state to 0 before freeing views, allowing a concurrent map to succeed and return pointers into views being freed. Fix: set state to 2 (in-progress) which map's CAS(0->1) rejects. Also update the kernel struct comment to document all 4 states (0=unmapped, 1=mapped, 2=mapping, 3=unmapping). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
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
1 participant
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.
Description
Describe the purpose of and changes within this Pull Request.
Please reference an issue with a keyword such as Fixes #abc, Closes #xyz, etc., so the work can be tracked.
Testing
Do any existing tests cover this change? Are new tests needed?
If new tests were added:
Documentation
Is there any documentation impact for this change?
Installation
Is there any installer impact for this change?