Skip to content
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.

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

Add Support For Flow Dissector Programs #802

Closed

Conversation

azenna
Copy link
Contributor

@azenna azenna commented Oct 1, 2023

Adds support for flow dissector programs. Only (1/5) know what I am doing and would appreciate feedback!
#216


This change is Reviewable

@netlify
Copy link

netlify bot commented Oct 1, 2023

Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 8d1e906
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/67c8cb5aae4c390009833a9d
😎 Deploy Preview https://deploy-preview-802--aya-rs-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mergify mergify bot added aya This is about aya (userspace) aya-bpf This is about aya-bpf (kernel) aya-obj Relating to the aya-obj crate labels Oct 1, 2023
Copy link
Member

@vadorovsky vadorovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great so far!

Apart from one comment I posted, it would be nice to add an integration test.

@azenna azenna force-pushed the 216-add-support-for-flow-dissector branch 2 times, most recently from 0ab5fe6 to c457da0 Compare October 1, 2023 22:32
@mergify mergify bot added the test A PR that improves test cases or CI label Oct 1, 2023
@azenna
Copy link
Contributor Author

azenna commented Oct 1, 2023

Added a integration test that's mostly a clone of the basic_uprobe integration test. Is this the right test or do we want something more comprehensive?

@azenna azenna force-pushed the 216-add-support-for-flow-dissector branch 2 times, most recently from bf3efa7 to 1c58716 Compare October 2, 2023 13:49
@mergify
Copy link

mergify bot commented Oct 2, 2023

Hey @alessandrod, this pull request changes the Aya Public API and requires your review.

@mergify mergify bot added the api/needs-review Makes an API change that needs review label Oct 2, 2023
@azenna azenna marked this pull request as ready for review October 2, 2023 13:54
@azenna azenna marked this pull request as draft October 5, 2023 18:45
@mergify
Copy link

mergify bot commented Oct 10, 2023

@azenna, this pull request is now in conflict and requires a rebase.

@mergify mergify bot added the needs-rebase label Oct 10, 2023
Copy link

mergify bot commented Feb 6, 2024

Hey @alessandrod, this pull request changes the Aya Public API and requires your review.

Copy link

mergify bot commented Feb 6, 2024

@azenna, this pull request is now in conflict and requires a rebase.

@mergify mergify bot requested a review from alessandrod March 5, 2024 09:02
@mergify mergify bot removed the needs-rebase label Jan 1, 2025
Copy link

mergify bot commented Jan 1, 2025

@azenna, this pull request is now in conflict and requires a rebase.

@mergify mergify bot added the needs-rebase label Jan 1, 2025
@tamird tamird force-pushed the 216-add-support-for-flow-dissector branch from 1c58716 to 1d85f5e Compare March 5, 2025 21:32
@mergify mergify bot removed the needs-rebase label Mar 5, 2025
@tamird tamird force-pushed the 216-add-support-for-flow-dissector branch from 1d85f5e to de8963b Compare March 5, 2025 21:37
@tamird tamird changed the title [216] Add Support For Flow Dissector Programs Add Support For Flow Dissector Programs Mar 5, 2025
@tamird tamird requested a review from dave-tucker March 5, 2025 21:37
@tamird tamird marked this pull request as ready for review March 5, 2025 21:37
@tamird
Copy link
Member

tamird commented Mar 5, 2025

Rebased, marking ready for review. Hope @dave-tucker can take a look.

@tamird tamird force-pushed the 216-add-support-for-flow-dissector branch 3 times, most recently from 239e6e6 to b7489cc Compare March 5, 2025 22:00
@tamird tamird force-pushed the 216-add-support-for-flow-dissector branch from b7489cc to 8d1e906 Compare March 5, 2025 22:08
Copy link
Contributor

@chenhengqi chenhengqi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall.

Copy link
Member

@dave-tucker dave-tucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 14 files at r1.
Reviewable status: 0 of 18 files reviewed, 8 unresolved discussions (waiting on @alessandrod and @vadorovsky)


aya-bpf-macros/src/lib.rs line 612 at r1 (raw file):

/// Marks a function as an eBPF Flow Dissector program that can be attached to
/// a network namespace.
///

Some description here about expected return values would be helpful.
See: https://elixir.bootlin.com/linux/v6.13.5/source/include/uapi/linux/bpf.h#L6311


aya/src/programs/flow_dissector.rs line 11 at r1 (raw file):

};

/// A program that can be attached as a Flow Dissector routine

/// A program that can be attached as a Flow Dissector routine.

Missing .


aya/src/programs/flow_dissector.rs line 14 at r1 (raw file):

///
/// ['FlowDissector'] programs operate on an __sk_buff.
/// However, only the limited set of fields is allowed: data, data_end and flow_keys.

Please reconcile the description you've provided against the eBPF documentation: https://docs.ebpf.io/linux/program-type/BPF_PROG_TYPE_FLOW_DISSECTOR/
There are some inconsistencies. i.e Minimum Kernel Version being 4.20, not 4.2
The docs description describes the purpose of the program and what it should do (set flow_keys).


aya/src/programs/flow_dissector.rs line 67 at r1 (raw file):

        let netns_fd = netns.as_fd();

        let link_fd = bpf_link_create(

We should support attachment via BPF_PROG_ATTACH also for kernels < 5.7.
See cgroup_device.rs for an example of how to do this.


bpf/aya-bpf/src/programs/flow_dissector.rs line 25 at r1 (raw file):

    #[inline]
    pub fn flow_keys(&self) -> usize {

Take a look at the sample program here to see how this program type should operate: https://elixir.bootlin.com/linux/v6.3/source/tools/testing/selftests/bpf/progs/bpf_flow.c

The API here for flow_keys would not be pleasant to work with - why force the caller to cast a usize to *mut bpf_flow_keys, when we could have returned that in this function.

Similarly, I feel we're missing helpers here - for example, load_bytes.

Please attempt to write a test flow_dissector program (based on the kernel selftest) and submit it as an integration test.
This should help us figure out what the ebpf-side API should look like.


test/integration-ebpf/src/test.rs line 38 at r1 (raw file):

#[flow_dissector]
pub fn test_flow(_ctx: FlowDissectorContext) -> u32 {

This is fine to test that the program loads, but it doesn't verify correct operation.
It would be better if you were able to provide an integration test that shows correct operation of a Flow Dissector program.

Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see if @azenna wants to pick this up.

Reviewed 14 of 18 files at r2, all commit messages.
Reviewable status: 14 of 18 files reviewed, 8 unresolved discussions (waiting on @alessandrod, @azenna, @dave-tucker, and @vadorovsky)


aya/src/programs/flow_dissector.rs line 14 at r1 (raw file):

Previously, dave-tucker (Dave Tucker) wrote…

Please reconcile the description you've provided against the eBPF documentation: https://docs.ebpf.io/linux/program-type/BPF_PROG_TYPE_FLOW_DISSECTOR/
There are some inconsistencies. i.e Minimum Kernel Version being 4.20, not 4.2
The docs description describes the purpose of the program and what it should do (set flow_keys).

I did fix the version mismatch (probably while you were reviewing)

Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 18 files at r2.
Dismissed @alessandrod and @vadorovsky from 2 discussions.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @alessandrod, @azenna, @dave-tucker, and @vadorovsky)

@azenna azenna closed this by deleting the head repository Mar 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api/needs-review Makes an API change that needs review aya This is about aya (userspace) aya-bpf This is about aya-bpf (kernel) aya-obj Relating to the aya-obj crate test A PR that improves test cases or CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants