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

Add handle_clone to ABI and runtime. #1782

Merged
merged 1 commit into from
Jan 29, 2021

Conversation

wildarch
Copy link
Contributor

@wildarch wildarch commented Nov 30, 2020

Checklist

  • Pull request affects core Oak functionality (e.g. runtime, SDK, ABI)
    • I have written tests that cover the code changes.
    • I have checked that these tests are run by
      Cloudbuild
    • I have updated documentation accordingly.
    • I have raised an issue to
      cover any TODOs and/or unfinished work.
  • Pull request includes prototype/experimental work that is under
    construction.

This adds a new ABI call handle_clone and implements it in the Oak runtime. This function can be used to create distinct read handles. Following PRs will change the SDK implementations of handles to correctly implement Clone and Drop based on handle_clone and channel_close, so that the runtime can keep track of the number of live channel handles.

Background: #1686 (comment)

@google-cla google-cla bot added the cla: yes label Nov 30, 2020
@daviddrysdale
Copy link
Contributor

Drive-by musing: I think this can be done in an SDK helper without needing a new ABI call – a Node can use a separate channel and just send the handle to itself.

@tiziano88
Copy link
Collaborator

Drive-by musing: I think this can be done in an SDK helper without needing a new ABI call – a Node can use a separate channel and just send the handle to itself.

Except if we make sending linear (i.e. move them instead of copy), which I expect will be the next step?

@wildarch
Copy link
Contributor Author

Drive-by musing: I think this can be done in an SDK helper without needing a new ABI call – a Node can use a separate channel and just send the handle to itself.

Except if we make sending linear (i.e. move them instead of copy), which I expect will be the next step?

I think it would make sense to making sending linear indeed.

Even if we don't do that, an ABI call might still be more appropriate. For this to work through a channel, we would either need a global variable with a handle copier channel (and initialize that lazily somewhere), or we would need to create a new channel for a every clone(), which might be a bit expensive?

@daviddrysdale
Copy link
Contributor

I think it would make sense to making sending linear indeed.

Ah, I hadn't realized there was still an intent to do that – I thought #1197 concluded that it wasn't needed/helpful.

Even if we don't do that, an ABI call might still be more appropriate. For this to work through a channel, we would either need a global variable with a handle copier channel (and initialize that lazily somewhere), or we would need to create a new channel for a every clone(), which might be a bit expensive?

At the moment I'd personally lean towards preserving a minimal ABI – it's easier to come back and add in a abi::handle_clone() optimization later if it turns out to be a bottleneck than it is to simplify a redundant API.

@tiziano88
Copy link
Collaborator

I think the conclusion was that linearity may not be needed for security reasons, but I still think it makes sense for handles to be linear anyways, unless duplicated. It also hopefully makes handles work more like value semantics in Rust, so that they may be closed on Drop, and passed by value (moved) between functions (and over ABI calls).

@wildarch
Copy link
Contributor Author

wildarch commented Dec 3, 2020

I agree that value semantics for handles would be a good thing to have, especially in Rust I think developers will expect that.

The conclusion then I believe is that we do need this ABI call, so we can make handle passing linear in a future PR.

Marking ready for review.

@wildarch
Copy link
Contributor Author

Rebased this onto the latest main, @tiziano88 and @daviddrysdale friendly ping to review these changes 😄

Copy link
Contributor

@daviddrysdale daviddrysdale left a comment

Choose a reason for hiding this comment

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

There's a few missing bits for a new ABI entrypoint – please could you

  • add some tests to abitest
  • add the new entrypoint to docs/abi.md
  • add the new entrypoint to sdk/rust/oak/src/stubs.rs

(Might also be worth a quick grep across the codebase for one of the existing ABI functions in case there's other places that I haven't remembered).

@daviddrysdale
Copy link
Contributor

(Might also be worth a quick grep across the codebase for one of the existing ABI functions in case there's other places that I haven't remembered).

E.g. oak/module/oak_abi.h

@wildarch
Copy link
Contributor Author

There's a few missing bits for a new ABI entrypoint – please could you

  • add some tests to abitest
  • add the new entrypoint to docs/abi.md
  • add the new entrypoint to sdk/rust/oak/src/stubs.rs

(Might also be worth a quick grep across the codebase for one of the existing ABI functions in case there's other places that I haven't remembered).

Done! Thanks for pointing those out 😄

Copy link
Contributor

@daviddrysdale daviddrysdale left a comment

Choose a reason for hiding this comment

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

Nice, thanks.

examples/abitest/module_0/rust/src/lib.rs Outdated Show resolved Hide resolved
examples/abitest/module_0/rust/src/lib.rs Outdated Show resolved Hide resolved
examples/abitest/module_0/rust/src/lib.rs Outdated Show resolved Hide resolved
oak_runtime/src/proxy.rs Outdated Show resolved Hide resolved
oak_runtime/src/lib.rs Show resolved Hide resolved
@wildarch
Copy link
Contributor Author

There is no error log for the failed test, I'm going to assume that it is just a flake, the GCP tests are passing. Will go ahead and merge this now

@wildarch wildarch merged commit 1b6d086 into project-oak:main Jan 29, 2021
@github-actions
Copy link

Reproducibility Index:

6f651fd0da154010969d3dbac57a7880c2ed2a91d8246484c7247be7c4db4667  ./examples/abitest/bin/abitest_0_frontend.wasm
f7ac0a621db5c53b3f86f77238f4df1ed5c6af09a63bf674a746678f2cb0644c  ./examples/abitest/bin/abitest_1_backend.wasm
9c108f615413aa114e68438d0f902b502e6c1c5e7d4fe7bb8bc7b750d4def60e  ./examples/aggregator/bin/aggregator.wasm
3105aacfdb98ef516ec58497acfb72a3af591a67faffd6c2a2aa7df98409e68d  ./examples/chat/bin/chat.wasm
1a8257aed76024e8a5c1f400a9657402b46ace69fc26bb4849f87c38ea2b2e96  ./examples/hello_world/bin/hello_world.wasm
8c97d83fe6aa157dbbb01b0931e206bc4d9da058adfc2e0df46a7d1979c495c2  ./examples/hello_world/bin/translator.wasm
00dc476d438a40e46fd5827bfc9e553d1074d3224117f4cb47057f099cb1ca56  ./examples/http_server/bin/http_server.wasm
b43a49ae8e06e165e282ed877050f1ab61ac84ffed8d04011c5dfbd9ea79b5e6  ./examples/injection/bin/injection.wasm
3d525c7bf98f76a3ac545ecd14722d900a804c91c69f04afd33b2e41a7e8a32a  ./examples/private_set_intersection/bin/private_set_intersection.wasm
e7baa8bbf76625baa88769d8a31e250886ad5ab527a7af0f0474400b278e5530  ./examples/private_set_intersection/bin/private_set_intersection_handler.wasm
8c97d83fe6aa157dbbb01b0931e206bc4d9da058adfc2e0df46a7d1979c495c2  ./examples/translator/bin/translator.wasm
281511f222523d02d7a369d6b00c8f4e80d3ad6754af29a04e8b4864fbc1a2bc  ./examples/trusted_database/bin/trusted_database.wasm
26e0fd1be9f3160842dce5363a29641a9c0f2a3a46170b4f3552e6f8f285eedf  ./oak_loader/bin/oak_loader

Reproducibility Index diff:

diff --git a/reproducibility_index b/reproducibility_index
index 71f596a..54f9ae6 100644
--- a/reproducibility_index
+++ b/reproducibility_index
@@ -1,4 +1,4 @@
-4a07f1c5fdc26ea6dd5d8f31cc441a901412e2aa7f74038c8d5578acffaf57d4  ./examples/abitest/bin/abitest_0_frontend.wasm
+6f651fd0da154010969d3dbac57a7880c2ed2a91d8246484c7247be7c4db4667  ./examples/abitest/bin/abitest_0_frontend.wasm
 f7ac0a621db5c53b3f86f77238f4df1ed5c6af09a63bf674a746678f2cb0644c  ./examples/abitest/bin/abitest_1_backend.wasm
 9c108f615413aa114e68438d0f902b502e6c1c5e7d4fe7bb8bc7b750d4def60e  ./examples/aggregator/bin/aggregator.wasm
 3105aacfdb98ef516ec58497acfb72a3af591a67faffd6c2a2aa7df98409e68d  ./examples/chat/bin/chat.wasm
@@ -10,4 +10,4 @@ b43a49ae8e06e165e282ed877050f1ab61ac84ffed8d04011c5dfbd9ea79b5e6  ./examples/inj
 e7baa8bbf76625baa88769d8a31e250886ad5ab527a7af0f0474400b278e5530  ./examples/private_set_intersection/bin/private_set_intersection_handler.wasm
 8c97d83fe6aa157dbbb01b0931e206bc4d9da058adfc2e0df46a7d1979c495c2  ./examples/translator/bin/translator.wasm
 281511f222523d02d7a369d6b00c8f4e80d3ad6754af29a04e8b4864fbc1a2bc  ./examples/trusted_database/bin/trusted_database.wasm
-de87dea62d531f07c4ac03bcf8f4465c014bbb2aaa1f0a35fe78e430041918ab  ./oak_loader/bin/oak_loader
+26e0fd1be9f3160842dce5363a29641a9c0f2a3a46170b4f3552e6f8f285eedf  ./oak_loader/bin/oak_loader

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants