Add utilities for allocating dynamic grants. #2053
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This adds two new utilities allocating arbitrary types as grants to kernel::grant::Allocator, and splits the internal `alloc_unowned` method into `alloc_raw` and `alloc_default_unowned` to allow allocating non-`Default` values. The new `alloc` allows allocating values not default-initializable, such as references to AppSlices. I believe this is a reasonable change, as no existing methods use `alloc`, and the new interface more closely matches other Rust allocation interfaces, such as `std::boxed::Box`. The new `alloc_n_with` then allows allocating a runtime-determined number of values. This will be useful if, for example, the size of allocation needed is determined by a variable given by a TockOS app. This is motivated by a use case in the Rubble BLE capsule, storing attributes. The number of attributes is usually known at compile-time for the app, but not for the kernel. If the capsule ends up using an array-like structure, `alloc_n_with` will be useful to allocate app-specific memory meant for dealing with attributes the app provides. If the capsule ends up using a linked-list-like structure, then the new `alloc` will allow allocating list nodes which contain non-Default data, such as `AppSlice`s. We don't necessarily need both interfaces, but I believe that together they form a sane base for everything one might need for dynamic memory allocation. Signed-off-by: David Ross <David.Ross@wdc.com>
2 tasks
|
I'm dropping a note that @daboross and I discussed this change one-on-one and I believe it is (a) necessary for safety and (b) correct. Because this is a security critical change, we should review this more carefully than normal and probably document it. Currently, I'm looking through this style (importantly, can we reduce the number of types which would help reason about security) as well as convince myself this is indeed correct this time. |
|
I in general like these changes, but I'm worried they undo #2003 which can cause stack overflows. |
alistair23
previously approved these changes
Aug 11, 2020
|
Also, otherwise this looks correct me now that I've looked through it more carefully |
This also renames the function, as I think the closure makes it more of a "_with" function than not. Signed-off-by: David Ross <David.Ross@wdc.com>
alistair23
approved these changes
Aug 17, 2020
|
Ping |
hudson-ayers
approved these changes
Sep 8, 2020
|
I added the last call label, hopefully someone can merge this soon. |
bradjc
approved these changes
Sep 17, 2020
|
bors r+ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Pull Request Overview
This pull request is intended to expand the basic functionality for dynamically allocating values. It:
allocmethod to accept non-Defaultvaluesalloc_n_with, for allocating a runtime-determined number of contiguous valuesalloc_unownedintoalloc_rawandalloc_default_unownedto allow for allocating non-DefaultvaluesI believe all of these changes will be useful, but the one which motivated this PR was
alloc_n_with. To support connections in the Rubble BLE capsule, we'll need to support attributes, and the capsule won't know ahead of time how many attributes any given app will want to have in a connection. Attributes are most naturally stored in an array, and since the kernel won't know at compile time how many attributes an app has, we need dynamic allocation. Hence,alloc_n_with.Note that this PR leaves the usage of
Owned, which I believe is unsound. Once we re-exportOwnedso that applications can store them, those applications could potentially store them outside of app-specific grant memory, and then access them after the app has been restarted (thus accessing deallocated memory). I have a fix for this, but since the changes are fairly separate, I've submitted it as a separate PR - #2052.If one PR is merged first, the other will need a rebase, but it shouldn't matter which one that is.
CC @alevy
Testing Strategy
I created a temporary test capsule and app which calls
allocandalloc_n_withevery other tick until we hitOutOfMemory, keeps each allocation one tick, and prints the data out after retrieving it.TODO or Help Wanted
While this was manually tested, automated testing would be better. Do we have a place for unit tests for kernel functionality exposed to capsules?
Documentation Updated
/docs, or no updates are required.Formatting
make prepush.