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

ebpf: Call bpf_probe_read on *const T BTF arguments #543

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vadorovsky
Copy link
Member

@vadorovsky vadorovsky commented Mar 14, 2023

It's necessary to call bpf_probe_read not only for pointers retrieved from PtRegs, but also from BTF arguments.

bpf_probe_read might return an error, so the return type of .arg() methods in contexts handling BTF arguments changes from Self to Option<Self>. None is returned when bpf_probe_read call is not successful.

Fixes: #542


This change is Reviewable

@netlify
Copy link

netlify bot commented Mar 14, 2023

Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 40b1c02
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/673f462f0e308e00089672fe
😎 Deploy Preview https://deploy-preview-543--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.

@tamird
Copy link
Member

tamird commented Aug 25, 2023

@vadorovsky mind rebasing?

@mergify mergify bot added aya-bpf This is about aya-bpf (kernel) test A PR that improves test cases or CI labels Sep 14, 2023
@mergify
Copy link

mergify bot commented Sep 14, 2023

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

@mergify mergify bot added the needs-rebase label Sep 14, 2023
Copy link

mergify bot commented Feb 6, 2024

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

@kamyuentse
Copy link

LGTM. I think this should be merged. @vadorovsky mind rebasing?

@mergify mergify bot removed the needs-rebase label Nov 21, 2024
@tamird tamird force-pushed the fentry-bpf-probe-read branch 2 times, most recently from 70f4e76 to 497d3e3 Compare November 21, 2024 11:51
@tamird
Copy link
Member

tamird commented Nov 21, 2024

I rebased this, but I think the tests need more work.

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 7 of 9 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @vadorovsky)


test/integration-test/src/tests/args.rs line 1 at r2 (raw file):

use aya::{

not clear to me what we're testing here


test/integration-ebpf/src/args.rs line 11 at r2 (raw file):

#[kprobe]
pub fn kprobe_vfs_write(ctx: ProbeContext) {
    let _: Option<usize> = ctx.arg(3);

what exactly are we testing here?

It's necessary to call `bpf_probe_read` not only for pointers retrieved
from `PtRegs`, but also from BTF arguments.

`bpf_probe_read` might return an error, so the return type of `.arg()`
methods in contexts handling BTF arguments changes from `T` to
`Option<T>`. `None` is returned when `bpf_probe_read` call is not
successful.

Fixes: aya-rs#542
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 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @vadorovsky)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aya-bpf This is about aya-bpf (kernel) test A PR that improves test cases or CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fentry/fexit: Do bpf_probe_read inside .arg()
3 participants