Tidy up aya-bpf-macros (again)

[dave-tucker]

This PR contains a number of commits that clean up aya-bpf-macros after their recent refactor.

The bulk of the PR is in aya-bpf-macros where I've added the ability to parse bool attributes.
This is neat as args are permitted in any order, and can be mixed with K/V style arguments.
For example, sleepable, function = "foo" and function = "foo", sleepable are both permitted.

Per https://kernel.org/doc/html/latest/bpf/libbpf/program_types.html I added support to all sleepable program types since this was already done at the macro-level for LSM. This required changes in aya-obj and aya.

The other major change is that pop_required_args is now gone, and all things that were "required" before are now Options. We may make these required one day, but not just yet.

---

This change is 

[netlify]

✅ Deploy Preview for aya-rs-docs ready!

Name	Link
🔨 Latest commit	71737f5
🔍 Latest deploy log	https://app.netlify.com/sites/aya-rs-docs/deploys/64c9961c2ef8910008cfb462
😎 Deploy Preview	https://deploy-preview-711--aya-rs-docs.netlify.app/src/aya/bpf.rs
📱 Preview on mobile	Toggle 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]

Reviewable status: 0 of 13 files reviewed, 3 unresolved discussions (waiting on @dave-tucker)

---

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

            let lookahead = input.lookahead1();
            if lookahead.peek(Token![=]) {
                let _ = input.parse::<Eq>()?;

a little odd that we're looking ahead, and then fallibly parsing the token. can we just skip the token, or perhaps take the else arm if parse::<Eq>() fails?

finally, should we be parsing to the end? it looks to me like this will now accept any jibberish that isn't an equal sign.

---

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

pub(crate) fn pop_string_arg(args: &mut Args, name: &str) -> Option<String> {
    let value = match args.args.iter().position(|arg| match arg {

this function should be

let index = args.args.iter().position(|arg| matches!(arg, ....))?;
match args.args.remove(index) {
  Arg::String => ...,
  arg => panic!("impossible variant {arg:?})
}

currently it seems to allow impossible things to happen.

---

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

pub(crate) fn pop_bool_arg(args: &mut Args, name: &str) -> bool {
    let value = match args.args.iter().position(|arg| match arg {

this looks like Option::map

more below

[dave-tucker]

Reviewable status: 0 of 13 files reviewed, 3 unresolved discussions (waiting on @dave-tucker)

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

            let lookahead = input.lookahead1();
            if lookahead.peek(Token![=]) {
                let _ = input.parse::<Eq>()?;

a little odd that we're looking ahead, and then fallibly parsing the token. can we just skip the token, or perhaps take the else arm if parse::<Eq>() fails?

finally, should we be parsing to the end? it looks to me like this will now accept any jibberish that isn't an equal sign.

This is now more robust. The lookahead is still there - we need to know after we've parse the first ident whether to continue parsing or not. However this time we handle cases where the next token is a comma, or there is no input and return early... otherwise we continue parsing.

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

pub(crate) fn pop_string_arg(args: &mut Args, name: &str) -> Option<String> {
    let value = match args.args.iter().position(|arg| match arg {

this function should be

let index = args.args.iter().position(|arg| matches!(arg, ....))?;
match args.args.remove(index) {
  Arg::String => ...,
  arg => panic!("impossible variant {arg:?})
}

currently it seems to allow impossible things to happen.

Done.

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

pub(crate) fn pop_bool_arg(args: &mut Args, name: &str) -> bool {
    let value = match args.args.iter().position(|arg| match arg {

this looks like Option::map

more below

And also done.

[tamird]

Reviewed 10 of 11 files at r1, 3 of 3 files at r4, 2 of 2 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @dave-tucker)

---

-- commits line 7 at r4:
can you include an integration test in this commit? does it make sense to do so?

---

aya/src/bpf.rs line 565 at r6 (raw file):

                            kind: ProbeKind::KRetProbe,
                        }),
                        ProgramSection::UProbe { sleepable, .. } => {

can you make these destructurings exhaustive? i think it's just one more field, and that would let the compiler helps us in the future.

---

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

Previously, dave-tucker (Dave Tucker) wrote…

This is now more robust. The lookahead is still there - we need to know after we've parse the first ident whether to continue parsing or not. However this time we handle cases where the next token is a comma, or there is no input and return early... otherwise we continue parsing.

OK. Would you mind writing this line as let _: Eq = input.parse()?? it's a little more obvious that way what it is that's being discarded.

---

aya-bpf-macros/src/args.rs line 80 at r4 (raw file):

        })
        .ok_or_else(|| {
            let tokens = match args.args.first().unwrap() {

isn't it possible for args to be empty?

---

aya-bpf-macros/src/fentry.rs line 75 at r4 (raw file):

            #[link_section = "fentry/sys_clone"]
            fn sys_clone(ctx: *mut ::core::ffi::c_void) -> i32 {
                let _ = sys_clone(::aya_bpf::programs::FEntryContext::new(ctx));

recognizing this pattern is already here, why not return this value rather than discard it?

            fn sys_clone(ctx: *mut ::core::ffi::c_void) -> i32 {
                fn sys_clone(ctx: &mut aya_bpf::programs::FEntryContext) -> i32 {
                    0
                }
                sys_clone(::aya_bpf::programs::FEntryContext::new(ctx))
            }

---

aya-bpf-macros/src/fexit.rs line 75 at r4 (raw file):

            #[link_section = "fexit/sys_clone"]
            fn sys_clone(ctx: *mut ::core::ffi::c_void) -> i32 {
                let _ = sys_clone(::aya_bpf::programs::FExitContext::new(ctx));

same here

---

aya-bpf-macros/src/kprobe.rs line 39 at r4 (raw file):

        let mut offset = None;

        if !attrs.is_empty() {

why is this check needed?

---

aya-bpf-macros/src/raw_tracepoint.rs line 17 at r4 (raw file):

impl RawTracePoint {
    pub(crate) fn parse(attrs: TokenStream, item: TokenStream) -> Result<RawTracePoint> {
        if attrs.is_empty() {

why not use pop_string_arg below and bail if it's absent? this check seems redundant.

---

aya-bpf-macros/src/uprobe.rs line 42 at r4 (raw file):

        let mut offset = None;
        let mut sleepable = false;
        if !attrs.is_empty() {

why is this check required?

---

aya-bpf-macros/src/uprobe.rs line 135 at r4 (raw file):

                #[link_section = "uprobe"]
                fn foo(ctx: *mut ::core::ffi::c_void) -> u32 {
                    let _ = foo(::aya_bpf::programs::ProbeContext::new(ctx));

ditto

[dave-tucker]

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @tamird)

---

-- commits line 7 at r4:

Previously, tamird (Tamir Duberstein) wrote…

can you include an integration test in this commit? does it make sense to do so?

I could, but I'd rather open an issue and do it later since these macro fixups are stacking up :)

---

aya/src/bpf.rs line 565 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

can you make these destructurings exhaustive? i think it's just one more field, and that would let the compiler helps us in the future.

Can I do it in a follow up? I'm about to do more tidy up on ProgramSection after these macro fixes.

---

aya-bpf-macros/src/args.rs line 80 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

isn't it possible for args to be empty?

Yes this is 100% the wrong thing to do, but I'm also ripping this out in #713.
I've closed #713 and brought that commit in here.

---

aya-bpf-macros/src/fentry.rs line 75 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

recognizing this pattern is already here, why not return this value rather than discard it?

            fn sys_clone(ctx: *mut ::core::ffi::c_void) -> i32 {
                fn sys_clone(ctx: &mut aya_bpf::programs::FEntryContext) -> i32 {
                    0
                }
                sys_clone(::aya_bpf::programs::FEntryContext::new(ctx))
            }

These tests only test that the macro contents is rendered correctly. BPF program return values is a can of worms we shouldn't open in this PR :)

---

aya-bpf-macros/src/kprobe.rs line 39 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why is this check needed?

There were a few of these around... they were to guard us from attempting to parse tokens if there were none to parse. Most of them can be removed. I've pushed a commit to do this here.

---

aya-bpf-macros/src/raw_tracepoint.rs line 17 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why not use pop_string_arg below and bail if it's absent? this check seems redundant.

yep, that was also something in #713, i've now added that commit here too. tracepoint is now an option so pop_string_arg doesn't need to bail.

[tamird]

Reviewed 4 of 4 files at r7, 2 of 2 files at r8, 1 of 1 files at r9, 3 of 3 files at r10, 1 of 1 files at r11, 6 of 6 files at r12, 7 of 7 files at r13, all commit messages.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @dave-tucker)

---

-- commits line 7 at r4:

Previously, dave-tucker (Dave Tucker) wrote…

I could, but I'd rather open an issue and do it later since these macro fixups are stacking up :)

OK.

---

-- commits line 36 at r12:
can we avoid this diary-entry style of commit message? reading this, i have no idea what this change does. Please use the imperative voice to describe what this commit is doing.

---

aya/src/bpf.rs line 565 at r6 (raw file):

Previously, dave-tucker (Dave Tucker) wrote…

Can I do it in a follow up? I'm about to do more tidy up on ProgramSection after these macro fixes.

Could you do it here just for the lines you're already touching?

---

aya-bpf-macros/src/args.rs line 80 at r4 (raw file):

Previously, dave-tucker (Dave Tucker) wrote…

Yes this is 100% the wrong thing to do, but I'm also ripping this out in #713.
I've closed #713 and brought that commit in here.

I don't love that a single logical change is spread over multiple commits.

---

aya-bpf-macros/src/btf_tracepoint.rs line 16 at r12 (raw file):

impl BtfTracePoint {
    pub(crate) fn parse(attrs: TokenStream, item: TokenStream) -> Result<Self> {
        let item = syn::parse2(item)?;

is this reversal meaningful?

---

aya-bpf-macros/src/btf_tracepoint.rs line 25 at r12 (raw file):

    pub(crate) fn expand(&self) -> Result<TokenStream> {
        let section_name: Cow<'_, _> = if self.function.is_none() {

is_none() + unwrap() is just if let Some

---

aya-bpf-macros/src/btf_tracepoint.rs line 67 at r12 (raw file):

            #[link_section = "tp_btf"]
            fn foo(ctx: *mut ::core::ffi::c_void) -> i32 {
                let _ = foo(::aya_bpf::programs::BtfTracePointContext::new(ctx));

please just let the value flow through, it's zero no matter what.

---

aya-bpf-macros/src/cgroup_skb.rs line 21 at r10 (raw file):

            match ident.to_string().as_str() {
                "ingress" | "egress" => (),
                _ => abort!(ident, "invalid attach type"),

the commit message on this change is aya-bpf-macros: Remove useless attrs.is_empty. Help me understand?

---

aya-bpf-macros/src/cgroup_sock.rs line 15 at r13 (raw file):

impl CgroupSock {
    pub(crate) fn parse(attrs: TokenStream, item: TokenStream) -> Result<CgroupSock> {
        if attrs.is_empty() {

why is this check needed? where's the test?

same for cgroup_sock_addr.rs and cgroup_sockopt.rs

---

aya-bpf-macros/src/fentry.rs line 75 at r4 (raw file):

Previously, dave-tucker (Dave Tucker) wrote…

These tests only test that the macro contents is rendered correctly. BPF program return values is a can of worms we shouldn't open in this PR :)

This returns zero no matter what, why do we need to draw the reader's eye to this let _?

---

aya-bpf-macros/src/fentry.rs line 31 at r12 (raw file):

    pub(crate) fn expand(&self) -> Result<TokenStream> {
        let section_prefix = if self.sleepable { "fentry.s" } else { "fentry" };
        let section_name: Cow<'_, _> = if self.function.is_none() {

this is if let Some

---

aya-bpf-macros/src/fentry.rs line 73 at r12 (raw file):

            #[link_section = "fentry"]
            fn sys_clone(ctx: *mut ::core::ffi::c_void) -> i32 {
                let _ = sys_clone(::aya_bpf::programs::FEntryContext::new(ctx));

please let the value flow through.

---

aya-bpf-macros/src/fexit.rs line 31 at r12 (raw file):

    pub(crate) fn expand(&self) -> Result<TokenStream> {
        let section_prefix = if self.sleepable { "fexit.s" } else { "fexit" };
        let section_name: Cow<'_, _> = if self.function.is_none() {

this is if let Some

---

aya-bpf-macros/src/fexit.rs line 73 at r12 (raw file):

            #[link_section = "fexit"]
            fn sys_clone(ctx: *mut ::core::ffi::c_void) -> i32 {
                let _ = sys_clone(::aya_bpf::programs::FExitContext::new(ctx));

ditto

---

aya-bpf-macros/src/lsm.rs line 31 at r12 (raw file):

    pub(crate) fn expand(&self) -> Result<TokenStream> {
        let section_prefix = if self.sleepable { "lsm.s" } else { "lsm" };
        let section_name: Cow<'_, _> = if self.hook.is_none() {

this is if let Some

---

aya-bpf-macros/src/raw_tracepoint.rs line 17 at r4 (raw file):

Previously, dave-tucker (Dave Tucker) wrote…

yep, that was also something in #713, i've now added that commit here too. tracepoint is now an option so pop_string_arg doesn't need to bail.

OK.

---

aya-bpf-macros/src/raw_tracepoint.rs line 17 at r12 (raw file):

impl RawTracePoint {
    pub(crate) fn parse(attrs: TokenStream, item: TokenStream) -> Result<RawTracePoint> {
        let item = syn::parse2(item)?;

is this reversal meaningful?

---

aya-bpf-macros/src/raw_tracepoint.rs line 25 at r12 (raw file):

    pub(crate) fn expand(&self) -> Result<TokenStream> {
        let section_name: Cow<'_, _> = if self.tracepoint.is_none() {

this is if let Some

[dave-tucker]

Reviewable status: 1 of 19 files reviewed, 17 unresolved discussions (waiting on @tamird)

---

-- commits line 36 at r12:

Previously, tamird (Tamir Duberstein) wrote…

can we avoid this diary-entry style of commit message? reading this, i have no idea what this change does. Please use the imperative voice to describe what this commit is doing.

Yep I'll squash the aya-bpf-macros changes together

---

aya/src/bpf.rs line 565 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Could you do it here just for the lines you're already touching?

Sure why not

---

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

Previously, tamird (Tamir Duberstein) wrote…

OK. Would you mind writing this line as let _: Eq = input.parse()?? it's a little more obvious that way what it is that's being discarded.

Done.

---

aya-bpf-macros/src/args.rs line 80 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

I don't love that a single logical change is spread over multiple commits.

Ack, will squash these together.

---

aya-bpf-macros/src/btf_tracepoint.rs line 16 at r12 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

is this reversal meaningful?

No, this will be squashed.

---

aya-bpf-macros/src/btf_tracepoint.rs line 25 at r12 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

is_none() + unwrap() is just if let Some

Ack.

---

aya-bpf-macros/src/btf_tracepoint.rs line 67 at r12 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

please just let the value flow through, it's zero no matter what.

again, this requires changing expand() - i'd rather not do that in this PR

---

aya-bpf-macros/src/cgroup_skb.rs line 21 at r10 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

the commit message on this change is aya-bpf-macros: Remove useless attrs.is_empty. Help me understand?

Ooops yes this snuck in because I accidentally removed the attrs.is_empty() guard here.
And then realized that the error manifested in the wrong place.

Compiletests will make this clear, when we finally add some.

This will be squashed anyway and I'll add some context in the commit message.

---

aya-bpf-macros/src/cgroup_sock.rs line 15 at r13 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why is this check needed? where's the test?

same for cgroup_sock_addr.rs and cgroup_sockopt.rs

It's needed to give a nice error message to the caller if they were to use #[cgroup_sock] - note, there's not attach type there. Failing to do that gives a generic message about not being able to parse ident, which isn't as nice.

There are no tests, because negative testing of proc_macros has to be done with compiletests and I haven't been motivated enough to set that up just yet.

Otherwise you get a "fun" error message like this:

thread 'cgroup_sock::tests::cgroup_sock_no_attach' panicked at 'proc-macro-error API cannot be used outside of `entry_point` invocation, perhaps you forgot to annotate your #[proc_macro] function with `#[proc_macro_error]', /home/dave/.cargo/registry/src/index.crates.io-6f17d22bba15001f/proc-macro-error-1.0.4/src/lib.rs:483:9

---

aya-bpf-macros/src/fentry.rs line 75 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

This returns zero no matter what, why do we need to draw the reader's eye to this let _?

You're argument is with the implementation of expand() for this macro.
I'm not saying it's not worth fixing, but I'm not doing it in this PR.

---

aya-bpf-macros/src/fentry.rs line 31 at r12 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this is if let Some

Ack

---

aya-bpf-macros/src/fentry.rs line 73 at r12 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

please let the value flow through.

Sorry what value?

---

aya-bpf-macros/src/fexit.rs line 75 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

same here

ditto - re: expand() changes

---

aya-bpf-macros/src/fexit.rs line 31 at r12 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this is if let Some

Done.

---

aya-bpf-macros/src/fexit.rs line 73 at r12 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

ditto

ditto - re: expand() changes

---

aya-bpf-macros/src/lsm.rs line 31 at r12 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this is if let Some

Done.

---

aya-bpf-macros/src/raw_tracepoint.rs line 17 at r12 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

is this reversal meaningful?

Fixed in squash

---

aya-bpf-macros/src/raw_tracepoint.rs line 25 at r12 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this is if let Some

Done.

---

aya-bpf-macros/src/uprobe.rs line 42 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why is this check required?

removed

---

aya-bpf-macros/src/uprobe.rs line 135 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

ditto

ditto: re: expand() changes

[tamird]

Reviewed 18 of 18 files at r14, 2 of 2 files at r15, 1 of 1 files at r16, 3 of 3 files at r17, 1 of 1 files at r18, 6 of 6 files at r19, 7 of 7 files at r20, 8 of 8 files at r21, 2 of 2 files at r22, 1 of 1 files at r23, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @dave-tucker)

---

aya-bpf-macros/src/fentry.rs line 75 at r4 (raw file):

Previously, dave-tucker (Dave Tucker) wrote…

You're argument is with the implementation of expand() for this macro.
I'm not saying it's not worth fixing, but I'm not doing it in this PR.

That seems fine, but I'm confused because lsm.rs seems to do the right thing. Am I missing something?

---

aya-bpf-macros/src/fentry.rs line 73 at r12 (raw file):

Previously, dave-tucker (Dave Tucker) wrote…

Sorry what value?

(this comment was let _ again), please ignore it for now

---

aya-bpf-macros/src/lsm.rs line 78 at r21 (raw file):

            #[link_section = "lsm.s/bprm_committed_creds"]
            fn bprm_committed_creds(ctx: *mut ::core::ffi::c_void) -> i32 {
                return bprm_committed_creds(::aya_bpf::programs::LsmContext::new(ctx));

is the expand impl here different than the others? here you return the value, while in the others you write let _ = thing-that-returns 0; return 0

[dave-tucker]

Dismissed @tamird from a discussion.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @tamird)

---

aya-bpf-macros/src/fentry.rs line 75 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

That seems fine, but I'm confused because lsm.rs seems to do the right thing. Am I missing something?

Yes, here's the body of the fentry macro - we're asserting that given a set of inputs, the output is correct:
https://github.com/aya-rs/aya/blob/main/aya-bpf-macros/src/fentry.rs#L38-L47

LSM does the right thing because the BPF program may return -EPERM to deny an action.
We're deliberately ignoring the return code here. I would guess that non-zero return codes trip the verifier, but I can't be certain.

Either way, it's probably better for fentry/fexit functions to have no return value vs. ignoring the value. That can be fixed in another PR though.

---

aya-bpf-macros/src/lsm.rs line 78 at r21 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

is the expand impl here different than the others? here you return the value, while in the others you write let _ = thing-that-returns 0; return 0

Yes: https://github.com/aya-rs/aya/blob/main/aya-bpf-macros/src/lsm.rs#L41-L48

[tamird]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dave-tucker)

---

aya-bpf-macros/src/fentry.rs line 75 at r4 (raw file):

Previously, dave-tucker (Dave Tucker) wrote…

Yes, here's the body of the fentry macro - we're asserting that given a set of inputs, the output is correct:
https://github.com/aya-rs/aya/blob/main/aya-bpf-macros/src/fentry.rs#L38-L47

LSM does the right thing because the BPF program may return -EPERM to deny an action.
We're deliberately ignoring the return code here. I would guess that non-zero return codes trip the verifier, but I can't be certain.

Either way, it's probably better for fentry/fexit functions to have no return value vs. ignoring the value. That can be fixed in another PR though.

OK, thanks for explaining.