Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
SIMD-0088: Enable Core BPF Programs #88
Changes from 7 commits
c8c77dc
6017a1d
02074b8
8191e4b
8533b54
be47e90
2c13cd0
fadff46
385401e
aa23787
3824771
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be standardized
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could standardize here by saying they will be upgraded via the BPF upgrade mechanism, but whether or not each upgrade needs a feature gate may not fit the standard for all programs. Same thing with who holds the multi-sig authority, if any.
As discussed in #99, engineers could choose to use feature gates for new features on a program, but it may not be necessary for all changes or all programs.
We could mention a standard for rollbacks, if it's appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, we may choose to make some programs upgradeable - like Feature Gate - but others could be non-upgradeable - like System - therefore they'd have separate loaders and different ways of introducing changes, if need be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lheeger-jump Curious what you think here. I'm leaning towards less standardization for the reasons listed above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @buffalojoec
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 at11111111111111111111111111111111
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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.
Sounds good to me, I sent my approval already. I just wanted to flag it for future discussion.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.