Design proposal for multi-attach support in netebpfext
#3764
Conversation
This file contains 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
saxena-anurag
requested review from
dthaler,
poornagmsft,
Alan-Jowett,
shankarseal,
dv-msft,
gtrevi,
shpalani,
matthewige,
mtfriesen and
rectified95
as code owners
shankarseal
reviewed
Aug 15, 2024
| multiple legacy apps using older libbpf APIs to attach programs can still attach to the same hook, with the programs | ||
| being "appended" in the invoke list in the order they are attached. | ||
|
|
||
| # Current Design |
There was a problem hiding this comment.
I think the current design section can be deleted unless in the new design section you refer to some of these. In any ways, for better readability you can significantly shorten this section.
shankarseal
reviewed
Aug 15, 2024
| required data structures changes are described below. | ||
|
|
||
| Even though multiple programs will attach with same "attach params", there will be a single instance of the | ||
| corresponding WFP filters added. This means there will be one-to-many mapping from filter context to client context. |
There was a problem hiding this comment.
For the benefit of the reader please establish that the client to the hook is an eBPF program. So program and client are used interchangeably in this doc.
shankarseal
reviewed
Aug 15, 2024
| being attached / detached. Client / attach operations are expected to be much less frequent than program invocations. | ||
| This approach eliminates any need for rundown reference for the client context. Since the list of client contexts for a | ||
| filter context is synchronized via a lock, it is always guaranteed that in the client detach flow, if the dispatch level | ||
| lock is acquired, there is no program invocation in progress for that program. Also, once the client context is removed |
There was a problem hiding this comment.
"if the dispatch level write lock is acquired" ....
There was a problem hiding this comment.
Lines 89-90 and 114 don't use the term "read" or "write" lock, they use "exclusive" or "shared".
shankarseal
reviewed
Aug 15, 2024
| 5. Create filter context and client context. | ||
| 6. Configure WFP filters --> We are not holding any dispatch level lock. IRQL is PASSIVE_LEVEL. | ||
| 7. Acquire dispatch lock (exclusive) --> Synchronizes access to the client and filter context lists. | ||
| 8. Insert the new filter context and client context in the corresponding queues. |
There was a problem hiding this comment.
What happens if WFP API fails?
Alan-Jowett
reviewed
Aug 15, 2024
| 1. Acquire passive lock --> Serializes multiple attach / detach calls to the same hook provider. | ||
| 2. Acquire dispatch lock (exclusive). --> Synchronizes access to the client and filter context lists. | ||
| 3. Check if a matching filter context already exists. If it exists, insert the new client context, and return. | ||
| 4. If no matching filter context exists, release dispatch lock. |
There was a problem hiding this comment.
What happens if the state changes after this lock is released? Won't this permit multiple callers to create the WFP context?
There was a problem hiding this comment.
The passive lock (acquired in step 1) still serializes attach / detach calls to this provider.
dthaler
reviewed
Aug 17, 2024
| @@ -0,0 +1,126 @@ | |||
| # Introduction | |||
|
|
|||
| Multi-attach support allows multiple eBPF programs to be attached to the same attach point (aka "hook") with the | |||
There was a problem hiding this comment.
The filename implies this proposal is only for netebpfext and not other extensions.
The title of the document (line 1) and first paragraph are generic though.
Assuming this is really just about netebpfext and won't work in general, please update the doc title.
| # Introduction | ||
|
|
||
| Multi-attach support allows multiple eBPF programs to be attached to the same attach point (aka "hook") with the | ||
| same attach parameters. For example, for `BPF_CGROUP_INET4_CONNECT` attach type, the attach parameter is the |
There was a problem hiding this comment.
Suggested change
| same attach parameters. For example, for `BPF_CGROUP_INET4_CONNECT` attach type, the attach parameter is the | |
| same attach parameters. For example, for the `BPF_CGROUP_INET4_CONNECT` attach type, the attach parameter is the |
| compartment ID. With this feature, multiple eBPF programs can be attached to the same compartment ID. | ||
|
|
||
| This document describes the design changes required in `netebpfext` to support multiple attach of eBPF programs in | ||
| `netebpfext`. This is independent of the platform changes needed in BPF runtime to add support for new multi-attach |
There was a problem hiding this comment.
Suggested change
| `netebpfext`. This is independent of the platform changes needed in BPF runtime to add support for new multi-attach | |
| `netebpfext`. This is independent of the platform changes needed in the BPF runtime to add support for new multi-attach |
|
|
||
| This document describes the design changes required in `netebpfext` to support multiple attach of eBPF programs in | ||
| `netebpfext`. This is independent of the platform changes needed in BPF runtime to add support for new multi-attach | ||
| APIs and flags. These changes in netebpfext also help enable the "default" behavior of multi-attach feature where the |
There was a problem hiding this comment.
Suggested change
| APIs and flags. These changes in netebpfext also help enable the "default" behavior of multi-attach feature where the | |
| APIs and flags. These changes in netebpfext also help enable the "default" behavior of the multi-attach feature where |
| `netebpfext`. This is independent of the platform changes needed in BPF runtime to add support for new multi-attach | ||
| APIs and flags. These changes in netebpfext also help enable the "default" behavior of multi-attach feature where the | ||
| multiple legacy apps using older libbpf APIs to attach programs can still attach to the same hook, with the programs | ||
| being "appended" in the invoke list in the order they are attached. |
There was a problem hiding this comment.
Suggested change
| being "appended" in the invoke list in the order they are attached. | |
| being "appended" to the invoke list in the order they are attached. |
|
|
||
| To provide synchronization, the hook provider will maintain a *dispatch* level RW lock to synchronize access to these | ||
| lists. (A dispatch level lock is chosen here as the WFP callouts can be invoked at either PASSIVE_LEVEL or DISPATCH_LEVEL). | ||
| 1. Attach and detach callbacks flow will acquire this lock in exclusive mode. |
There was a problem hiding this comment.
Suggested change
| 1. Attach and detach callbacks flow will acquire this lock in exclusive mode. | |
| 1. Attach and detach callback flows will acquire this lock in exclusive mode. |
| being attached / detached. Client / attach operations are expected to be much less frequent than program invocations. | ||
| This approach eliminates any need for rundown reference for the client context. Since the list of client contexts for a | ||
| filter context is synchronized via a lock, it is always guaranteed that in the client detach flow, if the dispatch level | ||
| lock is acquired, there is no program invocation in progress for that program. Also, once the client context is removed |
There was a problem hiding this comment.
Lines 89-90 and 114 don't use the term "read" or "write" lock, they use "exclusive" or "shared".
| new invocation can also happen. | ||
|
|
||
| Along with the above synchronization, there is also a need to **serialize multiple attach and detach operations callbacks**, | ||
| as the whole flow of creating filter context, adding filter context to the provider list, and configuring WFP filters |
There was a problem hiding this comment.
Suggested change
| as the whole flow of creating filter context, adding filter context to the provider list, and configuring WFP filters | |
| as the whole flow of creating filter context, adding the filter context to the provider list, and configuring WFP filters |
|
|
||
| Along with the above synchronization, there is also a need to **serialize multiple attach and detach operations callbacks**, | ||
| as the whole flow of creating filter context, adding filter context to the provider list, and configuring WFP filters | ||
| needs to happen atomically (as mentioned in the current design section, there is bug currently where a lock is not held |
There was a problem hiding this comment.
Suggested change
| needs to happen atomically (as mentioned in the current design section, there is bug currently where a lock is not held | |
| needs to happen atomically (as mentioned in the current design section, there is a bug currently where a lock is not held |
Please add the issue # of the current bug.
|
|
||
| As the above mentioned dispatch level RW already serializes attach / detach callbacks for a hook provider, it could have | ||
| solved this serialization problem also. But there is a problem with using dispatch level locks: WFP APIs for adding | ||
| filters require PASSIVE_LEVEL, hence same DISPATCH_LEVEL lock cannot be used to serialize the attach and detach |
There was a problem hiding this comment.
Suggested change
| filters require PASSIVE_LEVEL, hence same DISPATCH_LEVEL lock cannot be used to serialize the attach and detach | |
| filters require PASSIVE_LEVEL, hence the same DISPATCH_LEVEL lock cannot be used to serialize the attach and detach |
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.
Description
This PR adds design proposal for adding multi-attach support for hooks in
netebpfext.Testing
NA
Documentation
This PR adds documentation.
Installation
No.