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

aya: skip map creation and relocation for maps that should be ignored #968

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

martinsoees
Copy link

@martinsoees martinsoees commented Jun 13, 2024

Allows programs containing unsupported maps for the current
kernel to be loaded by skipping map creation and relocation before loading.

This is useful when you have a single program containing e.g. a RingBuf
and a PerfEventArray and you decide from user space which one to use
before loading the bytecode by checking if the kernel you are running
on has support for e.g. RingBuf.
That way you can use the same eBPF bytecode across multiple kernels
regardless of the kernel has support for it or not.
Can be used with .set_global() to signal if the map is supported
until support for bpf_core_enum_value_exists.

Example user space code:

let ringbuf_supported = ...

let mut set = HashSet::new();
set.insert(bpf_map_type::BPF_MAP_TYPE_RINGBUF);

let mut ebpf = EbpfLoader::new()
    .deactivate_maps(set)
    .set_global("RINGBUF_SUPPORTED", &ringbuf_supported, true)
    .load(include_bytes_aligned!(
    "../../target/bpfel-unknown-none/debug/disable_maps"
))?;

// ...
if ringbuf_supported {
    ringbuf_events()?.await;
} else {
    perfbuf_events()?.await;
}
// ...

Example eBPF bytecode:

#[no_mangle]
static RINGBUF_SUPPORTED: i32 = 0;

#[map]
static mut RINGBUF: RingBuf = RingBuf::with_byte_size(1024 * 10, 0);

#[map]
static mut PERFBUF: PerfEventArray<Buffer> = PerfEventArray::with_max_entries(1, 0);

#[xdp]
pub fn my_func(ctx: XdpContext) -> u32 {
    match try_my_func(ctx) {
        Ok(ret) => ret,
        Err(_) => xdp_action::XDP_ABORTED,
    }
}

fn try_my_func(ctx: XdpContext) -> Result<u32, u32> {
    info!(&ctx, "received a packet");
    let ringbuf = unsafe { core::ptr::read_volatile(&RINGBUF_SUPPORTED) };
    if ringbuf == 1 {
        info!(&ctx, "Ringbuf supported");
        // do ringbuf output
    } else {
        info!(&ctx, "Defaulting to perfevent");
        // do perfbuf output
    }
    Ok(xdp_action::XDP_PASS)
}

This change is Reviewable

This makes it possible to have bytecode with unsupported map types such as BPF_MAP_TYPE_RINGBUF on a target kernel that doesn't have that map type
Copy link

netlify bot commented Jun 13, 2024

Deploy Preview for aya-rs-docs failed.

Built without sensitive environment variables

Name Link
🔨 Latest commit 0b0d4c7
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/6690daa22080720008772feb

@mergify mergify bot added aya This is about aya (userspace) aya-obj Relating to the aya-obj crate labels Jun 13, 2024
@martinsoees
Copy link
Author

Perhaps this should be scoped to only kernel-userspace data streaming (RingBufand PerfEvent).
Then the API could define a primary and a fallback data map, where RingBuf is primary and PerfEvent is a fallback map.
Would obviously require that the dev correctly handles this and reads from the correct map

@alessandrod
Copy link
Collaborator

 fn try_my_func(ctx: XdpContext) -> Result<u32, u32> {
    info!(&ctx, "received a packet");
    let ringbuf = unsafe { core::ptr::read_volatile(&RINGBUF_SUPPORTED) };
    if ringbuf == 1 {
        info!(&ctx, "Ringbuf supported");
        // do ringbuf output
    } else {
        info!(&ctx, "Defaulting to perfevent");
        // do perfbuf output
    }
    Ok(xdp_action::XDP_PASS)
}

Does this actually work, if in the ringbuf==1 case you access the RINGBUF map, and then in the else you access the PERFBUF map? Because if you skip relocating it, I would have expected the verifier to complain that the map fd is not valid.

If this does work, can you add an integration test please?

@alessandrod
Copy link
Collaborator

Perhaps this should be scoped to only kernel-userspace data streaming (RingBufand PerfEvent). Then the API could define a primary and a fallback data map, where RingBuf is primary and PerfEvent is a fallback map. Would obviously require that the dev correctly handles this and reads from the correct map

I tend to think the opposite: the current mechanism is too restrictive and it should be more generic. Right now you can ignore loading a map type, but what if I want to ignore loading only an instance of a map type (because eg, a bit of functionality is disabled, and I want to save the memory?).

I'm thinking it's probably better to have something like EbpfLoader::ignore_maps(&["RINGBUF"]) - do the ignore by name that is. Then an user can ignore for whatever reason, not only based on map type.

@martinsoees
Copy link
Author

Perhaps this should be scoped to only kernel-userspace data streaming (RingBufand PerfEvent). Then the API could define a primary and a fallback data map, where RingBuf is primary and PerfEvent is a fallback map. Would obviously require that the dev correctly handles this and reads from the correct map

I tend to think the opposite: the current mechanism is too restrictive and it should be more generic. Right now you can ignore loading a map type, but what if I want to ignore loading only an instance of a map type (because eg, a bit of functionality is disabled, and I want to save the memory?).

I'm thinking it's probably better to have something like EbpfLoader::ignore_maps(&["RINGBUF"]) - do the ignore by name that is. Then an user can ignore for whatever reason, not only based on map type.

It actually started out like that, with the name rather than the type. I can revert it back to that or add a new function that filters by name and keep the one by id also? I see no reason why both can't exist

…hat does the same by name. Fixed renaming and added the new 3rd argument to relocate_maps() for rbpf test case
@mergify mergify bot added the test A PR that improves test cases or CI label Jun 18, 2024
@martinsoees
Copy link
Author

This PR has a clippy allow for too_many_arguments on relocate_maps. Ideally this should be fixed, but would require a larger rewrite of the map relocation section, so I suggest that it should be a separate PR. Unless you want it in this PR?

@martinsoees martinsoees changed the title Draft: aya: skip map creation and relocation before loading for maps that should be deactivated aya: skip map creation and relocation before loading for maps that should be deactivated Jun 20, 2024
@martinsoees martinsoees changed the title aya: skip map creation and relocation before loading for maps that should be deactivated aya: skip map creation and relocation for maps that should be ignored Jun 20, 2024
martinsoees added a commit to martinsoees/aya that referenced this pull request Jun 20, 2024
aya/src/bpf.rs Outdated Show resolved Hide resolved
aya/src/bpf.rs Show resolved Hide resolved
aya/src/bpf.rs Outdated Show resolved Hide resolved
aya/src/bpf.rs Show resolved Hide resolved
aya/src/bpf.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@alessandrod alessandrod left a comment

Choose a reason for hiding this comment

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

Very nice work! See comments

@martinsoees
Copy link
Author

Very nice work! See comments

Thanks for the feedback. I left a comment to one of your review comments. The rest is straight forward. I'll see when I have time to fix it

Copy link

mergify bot commented Jul 3, 2024

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

@mergify mergify bot added the needs-rebase label Jul 3, 2024
- Removed 'ignore_map_by_type' since 'ignore_map_by_name' makes more sense
@mergify mergify bot removed the needs-rebase label Jul 12, 2024
The 'ignore_map_by_type' function has been removed. Also renamed test function and simplified the relocation builder since 'by_name' is the only option now
@@ -232,6 +250,8 @@ fn relocate_maps<'a, I: Iterator<Item = &'a Relocation>>(
);
debug_assert_eq!(map.symbol_index().unwrap(), rel.symbol_index);
m
} else if ignored_by_section.contains(&section_index) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is correct, it's just not getting exercised by the test because we do have symbols. We switched to putting all maps in the same section (like libbpf does), so I think that this would skip all the maps?

Copy link

mergify bot commented Nov 24, 2024

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

@mergify mergify bot added the needs-rebase label Nov 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aya This is about aya (userspace) aya-obj Relating to the aya-obj crate needs-rebase test A PR that improves test cases or CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants