aya: support socket filter reuseport attach#1578
Conversation
✅ Deploy Preview for aya-rs-docs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
tamird
left a comment
There was a problem hiding this comment.
@tamird reviewed 5 files and all commit messages, and made 6 comments.
Reviewable status: 5 of 7 files reviewed, 6 unresolved discussions (waiting on swananan).
aya/src/programs/mod.rs line 78 at r1 (raw file):
// `libc` exposes `SO_ATTACH_REUSEPORT_EBPF` on all architectures, but // `SO_DETACH_REUSEPORT_BPF` is still commented out in libc's // `src/unix/linux_like/linux/arch/{mips,powerpc,sparc}/mod.rs`.
can we fix upstream? did we already discuss this?
aya/src/programs/socket_filter.rs line 88 at r1 (raw file):
/// A program used to inspect and filter incoming packets on a socket. /// /// [`SocketFilter`] programs can be attached as regular socket filters with
this is interesting - this really means that these are two different program types. it's hard to imagine a program that would make sense for both attachment types.
should we encode the attachment type on the program itself so that the constant we pass is implied?
aya/src/programs/socket_filter.rs line 91 at r1 (raw file):
/// [`SocketFilterAttachType::SocketFilter`]. In that mode, the return value is /// interpreted as a packet length: `0` drops the packet, and a non-zero value /// accepts or trims it.
what do you mean by "or" here?
aya/src/programs/socket_filter.rs line 169 at r1 (raw file):
/// already-attached error. The program return value is interpreted as a /// packet length: `0` drops the packet, and a non-zero value accepts or /// trims it.
this last sentence is redundant and misplaced - it already exists on the type
Code quote:
/// already-attached error. The program return value is interpreted as a
/// packet length: `0` drops the packet, and a non-zero value accepts or
/// trims it.aya/src/programs/socket_filter.rs line 204 at r1 (raw file):
} /// Detaches the current program for `attach_type` from the given socket.
There's something I don't understand: is there one slot per attachment type or one slot total? What happens if you attach with one and attempt to detach with the other?
test/integration-common/src/lib.rs line 187 at r1 (raw file):
} pub mod socket_filter {
logical separation between groups please
8e97f3e to
fd0b17d
Compare
swananan
left a comment
There was a problem hiding this comment.
@swananan made 6 comments.
Reviewable status: 3 of 7 files reviewed, 6 unresolved discussions (waiting on tamird).
aya/src/programs/mod.rs line 78 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
can we fix upstream? did we already discuss this?
Yes, rust-lang/libc#5081 has been merged, but the change haven't been released yet. So I just moved constants here for now. I will open a follow up PR, once the libc releases a new version.
aya/src/programs/socket_filter.rs line 88 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
this is interesting - this really means that these are two different program types. it's hard to imagine a program that would make sense for both attachment types.
should we encode the attachment type on the program itself so that the constant we pass is implied?
Got your point, these programs won't make sense for both attachment modes because the return values semantics differ.
But in my understanding, aya loader follows libbpf section rules, so I don't think we should add SEC("socket/reuseport"). libbpf only has SEC("socket")` for socket filters program type.
https://github.com/libbpf/libbpf/blob/dd92bef7f6c7a00bb0312554119b6d9cf38e4f32/src/libbpf.c#L9979-L9981
aya/src/programs/socket_filter.rs line 91 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
what do you mean by "or" here?
Done.
aya/src/programs/socket_filter.rs line 169 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
this last sentence is redundant and misplaced - it already exists on the type
Done.
aya/src/programs/socket_filter.rs line 204 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
There's something I don't understand: is there one slot per attachment type or one slot total? What happens if you attach with one and attempt to detach with the other?
Good question, I hadn't realized this case (:
I tested it, and reuseport and socket filter don't share the same kernel slot. Attaching one attachment type and detaching with the other returns ENOENT instead of removing the other attachment. I added a test case covering both directions.
IMO, UDP socket would be more appropriate for this test. To keep it consistent with the existing tests, I only added a smoke test using a TCP listener.
test/integration-common/src/lib.rs line 187 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
logical separation between groups please
Done.
tamird
left a comment
There was a problem hiding this comment.
@tamird reviewed 3 files and all commit messages, made 2 comments, and resolved 4 discussions.
Reviewable status: 5 of 7 files reviewed, 2 unresolved discussions (waiting on swananan).
aya/src/programs/mod.rs line 78 at r1 (raw file):
Previously, swananan (swananan) wrote…
Yes, rust-lang/libc#5081 has been merged, but the change haven't been released yet. So I just moved constants here for now. I will open a follow up PR, once the libc releases a new version.
Ah ok! Might be good to add a TODO(rust-lang/libc@95c9572): ...
the link to the commit makes it easy to check in one click if the commit is included in a release
aya/src/programs/socket_filter.rs line 88 at r1 (raw file):
Previously, swananan (swananan) wrote…
Got your point, these programs won't make sense for both attachment modes because the return values semantics differ.
But in my understanding, aya loader follows libbpf section rules, so I don't think we should add
SEC("socket/reuseport"). libbpf only hasSEC("socket")` for socket filters program type.
https://github.com/libbpf/libbpf/blob/dd92bef7f6c7a00bb0312554119b6d9cf38e4f32/src/libbpf.c#L9979-L9981
I see. Well, could we still model it as two different program types in aya that both impl TryFrom for the underlying "libbpf type"?
fd0b17d to
6b515f7
Compare
swananan
left a comment
There was a problem hiding this comment.
@swananan made 2 comments.
Reviewable status: 3 of 8 files reviewed, 2 unresolved discussions (waiting on tamird).
aya/src/programs/mod.rs line 78 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Ah ok! Might be good to add a TODO(rust-lang/libc@95c9572): ...
the link to the commit makes it easy to check in one click if the commit is included in a release
Done.
aya/src/programs/socket_filter.rs line 88 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
I see. Well, could we still model it as two different program types in aya that both impl TryFrom for the underlying "libbpf type"?
Done. Added a separate ReusePortSocketFilter program abstraction for reuseport socket filters, which makes the API more explicit. This still has some constraints because Aya follows libbpf’s SEC("socket") section rules, so I documented those as well.
|
This looks like a GitHub-side CI failure rather than a code regression. |
cd5b5cc to
2c56b1f
Compare
Add ReusePortSocketFilter as the aya program abstraction for
attaching a BPF_PROG_TYPE_SOCKET_FILTER program as a SO_REUSEPORT
selector. Regular socket filters and reuseport selectors use different
attachment paths, different kernel slots, and different return-value
semantics, so modeling them as separate aya program views makes the
user-facing API explicit.
Aya still follows libbpf section rules: socket filter programs load
from SEC("socket"), and aya does not introduce a socket/reuseport
section. Since the section cannot distinguish the two views,
ReusePortSocketFilter is selected by the user's try_into target type
over the same loaded program type.
Fixes aya-rs#441.
2c56b1f to
793bd24
Compare
tamird
left a comment
There was a problem hiding this comment.
@codex review
@tamird reviewed 6 files and all commit messages, made 6 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on swananan).
test/integration-test/src/tests/prog_test_run.rs line 22 at r4 (raw file):
} fn skip_if_socket_filter_test_run_unsupported() -> bool {
i left this kind of comment on another recent PR: the word "skip" in this function name does not make sense.
but also why did we need to extract this function?
test/integration-test/src/tests/socket_filter.rs line 32 at r4 (raw file):
const ACCEPT_TIMEOUT: Duration = Duration::from_secs(10); const TEST_PAYLOAD: &[u8] = b"hello-world"; const LISTEN_BACKLOG: u32 = SOMAXCONN as u32;
do we need this? why not pass 0 or 1?
test/integration-test/src/tests/socket_filter.rs line 41 at r4 (raw file):
} fn skip_if_reuseport_detach_unsupported() -> bool {
same comment as above
test/integration-test/src/tests/socket_filter.rs line 84 at r4 (raw file):
// https://github.com/torvalds/linux/blob/v6.9/net/core/sock_reuseport.c#L124-L130 let first_accept = async { let (_stream, _) = first
why even bind this?
test/integration-test/src/tests/socket_filter.rs line 193 at r4 (raw file):
} let _netns = NetNsGuard::new();
do we need all these guards? why?
Add SocketFilterAttachType so SocketFilter can attach either as a regular socket filter with SO_ATTACH_BPF or as a reuseport selector with SO_ATTACH_REUSEPORT_EBPF.
Fixes #441.
Added/updated tests?
We strongly encourage you to add a test for your changes.
Checklist
cargo +nightly fmt.You can find failing lints with
cargo xtask clippy.cargo test.cargo xtask public-api --bless.(Optional) What GIF best describes this PR or how it makes you feel?
This change is