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

Example: crypto inline method addition to crypto_accelerator object #59

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

loalan
Copy link

@loalan loalan commented Oct 3, 2022

This example expands on the definition of crypto_accelerator (#53).

The example adds two methods for encrypt/decrypt that assumes that inline accelerators operate immediately on the packet (e.g. deparse, decrypt and reparse). Packet recirculation is not necessary for either inline method.

The example shows the use of inline encrypt and decrypt, as well as how the crypto accelerator results can be used.

@pbhide
Copy link
Contributor

pbhide commented Oct 4, 2022

We should separate out hidden/implicit side effects such as 'reparse' and associated state modification from crypto.encrypt/decrypt methods and generalize it so that it can be used for other accelerators as well.
Consider the following -

action ipsec_esp_decrypt(...) {
    ....
   <existing methods to setup the decryption parameters>
    ....
    ipsec_acc.decrypt(enable_auth);

    if (reparse_packet() == True) {
        results = ipsec_acc.get_results()
        .... <check results, post_decrypt_actions >....
    } else {
        recirc_packet();
        exit();
    }
}

I see the following advantages of this -

  1. We can define/debate reparse_packet() extern independent of accelerator methods.
  2. This will be useful for all accelerators (present and future).
  3. Programmer will be able launch multiple accelerators and reparse the packet once. Mixing this with accelerator method will prevent that.

Overall I think it will keep accelerator functions clean and code slightly more portable across targets.

examples/crypto-inline.p4 Outdated Show resolved Hide resolved
examples/crypto-inline.p4 Outdated Show resolved Hide resolved
}

enum bit<2> crypto_mode_e {
TUNNEL = 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the hardware accelerator should be wire protocol agnostic. We can specify offset/len of the cipher-text in the packet instead of defining these modes. That will allow programmer to use the engine for other protocols.. such as DTLS or any other custom protocol that uses AES-GCM crypto.

Copy link
Author

Choose a reason for hiding this comment

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

I suppose the difference here is in the async model encrypt/decrypt is in the HW block, and encap/decap is in P4.
In the inline model both encap/encrypt and decrypt/decap are in the HW block. This is because both deparsing and encryption are triggered inline, prior to the P4 deparse stage. Similar for decrypt and removal of the IPSec headers, this executes inline prior to P4 deparser. (Subsequently there is no need to reinject in order check results and to run it through the deparser).

This is also somewhat different from existing P4 implementations, but seem to be what is needed to make this packet transformation inline.

examples/crypto-inline.p4 Outdated Show resolved Hide resolved
@roop-nvda
Copy link
Contributor

roop-nvda commented Oct 7, 2022

IMHO: This is indeed good example for what we may expect to see with inline accelerators; but there are other (than IPSec) inline accelerations that will need to be expressed as well. The value of inline acceleration in this domain is clear. Also, most not-trivial accelerators will likely change a multitude of things. My hope is that the solution we land of is general enough to be reused in other inline accelerators that will be needed in the near future by all vendors.

key = {
// For encrypt case get sa_idx from parser
// for decrypt case esp hdr spi value will be used as sa_idx
main_meta.sa_index : exact;
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to have separate match-action tables for encryption and decryption?

For example, encrypt table could match on the destination IP address, while decrypt table could match on source and destination IP address, as well as SPI value. (This is similar to the approach proposed in P4-IPsec)

Copy link
Author

Choose a reason for hiding this comment

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

Sure, that's possible. This is just an example, to focus on how the inline model works

Copy link
Author

Choose a reason for hiding this comment

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

Actually, what you proposed is simpler than setting parser metadata. I'll look at rewriting the example app.

@loalan loalan force-pushed the crypto_acc_inline branch 2 times, most recently from 3ca66b7 to e541c5c Compare November 2, 2022 05:03
// packet.
// H - inout Headers that is the output of the parser block
// M - inout Metadata that is from the parser block and shared with the control
crypto_results_e encrypt_inline<H,M,T,S,I>(inout H hdr, inout M meta,
Copy link
Author

Choose a reason for hiding this comment

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

add a comment of order of operations. e.g. deparser or byte stream update must occur when the extern is invoked

Copy link
Author

Choose a reason for hiding this comment

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

add comment on preconditions and postconditions

Copy link
Author

Choose a reason for hiding this comment

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

add side effects

Copy link
Author

Choose a reason for hiding this comment

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

it may need packet in as well

…).

The example adds two methods for encrypt/decrypt that assumes that inline accelerators operate immediately on the packet (e.g. deparse, decrypt and reparse).  Packet recirculation is not necessary for either inline method.
The example shows the use of inline encrypt and decrypt, as well as how the crypto accelerator results can be used.
@apinski-cavium
Copy link
Contributor

https://p4.org/p4-spec/docs/P4-16-working-spec.html#sec-packet-data-extraction

The packet_in extern is special: it cannot be instantiated by the user explicitly. Instead, the architecture supplies a separate instance for each packet_in argument to a parser instantiation.

Copy link
Member

@thomascalvert-xlnx thomascalvert-xlnx left a comment

Choose a reason for hiding this comment

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

I suspect that the programming model proposed here (pseudo-recirculation) will be a source of confusion and bugs. For example, pre-conditions in the control block (leading up to the decrypt_inline call) could change, resulting in broken programmer's assumptions as well as inability to perform static analysis. Most targets are likely to implement this functionality by recirculating anyway, so it's not clear to me what we are gaining here with this clunky passing of pkt/hdr/meta arguments (imho).

The same effect can be easily achieved explicitly by the programmer:

control {
  if (recirculating == false) {
    (...) // set things up
    result = decrypt_inline(...);
    if (result == SUCCESS) {
      recirc(); // or whatever
    } else {
      (...) // deal with error
    }
  }
  else {
    (...) // continue processing decrypted packet
  }
}

With the advantage that it can then be used for other purposes too.

in T enable_auth,
in bit<32> spi,
in S seq,
in I iv);
Copy link
Member

Choose a reason for hiding this comment

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

Why include all these arguments rather than reuse the set_iv(), set_key(), etc. methods above?

@@ -122,6 +128,55 @@ extern crypto_accelerator {
void enable_encrypt<T>(in T enable_auth);
void enable_decrypt<T>(in T enable_auth);

Copy link
Member

Choose a reason for hiding this comment

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

Do we expect any targets to support both enable_encrypt and encrypt_inline ? If not, how would a program be written to be portable?

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.

6 participants