tests/socket: Add bind multi-attach test suite and framework extension (#5338)#5381
Open
mikeagun wants to merge 15 commits into
Open
tests/socket: Add bind multi-attach test suite and framework extension (#5338)#5381mikeagun wants to merge 15 commits into
mikeagun wants to merge 15 commits into
Conversation
native_module_helper_t owns a generated .sys file that the destructor
deletes when _delete_file_on_destruction is true. The class previously
relied on the implicit copy/move members:
- The implicit copy ctor copied _delete_file_on_destruction and
_file_name verbatim, so two instances could end up racing to delete
the same file.
- The implicit move ctor moved _file_name (which leaves the moved-from
string in an implementation-defined valid state) but only copied the
bool flag, leaving the moved-from instance still claiming ownership.
Whether the source destructor then attempted to delete the wrong file
depended on std::string's moved-from representation.
Make the type non-copyable and add explicit noexcept move operations that
transfer ownership of _delete_file_on_destruction and clear it on the
source, so only the destination's destructor deletes the file.
This is required for multi-attach tests that push several
native_module_helper_t instances into std::vector containers, which
may reallocate and move-construct elements.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The sock_addr bind hook's process_verdict callback silently ignored address/port writes by attached programs (the WFP ALE_RESOURCE_ASSIGNMENT layer does not support context rewrite) but did not restore the bpf_sock_addr_t context between program invocations. Under multi-attach, an earlier program could mutate context fields such as user_ip / user_port / msg_src_* and those mutated values would then be visible to any subsequent attached program, allowing one program to influence the policy decisions of another. Rename _net_ebpf_extension_sock_addr_bind_process_verdict to _net_ebpf_extension_sock_addr_authorize_process_verdict and have it restore *sock_addr_ctx = *original_context after each program, reusing the existing original_context snapshot pattern used by the CONNECT_AUTHORIZATION accumulator. Update bind_classify to set original_context from a stack-local snapshot of the WFP-populated context before invoking programs. The accumulator is also intended to be used by the sock_addr listen hook once it becomes multi-attach. Naming it for the abstract role (authorization gates that do not support context rewrite) rather than for the bind hook avoids a second rename when listen is wired in. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ions
Microsoft's FWPS_CLASSIFY_OUT0 contract gates writes to actionType on
the FWPS_RIGHT_ACTION_WRITE bit; a callout with higher weight can
terminate WFP arbitration and revoke that right before reaching our
callout. The sock_addr bind, listen, recv_accept, and
authorize_connection classify routines wrote actionType unconditionally
on entry and from the verdict-driven switch in their Exit blocks, so
they could attempt to overwrite a prior terminating decision.
Mirror the early-exit rights check already used by
net_ebpf_extension_sock_addr_redirect_connection_classify:
- For bind, listen, and recv_accept: bail out before the default
actionType = FWP_ACTION_PERMIT write.
- For authorize_connection: set a rights_revoked flag and gate the
Exit-block switch on it, since that classify defers actionType
writes to Exit.
This is independent of multi-attach but visible alongside it, since
multi-attach configurations are more likely to share filter layers
with other callouts.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Multi-attach tests for the sock_addr bind and listen hooks need to bind
and listen inside specific network compartments to exercise wildcard +
compartment-specific filter selectivity. The existing socket_helper
classes create and bind sockets on the current thread compartment with
no way to override.
Add an optional compartment_id parameter (default
UNSPECIFIED_COMPARTMENT_ID = leave compartment untouched, current
behavior) to every public socket_helper constructor and forward it
through the inheritance chain.
The new compartment_scope RAII helper in socket_helper.cpp brackets
the relevant socket calls with SetCurrentThreadCompartmentId save /
restore, so:
- _base_socket creates and binds the socket inside the requested
compartment.
- _stream_server_socket additionally re-enters the compartment for
listen() and the accept-socket setup, since the base ctor's scope
has already destructed by the time the derived ctor body runs.
UNSPECIFIED_COMPARTMENT_ID is a no-op fast path, so existing call sites
that omit the parameter are bit-for-bit identical to before.
iphlpapi.lib is pulled in via #pragma comment(lib, ...) in
socket_helper.cpp to avoid touching the test project files.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add per-module map discovery, program_policies for targeted map updates, attachment_step for dynamic detach/reattach, and compartment_id on program_spec for compartment-aware attachment. Existing single-program call sites remain source-identical and behavior-identical. New framework types: - program_ref: bounds-checked reference to a specific module/program - program_policy_spec: per-module verdict/policy configuration - attachment_step/attachment_action: dynamic attach/detach between steps - counter_expectation: reserved for future invocation counting The legacy global-first-match map discovery is preserved as a fallback for callers that do not use program_policies. When program_policies is non-empty, per-module maps are used instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add 9 multi-program bind test scenarios exercising the verdict accumulation, short-circuit, hard-permit-over-WFP, and dynamic detach/reattach behaviors of the sock_addr-aligned bind hook (cgroup/bind4 / cgroup/bind6). Each scenario runs across TCP/UDP and IPv4/IPv6 via TEMPLATE_TEST_CASE. Test scenarios: - All programs PROCEED_SOFT → bind allowed - Second program REJECT after soft → bind denied - First program REJECT (short-circuit) → bind denied - PROCEED_SOFT + PROCEED_HARD mix → hard permit (higher priority) - Multi-program soft blocked by WFP filter → bind denied - Multi-program hard overrides WFP filter → bind allowed - Detach middle program (REJECT) → bind recovers to allow - Detach and reattach with changed verdict → bind denied - Three programs all soft → bind allowed Helper functions added: - sock_addr_bind_module(): creates a module_spec for the bind hook - sock_addr_bind_verdict(): creates a per-module policy spec - sock_addr_bind_wfp_layer(): returns the WFP layer GUID for bind All tests tagged [bind_tests][multi_attach] for selective execution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Expand the brief mention of multi-attach behavior in BindHook.md with detailed documentation of: - Verdict priority ordering (REJECT > PROCEED_HARD > PROCEED_SOFT) - Short-circuit on REJECT behavior - WFP interaction with accumulated verdicts (soft vs hard permit) - Default verdict when no programs are attached - Unknown/invalid verdict treatment Add a Multi-Attach Test Coverage table summarizing the 9 test scenarios now exercised in socket_tests.cpp [bind_tests][multi_attach]. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fix FR-1/BT-4: Check bpf_prog_detach2 return code in detach_program helper — SAFE_REQUIRE(rc == 0) before clearing the attached flag. Add coverage for adversarial review gaps: - HARD-first ordering with WFP (BT-2): [HARD, SOFT] + WFP → allow - REJECT + HARD combinations (BT-9): both [REJECT, HARD] and [HARD, REJECT] → bind denied (REJECT priority > HARD) - Third-program-decisive (BT-7): [SOFT, SOFT, REJECT] → denied and [SOFT, SOFT, HARD] + WFP → allowed - Detach first and detach last (BT-8/F2): asymmetric detach positions Remove unused TCP_ONLY_PARAMS macro (BT-13). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… cleanup Move the FWPS_RIGHT_ACTION_WRITE check in authorize_connection_classify to after the connection context cache lookup. Previously, the rights check at the top of the function caused an early exit that skipped _net_ebpf_ext_find_and_remove_connection_context, leaving stale cache entries from the redirect layer when a higher-weight WFP callout had revoked write permission. The cache cleanup must run unconditionally so redirect-layer entries are always consumed, regardless of whether this callout can write the classify output. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mikeagun
force-pushed
the
pr1-bind-multi-attach
branch
from
June 19, 2026 20:57
bde16ec to
8f1802a
Compare
…rograms The socket test framework's detach_program helper uses bpf_prog_detach2 uniformly for all attach types. However, BPF_ATTACH_TYPE_BIND was not included in _does_attach_type_support_attachable_fd, causing detach to return -ENOTSUP (-129) for legacy bind programs (bind_policy tests). Add BPF_ATTACH_TYPE_BIND to the whitelist. The bind provider accepts attach data but ignores it (always global scope), so this is safe for both the attach and detach paths. Also remove the unused counter_expectations scaffolding from the test framework -- it was never populated by any test and added reader overhead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove the compartment_id parameter from all socket helper constructors and the compartment_scope RAII class. This infrastructure is unused by any current bind multi-attach test (no test sets program_spec.compartment_id) and will be reintroduced in a focused follow-up branch. Changes: - Remove compartment_id param from _base_socket, _client_socket, _datagram_client_socket, _stream_client_socket, _server_socket, _datagram_server_socket, _stream_server_socket constructors - Remove compartment_scope class and iphlpapi dependency - Remove compartment_id field from program_spec struct - Simplify attach_program/detach_program lambdas to use hardcoded 0 (wildcard compartment), which is the value all tests were using Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mikeagun
marked this pull request as ready for review
June 24, 2026 16:41
mikeagun
requested review from
Alan-Jowett,
LakshK98,
dthaler,
matthewige,
mtfriesen,
poornagmsft,
saxena-anurag and
shankarseal
as code owners
June 24, 2026 16:41
dthaler
reviewed
Jun 25, 2026
Comment on lines
+2092
to
+2095
| // Restore the context so the next attached program sees the original | ||
| // WFP-provided values, not whatever the previous program may have written | ||
| // to user_ip / user_port / msg_src_*. | ||
| *sock_addr_ctx = *original_context; |
Collaborator
There was a problem hiding this comment.
Why is this done here instead of by the caller?
Contributor
Author
There was a problem hiding this comment.
The context restore has to run between each program in a multi-attach invocation, and the per-program process_verdict callback is the only place with that granularity — the generic hook-provider loop (net_ebpf_ext_hook_provider.c) invokes each attached program and calls process_verdict after each one, whereas the classify caller runs once around the whole batch. Keeping it in the callback also keeps the sock_addr-specific knowledge (which bpf_sock_addr_t fields must be reset) out of the hook-agnostic loop. The connect/redirect variant _net_ebpf_extension_sock_addr_process_verdict does the analogous restore for the same reason.
Alan-Jowett
reviewed
Jul 7, 2026
| case BPF_CGROUP_INET4_LISTEN: | ||
| case BPF_CGROUP_INET6_LISTEN: | ||
| case BPF_CGROUP_SOCK_OPS: | ||
| case BPF_ATTACH_TYPE_BIND: |
Member
There was a problem hiding this comment.
Adding BPF_ATTACH_TYPE_BIND here makes the libbpf-compat path advertise target-fd semantics for the legacy bind hook, but the kernel side still ignores that attach parameter entirely.
bpf_prog_attach(prog_fd, attachable_fd, BPF_ATTACH_TYPE_BIND, 0) now always forwards a 4-byte attachable_fd payload via ebpf_program_attach_by_fd(). However, the legacy bind provider's _net_ebpf_ext_bind_validate_client_data() explicitly says bind does not require any client data and ignores it, and _net_ebpf_ext_bind_create_filter_context() installs the same filters with zero conditions regardless of the supplied bytes. So bpf_prog_attach(..., 1234, BPF_ATTACH_TYPE_BIND, 0) will succeed, but it will not attach "to 1234"; it will silently behave like the wildcard/global bind attach.
That is a real API regression, not just a cosmetic mismatch: callers can believe they scoped the program to a specific target when they actually enabled it for all bind operations, and bpf_prog_detach2(..., 1234, BPF_ATTACH_TYPE_BIND) is equally misleading because the kernel never used that value to distinguish attachments.
I think this should either stay unsupported on the bpf_prog_attach path, or be accepted only for the wildcard case (for example attachable_fd == 0) with the no-client-data semantics made explicit.
Contributor
Author
There was a problem hiding this comment.
Good catch — went with option 2. bpf_prog_attach/bpf_prog_detach2 now reject a non-zero attachable_fd for BPF_ATTACH_TYPE_BIND with EINVAL instead of silently treating it as the wildcard, and the wildcard-only/no-client-data semantics are spelled out in a comment. The whitelist entry stays because the wildcard is encoded as a 4-byte zero payload that bpf_prog_detach2 relies on to locate the link (ebpf_program_detach only sets attach_data_present when size > 0, and the core unlink path only searches links when that flag is set). Added a unit test asserting non-zero fd → -EINVAL for both attach and detach.
Address PR feedback on the libbpf-compat attach path. The legacy bind hook (BPF_ATTACH_TYPE_BIND) has no per-target attach parameter and its provider ignores any supplied client data, so bpf_prog_attach/bpf_prog_detach2 would accept a non-zero attachable_fd and silently treat it as the wildcard attachment -- giving callers the false impression that a program was scoped to a specific target. Reject a non-zero attachable_fd for such attach types with EINVAL. The wildcard value is still encoded as a 4-byte zero payload, which bpf_prog_detach2 relies on to locate the matching link to detach. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b71da926-7c8c-4526-87ea-084c89545896
Address PR feedback on the bind multi-attach test suite: replace non-ASCII arrows and dashes in comments and test descriptions with ASCII equivalents, and spell out the PROCEED_SOFT / PROCEED_HARD verdict names in the scenario comments so they match the enum used in the code. Comment/description text only; no test behavior changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b71da926-7c8c-4526-87ea-084c89545896
Address PR feedback on docs/BindHook.md: simplify the "Hard overrides WFP" scenario row to the minimal programs that demonstrate it, and add a note that the "no subsequent WFP filter can override" guarantee for PROCEED_HARD applies to WFP filters only -- among eBPF programs the most-restrictive verdict wins, so a later program's REJECT still denies a bind another program permitted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b71da926-7c8c-4526-87ea-084c89545896
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
5 participants
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
Multi-attach support for the sock_addr bind hook (
cgroup/bind4/cgroup/bind6), including production-code fixes discovered during audit and a new multi-program test framework with 15 bind multi-attach test scenarios.Closes #5338.
Production-code fixes (discovered during audit)
bpf_sock_addr_tbetween multi-attach program invocations to prevent context poisoning across programs.BPF_ATTACH_TYPE_BINDto_does_attach_type_support_attachable_fdsobpf_prog_detach2works for bind programs.Test framework extension
program_policiesfor targeted per-module verdict configurationattachment_stepfor dynamic detach/reattach between test stepsattach_program/detach_programlambdas with explicit return-code verificationprogram_refresolverBind multi-attach test suite (15 scenarios × TCP/UDP × IPv4/IPv6)
Infrastructure
native_module_helper_tmove semantics fix (prevents double-delete on vector reallocation)Testing
New bind multi-attach tests added to cover verdict combination and dynamic attachment scenarios.
Documentation
docs/BindHook.mdadded: verdict combination semantics, priority ordering, WFP interaction rules, and test coverage table.Installation
No installer impact.