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

SIMD-0088: Enable Core BPF Programs #88

Merged

Conversation

buffalojoec
Copy link
Contributor

@buffalojoec buffalojoec commented Nov 18, 2023

This proposal defines the concept of "Core BPF Programs" - programs which are essential to cluster operations but exist as BPF programs, rather than native built-in programs.

The proposal describes, in a general sense, how native programs are migrated to Core BPF, and defines the requirements for proposing a native program migration to Core BPF.

@buffalojoec buffalojoec force-pushed the enable-core-bpf-programs branch from 9c4ff7c to e53c3cd Compare November 18, 2023 14:48
@buffalojoec buffalojoec changed the title XXXX: Enable Core BPF Programs 0088: Enable Core BPF Programs Nov 18, 2023
@buffalojoec buffalojoec force-pushed the enable-core-bpf-programs branch 2 times, most recently from 967a9b3 to 175b8f0 Compare November 21, 2023 10:26
@buffalojoec buffalojoec force-pushed the enable-core-bpf-programs branch from 175b8f0 to c8c77dc Compare November 21, 2023 10:33
@buffalojoec buffalojoec changed the title 0088: Enable Core BPF Programs SIMD-0088: Enable Core BPF Programs Nov 21, 2023
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks really good for the most part! Mostly some small bits to clean up, then we can start getting more eyes on it

proposals/0088-enable-core-bpf-programs.md Outdated Show resolved Hide resolved
proposals/0088-enable-core-bpf-programs.md Outdated Show resolved Hide resolved
proposals/0088-enable-core-bpf-programs.md Outdated Show resolved Hide resolved
proposals/0088-enable-core-bpf-programs.md Outdated Show resolved Hide resolved
proposals/0088-enable-core-bpf-programs.md Outdated Show resolved Hide resolved
proposals/0088-enable-core-bpf-programs.md Outdated Show resolved Hide resolved
proposals/0088-enable-core-bpf-programs.md Outdated Show resolved Hide resolved
proposals/0088-enable-core-bpf-programs.md Outdated Show resolved Hide resolved
proposals/0088-enable-core-bpf-programs.md Outdated Show resolved Hide resolved
proposals/0088-enable-core-bpf-programs.md Outdated Show resolved Hide resolved
@buffalojoec buffalojoec force-pushed the enable-core-bpf-programs branch from 549dd9d to 6017a1d Compare December 5, 2023 18:26
proposals/0088-enable-core-bpf-programs.md Outdated Show resolved Hide resolved
proposals/0088-enable-core-bpf-programs.md Outdated Show resolved Hide resolved
proposals/0088-enable-core-bpf-programs.md Outdated Show resolved Hide resolved
proposals/0088-enable-core-bpf-programs.md Outdated Show resolved Hide resolved
proposals/0088-enable-core-bpf-programs.md Outdated Show resolved Hide resolved
proposals/0088-enable-core-bpf-programs.md Outdated Show resolved Hide resolved
proposals/0088-enable-core-bpf-programs.md Outdated Show resolved Hide resolved
@buffalojoec
Copy link
Contributor Author

@joncinque @CriesofCarrots I think this SIMD is good to go.

@lheeger-jump @ripatel-fd can you guys have a look please?

Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Just a couple language suggestions. Content lgtm!

proposals/0088-enable-core-bpf-programs.md Outdated Show resolved Hide resolved
proposals/0088-enable-core-bpf-programs.md Outdated Show resolved Hide resolved
proposals/0088-enable-core-bpf-programs.md Outdated Show resolved Hide resolved

1. Verifiably build the source BPF program.
2. Generate a new keypair for the source program.
3. Deploy the program to the source program address.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this method of deployment would work. The regular BPF program loader might not be able to load a program using privileged instructions. (Which could be in the form of syscalls that are disabled for regular programs, thus fail loading, or dynamic imports for sBPFv2)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking it should work if you're just deploying and not invoking the program (right?).

In theory, if the core BPF program at say 11111111111111111111111111111111 has special privileges, then unless the executable lives at 11111111111111111111111111111111 it's not going to be able to use those privileges. So you deploy the BPF version of it to some arbitrary address, and if it was to be invoked at that arbitrary address it would fail to load. However, it shouldn't ever be invoked at that arbitrary address. It's only temporarily living there until the swap is complete.

Additionally, I think if any tweaks are necessary in the future as we figure out how to provide all of these core BPF programs with the necessary privileges, we can issue another proposal to modify the process or introduce a new one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking it should work if you're just deploying and not invoking the program (right?).

Currently, deployment includes the full ELF loading and eBPF verify pass. Deploying is actually even more restrictive than invoking. It's done that way to allow caching of the verification result, so it is unlikely to change soon.

In theory, if the core BPF program at say 11111111111111111111111111111111 has special privileges, then unless the executable lives at 11111111111111111111111111111111 it's not going to be able to use those privileges.

@Lichtso Has your team thought of ways to privilege core BPF programs? Sounds like @buffalojoec is proposing doing these checks at runtime rather than load time. If we decide to use syscalls, then these syscalls can be invoked by any program but would fail depending on the execution context. I don't think we can use cross program invocations since those are not aware of the caller identity, so I don't see any other mechanism than syscalls without some design changes.

Additionally, I think if any tweaks are necessary in the future as we figure out how to provide all of these core BPF programs with the necessary privileges, we can issue another proposal to modify the process or introduce a new one.

Sounds good to me, I sent my approval already. I just wanted to flag it for future discussion.

Copy link
Contributor

Choose a reason for hiding this comment

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

More discussion is required from VM stakeholders about privileged execution before I can consider approval. I do not want to add more syscalls if they all have to be re-invented in PRv2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More discussion is required from VM stakeholders about privileged execution before I can consider approval. I do not want to add more syscalls if they all have to be re-invented in PRv2.

Fair point. I guess the question is, does the number of syscalls block us from migration any native program to BPF?

In other words, is it possible we could move this SIMD along to acknowledge that at least some programs will be migrated, and then assess the impact of any necessary syscalls when the SIMDs outlining migration specifications are proposed for a program requiring privileges?

Copy link
Contributor

@ripatel-fd ripatel-fd Jan 7, 2024

Choose a reason for hiding this comment

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

More discussion is required from VM stakeholders about privileged execution before I can consider approval. I do not want to add more syscalls if they all have to be re-invented in PRv2.

@lheeger-jump PRv2 does not require re-inventing any syscalls. It suggests doing so. The benefit of doing that for privileged operations is debatable. This SIMD merely introduces the concept of Core BPF programs and sets the direction towards them long term without specifying a particular way to do privileged operations. We can do that in a follow-up SIMD.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are also a bunch of native programs whose instruction processor is entirely unprivileged. (The privileged parts happen in the slot boundary). Therefore, strongly suggest unblocking this so we can already start porting some programs while we're figuring out the privileges story.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Up-voting previous message by @ripatel-fd. If we can move this proposal along to introduce these programs and their migration path, then each program's migration SIMD - outlining the details of its migration to BPF - can tackle privileges.

If we learn that privileged programs will need a different migration path, we can publish another SIMD that adds the necessary changes to the migration process, and block any program migration SIMDs on that new proposal.

@ripatel-fd
Copy link
Contributor

@buffalojoec Apologies for reviewing this so late. I think this is a great idea.

ripatel-fd
ripatel-fd previously approved these changes Jan 3, 2024
@2501babe 2501babe self-requested a review January 4, 2024 06:33
@Lichtso
Copy link
Contributor

Lichtso commented Jan 7, 2024

Another slight change in behavior in which the on-chain version can not precisely match the original built-in is the compute meter and other resource limits. I think that should be mentioned in the document.

@buffalojoec
Copy link
Contributor Author

I also apologize for my very late review. We need more input from the VM team and the deployment procedure is unclear. Would this be an EOA that controls the account? I think changing the address would really hurt app devs and UX.

As mentioned in this comment, the address would not be changed. I've added some details on this into the SIMD.

@buffalojoec
Copy link
Contributor Author

Another slight change in behavior in which the on-chain version can not precisely match the original built-in is the compute meter and other resource limits. I think that should be mentioned in the document.

Added.

@jacobcreech
Copy link
Contributor

Looks like we've got general consensus. I'll give a last call for reviews and merge on Thursday, January 25th if there are no further objections from those with level 2 access.

Copy link
Contributor

@jacobcreech jacobcreech left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks Joe for moving this along

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Standard SIMD with type Core
Projects
Status: SIMDs
Development

Successfully merging this pull request may close these issues.

10 participants