Skip to content

Conversation

@nathanjsweet
Copy link

Add a new "alloc" feature that indicates that the crate importing rbpf has an implementation of the GlobalAlloc trait. Memory protection is ignored in the "no_std" environment.

@qmonnet
Copy link
Owner

qmonnet commented Jul 2, 2025

Hi Nate, nice to hear from you 🙂 and thanks a lot for this work!

I haven't looked at the details yet, but have you looked at the work from #125? The JIT should be available in no_std now (I see the README.md is outdated in that regard). Is your PR adding something else, or are you trying to achieve the same?

@nathanjsweet
Copy link
Author

Yes I saw #125, but passing the library a pointer of bytes creates problems for my use case. Now I have to track those bytes when I don't want to. I would much rather implement GlobalAlloc. Core library crates that have strong use cases for running without "std" use the "alloc" feature coupled with a use of the core::alloc. Both the crossbeam-queue, and the futures-util implement no-std support thus. Definitely some documentation changes are in order regardless of the change. Thanks for taking the time 🙏 !

@Godones
Copy link
Contributor

Godones commented Jul 11, 2025

In this case, the rbpf library cannot ensure that the memory has executable permissions. Developers need to ensure that the memory allocated by GlobalAlloc has executable permissions, which seems unreasonable and dangerous.

Add a new "alloc" feature that indicates that the crate importing
rbpf has an implementation of the GlobalAlloc trait. Memory protection
is ignored in the "no_std" environment.

Signed-off-by: Nate Sweet <[email protected]>
@nathanjsweet nathanjsweet force-pushed the build-without-stdlib branch from 6508eb8 to 4b1ff20 Compare July 11, 2025 15:16
@nathanjsweet
Copy link
Author

I cleaned up the commit which had some whitespace issues.

@Godones

In this case, the rbpf library cannot ensure that the memory has executable permissions. Developers need to ensure that the memory allocated by GlobalAlloc has executable permissions, which seems unreasonable and dangerous.

Only if they turn on the alloc feature, which is almost always in a kernel or/and embedded environment (which can't offer executable protection). This commit is specifically for this scenario.

Copy link
Owner

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Sorry for the delay - took me some time to look into this. Thank you also Linfeng for the feedback.

I agree with Nate, it sounds acceptable to leave the user responsible for checking memory permissions, given that the alloc feature is opt-in.

Maybe we could add a comment about this in the feature description in the README, though?

Code looks good otherwise, with just some typos in the README update.

Could you please add a full test or example to show how to use the feature? (It doesn't have to be a complex one, but something providing a basic implementation for GlobalAlloc and running a JITed program on that would help users (and maintainers 😇) understand how to use it.

Comment on lines +595 to +596
The `rbpf` crate has a Cargoe featured named "alloc" that is not enabled by
default. `std` must be turned off and is mutually exclusive with alloc.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
The `rbpf` crate has a Cargoe featured named "alloc" that is not enabled by
default. `std` must be turned off and is mutually exclusive with alloc.
The `rbpf` crate has a Cargo feature named "alloc" that is not enabled by
default. `std` must be turned off and is mutually exclusive with `alloc`.

@qmonnet
Copy link
Owner

qmonnet commented Jul 31, 2025

We also need to adjust the CI runs, where we build with --all-features. Is there a way to make --all-features do all-features-but-actually-not-mutually-exclusive-ones in Rust/Cargo?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants