Skip to content

[netebpfext] Add per-provider WFP handles to avoid improper use from parallel invocations #3866

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 14 commits into from
Oct 10, 2024
Merged

[netebpfext] Add per-provider WFP handles to avoid improper use from parallel invocations #3866

merged 14 commits into from
Oct 10, 2024

Conversation

matthewige
Copy link
Contributor

@matthewige matthewige commented Sep 24, 2024 •
edited
Loading

Description

Issue:
Our KM stress tests revealed an issue in our netebpfext code. There is a single global WFP handle, but multiple threads could use this at the same time (such as two programs attaching in parallel), leading to errors in the WFP APIs.

Fix:

  • Add per-provider handles
  • Make all WFP engine handles use static sessions (this allows a separate WFP handle to access the same objects, such as the sublayer/callouts)
  • Add proper cleanup logic to remove all WFP objects

Closes #3607

Testing

Existing tests validate this functionality.

Documentation

None.

Installation

None.

@matthewige matthewige changed the title [draft] [draft] [netebpfext] Add per-provider WFP handles to avoid improper use from parallel invocations Sep 26, 2024
@matthewige matthewige changed the title [draft] [netebpfext] Add per-provider WFP handles to avoid improper use from parallel invocations [netebpfext] Add per-provider WFP handles to avoid improper use from parallel invocations Sep 27, 2024
@matthewige matthewige marked this pull request as ready for review September 27, 2024 16:56
shankarseal
shankarseal reviewed Sep 28, 2024
View reviewed changes
shankarseal
shankarseal reviewed Sep 28, 2024
View reviewed changes
shankarseal
shankarseal reviewed Sep 28, 2024
View reviewed changes
shankarseal
shankarseal reviewed Sep 28, 2024
View reviewed changes
saxena-anurag
saxena-anurag reviewed Oct 2, 2024
View reviewed changes
shankarseal
shankarseal previously approved these changes Oct 2, 2024
View reviewed changes
@matthewige
CR - move filter unregister to after filter delete, replace null chec…
2e05507 
…ks with asserts, remove unneded helper function
@Alan-Jowett Alan-Jowett added this pull request to the merge queue Oct 10, 2024
Merged via the queue into microsoft:main with commit 8fb3a7e Oct 10, 2024
88 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Alan-Jowett Alan-Jowett Alan-Jowett approved these changes

@saxena-anurag saxena-anurag saxena-anurag approved these changes

@dthaler dthaler Awaiting requested review from dthaler dthaler is a code owner

@poornagmsft poornagmsft Awaiting requested review from poornagmsft poornagmsft is a code owner

@dv-msft dv-msft Awaiting requested review from dv-msft

@gtrevi gtrevi Awaiting requested review from gtrevi

@shpalani shpalani Awaiting requested review from shpalani

@mtfriesen mtfriesen Awaiting requested review from mtfriesen mtfriesen is a code owner

@rectified95 rectified95 Awaiting requested review from rectified95

@shankarseal shankarseal Awaiting requested review from shankarseal shankarseal is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Workflow failed - km_mt_stress_tests
4 participants
@matthewige @Alan-Jowett @saxena-anurag @shankarseal