Fix closing native program / maps handles in system worker thread. #2500
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2500 +/- ##
==========================================
+ Coverage 84.06% 84.11% +0.04%
==========================================
Files 155 157 +2
Lines 28847 29037 +190
==========================================
+ Hits 24251 24425 +174
- Misses 4596 4612 +16
|
saxena-anurag
changed the title
saxena-anurag
requested review from
dthaler,
poornagmsft,
Alan-Jowett,
shankarseal,
dv-msft,
delaramamiri and
gtrevi
as code owners
| @@ -15,6 +15,11 @@ static uint32_t _ebpf_platform_maximum_processor_count = 0; | |||
| extern DEVICE_OBJECT* | |||
| ebpf_driver_get_device_object(); | |||
|
|
|||
| typedef struct _ebpf_process_state | |||
There was a problem hiding this comment.
typedef KAPC_STATE ebpf_process_state_t ?
| return (intptr_t)process; | ||
| } | ||
|
|
||
| _Ret_maybenull_ ebpf_process_state_t* |
There was a problem hiding this comment.
?? what is the need for this function?
There was a problem hiding this comment.
Since ebpf_platform.h only contains:
typedef struct _ebpf_process_state ebpf_process_state_t;
the file which includes this does not know the size of the struct -- so sizeof(ebpf_process_state_t) will give compiler error.
| ebpf_platform_detach_process(handle_info->process_state); | ||
|
|
||
| // Release the reference on the process object. | ||
| ebpf_platform_dereference_process(handle_info->process_handle); |
There was a problem hiding this comment.
Is this needed given line 1319 ?
There was a problem hiding this comment.
Yes, 1319 line is for the cleanup path which is not called in this path.
| @@ -1408,3 +1413,37 @@ ebpf_utf8_string_to_unicode(_In_ const ebpf_utf8_string_t* input, _Outptr_ wchar | |||
| ebpf_free(unicode_string); | |||
| return retval; | |||
| } | |||
|
|
|||
| _Ret_maybenull_ ebpf_process_state_t* | |||
| ebpf_allocate_process_state() | |||
There was a problem hiding this comment.
Can you check Fault injection needed for all the new functions introduced?
I believe it is needed.
There was a problem hiding this comment.
This PR is adding the following APIs:
ebpf_allocate_process_state
ebpf_platform_reference_process
ebpf_platform_dereference_process
ebpf_platform_attach_process
ebpf_platform_detach_process
Only one of these APIs is expected to fail -- ebpf_allocate_process_state. And since that API calls ebpf_allocate(), it is already covered for fault injection.
There was a problem hiding this comment.
Thanks for checking it. Please add a comment for ebpf_allocate_process_state with
Fault injection as skipped.
Fixes #2457
Description
Earlier fix done in PR #2395 was incomplete. The previous fix queued a workitem to close the program and map handles in the case of native module load failure. Closing the handles in system worker thread fails with
INVALID_HANDLEas the handle is created for typeUserMode, and the worker thread is running in system context.The fix is to attach the current process to the worker thread to be able to close the handles. The new fix does the following:
Testing
Added new kernel tests.
Documentation
NA