2018-01-31 16:04:37

by Vakul Garg

[permalink] [raw]
Subject: [PATCHv2] tls: Add support for encryption using async offload accelerator

Async crypto accelerators (e.g. drivers/crypto/caam) support offloading
GCM operation. If they are enabled, crypto_aead_encrypt() return error
code -EINPROGRESS. In this case tls_do_encryption() needs to wait on a
completion till the time the response for crypto offload request is
received.

Signed-off-by: Vakul Garg <[email protected]>
---
v1-v2:
- Used crypto_wait_req() to wait for async operation completion
- Passed CRYPTO_TFM_REQ_MAY_BACKLOG to crypto_aead_encrypt

include/net/tls.h | 2 ++
net/tls/tls_sw.c | 8 +++++++-
2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/net/tls.h b/include/net/tls.h
index 936cfc5cab7d..8a8e1256caee 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -36,6 +36,7 @@

#include <linux/types.h>
#include <asm/byteorder.h>
+#include <linux/crypto.h>
#include <linux/socket.h>
#include <linux/tcp.h>
#include <net/tcp.h>
@@ -57,6 +58,7 @@

struct tls_sw_context {
struct crypto_aead *aead_send;
+ struct crypto_wait async_wait;

/* Sending context */
char aad_space[TLS_AAD_SPACE_SIZE];
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 73d19210dd49..31a9f0ef8af6 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -214,7 +214,11 @@ static int tls_do_encryption(struct tls_context *tls_ctx,
aead_request_set_ad(aead_req, TLS_AAD_SPACE_SIZE);
aead_request_set_crypt(aead_req, ctx->sg_aead_in, ctx->sg_aead_out,
data_len, tls_ctx->iv);
- rc = crypto_aead_encrypt(aead_req);
+
+ aead_request_set_callback(aead_req, CRYPTO_TFM_REQ_MAY_BACKLOG,
+ crypto_req_done, &ctx->async_wait);
+
+ rc = crypto_wait_req(crypto_aead_encrypt(aead_req), &ctx->async_wait);

ctx->sg_encrypted_data[0].offset -= tls_ctx->prepend_size;
ctx->sg_encrypted_data[0].length += tls_ctx->prepend_size;
@@ -663,6 +667,8 @@ int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx)
goto out;
}

+ crypto_init_wait(&sw_ctx->async_wait);
+
ctx->priv_ctx = (struct tls_offload_context *)sw_ctx;

crypto_info = &ctx->crypto_send;
--
2.13.6


2018-01-31 15:22:28

by Dave Watson

[permalink] [raw]
Subject: Re: [PATCHv2] tls: Add support for encryption using async offload accelerator

On 01/31/18 09:34 PM, Vakul Garg wrote:
> Async crypto accelerators (e.g. drivers/crypto/caam) support offloading
> GCM operation. If they are enabled, crypto_aead_encrypt() return error
> code -EINPROGRESS. In this case tls_do_encryption() needs to wait on a
> completion till the time the response for crypto offload request is
> received.

Comments from V1
> On Wed, Jan 31, 2018 at 8:10 AM, Gilad Ben-Yossef <[email protected]> wrote:
>> Hi Vakul,
>>
>> On Wed, Jan 31, 2018 at 12:36 PM, Vakul Garg <[email protected]> wrote:
>>> Async crypto accelerators (e.g. drivers/crypto/caam) support offloading
>>> GCM operation. If they are enabled, crypto_aead_encrypt() return error
>>> code -EINPROGRESS. In this case tls_do_encryption() needs to wait on a
>>> completion till the time the response for crypto offload request is
>>> received.
>>>
>>
>> Thank you for this patch. I think it is actually a bug fix and should
>> probably go into stable
>
> On second though in stable we should probably just disable async tfm
> allocations.
> It's simpler. But this approach is still good for -next
>
>
> Gilad

I agree with Gilad, just disable async for now.

If the flag MSG_DONTWAIT is set, we should be returning -EINPROGRESS
and not wait for a response. I had started working on a patch for
that, but it's pretty tricky to get right.

2018-01-31 15:33:29

by David Miller

[permalink] [raw]
Subject: Re: [PATCHv2] tls: Add support for encryption using async offload accelerator

From: Vakul Garg <[email protected]>
Date: Wed, 31 Jan 2018 21:34:37 +0530

> Async crypto accelerators (e.g. drivers/crypto/caam) support offloading
> GCM operation. If they are enabled, crypto_aead_encrypt() return error
> code -EINPROGRESS. In this case tls_do_encryption() needs to wait on a
> completion till the time the response for crypto offload request is
> received.
>
> Signed-off-by: Vakul Garg <[email protected]>
> ---
> v1-v2:
> - Used crypto_wait_req() to wait for async operation completion
> - Passed CRYPTO_TFM_REQ_MAY_BACKLOG to crypto_aead_encrypt

Applied.

2018-01-31 17:22:41

by Vakul Garg

[permalink] [raw]
Subject: RE: [PATCHv2] tls: Add support for encryption using async offload accelerator



> -----Original Message-----
> From: Dave Watson [mailto:[email protected]]
> Sent: Wednesday, January 31, 2018 8:52 PM
> To: Vakul Garg <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> Gilad Ben-Yossef <[email protected]>
> Subject: Re: [PATCHv2] tls: Add support for encryption using async offload
> accelerator
>
> On 01/31/18 09:34 PM, Vakul Garg wrote:
> > Async crypto accelerators (e.g. drivers/crypto/caam) support
> > offloading GCM operation. If they are enabled, crypto_aead_encrypt()
> > return error code -EINPROGRESS. In this case tls_do_encryption() needs
> > to wait on a completion till the time the response for crypto offload
> > request is received.
>
> Comments from V1
> > On Wed, Jan 31, 2018 at 8:10 AM, Gilad Ben-Yossef
> <[email protected]> wrote:
> >> Hi Vakul,
> >>
> >> On Wed, Jan 31, 2018 at 12:36 PM, Vakul Garg <[email protected]>
> wrote:
> >>> Async crypto accelerators (e.g. drivers/crypto/caam) support
> >>> offloading GCM operation. If they are enabled, crypto_aead_encrypt()
> >>> return error code -EINPROGRESS. In this case tls_do_encryption()
> >>> needs to wait on a completion till the time the response for crypto
> >>> offload request is received.
> >>>
> >>
> >> Thank you for this patch. I think it is actually a bug fix and should
> >> probably go into stable
> >
> > On second though in stable we should probably just disable async tfm
> > allocations.
> > It's simpler. But this approach is still good for -next
> >
> >
> > Gilad
>
> I agree with Gilad, just disable async for now.
>

How to do it? Can you help with the api name?

> If the flag MSG_DONTWAIT is set, we should be returning -EINPROGRESS and
> not wait for a response. I had started working on a patch for that, but it's
> pretty tricky to get right.

Can you point me to your WIP code branch for this?

If MSG_DONTWAIT is not used, will it be sane if enqueue the crypto request to
accelerator and return to user space back so that user space can send more plaintext data while
crypto accelerator is working in parallel?


2018-01-31 17:38:08

by Dave Watson

[permalink] [raw]
Subject: Re: [PATCHv2] tls: Add support for encryption using async offload accelerator

On 01/31/18 05:22 PM, Vakul Garg wrote:
> > > On second though in stable we should probably just disable async tfm
> > > allocations.
> > > It's simpler. But this approach is still good for -next
> > >
> > >
> > > Gilad
> >
> > I agree with Gilad, just disable async for now.
> >
>
> How to do it? Can you help with the api name?

*aead = crypto_alloc_aead("gcm(aes)", 0, CRYPTO_ALG_ASYNC);

https://github.com/ktls/net_next_ktls/commit/f3b9b402e755e4b0623fa83f88137173fc249f2d

> > If the flag MSG_DONTWAIT is set, we should be returning -EINPROGRESS and
> > not wait for a response. I had started working on a patch for that, but it's
> > pretty tricky to get right.
>
> Can you point me to your WIP code branch for this?

https://github.com/ktls/net_next_ktls/commit/9cc839aa551ed972d148ecebf353b25ee93543b9

> If MSG_DONTWAIT is not used, will it be sane if enqueue the crypto request to
> accelerator and return to user space back so that user space can send more plaintext data while
> crypto accelerator is working in parallel?

Right, that's roughly what the above does. I believe the tricky
unfinished part was getting poll() to work correctly if there is an
async crypto request outstanding. Currently the tls poll() just
relies on backpressure from do_tcp_sendpages.

2018-01-31 17:48:09

by Gilad Ben-Yossef

[permalink] [raw]
Subject: Re: [PATCHv2] tls: Add support for encryption using async offload accelerator

On Wed, Jan 31, 2018 at 7:34 PM, Dave Watson <[email protected]> wrote:
> On 01/31/18 05:22 PM, Vakul Garg wrote:
>> > > On second though in stable we should probably just disable async tfm
>> > > allocations.
>> > > It's simpler. But this approach is still good for -next
>> > >
>> > >
>> > > Gilad
>> >
>> > I agree with Gilad, just disable async for now.
>> >
>>
>> How to do it? Can you help with the api name?
>
> *aead = crypto_alloc_aead("gcm(aes)", 0, CRYPTO_ALG_ASYNC);
>
> https://github.com/ktls/net_next_ktls/commit/f3b9b402e755e4b0623fa83f88137173fc249f2d

I said disabling async tfms is the right way to go for -stable since
it's the simplest and less risky way
of plugging this bug.

I don't think this is the way to go for -next (and it seems davem
agrees with me). Vakul's
patch looks good to me for now.

>
>> > If the flag MSG_DONTWAIT is set, we should be returning -EINPROGRESS and
>> > not wait for a response. I had started working on a patch for that, but it's
>> > pretty tricky to get right.
>>

That would be a great addition, but I don't think we need to wait for
that. It can come later.

Gilad
--
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
-- Jean-Baptiste Queru

2018-02-01 06:17:53

by Vakul Garg

[permalink] [raw]
Subject: RE: [PATCHv2] tls: Add support for encryption using async offload accelerator



> -----Original Message-----
> From: Dave Watson [mailto:[email protected]]
> Sent: Wednesday, January 31, 2018 11:05 PM
> To: Vakul Garg <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> Gilad Ben-Yossef <[email protected]>
> Subject: Re: [PATCHv2] tls: Add support for encryption using async offload
> accelerator
>
> On 01/31/18 05:22 PM, Vakul Garg wrote:
> > > > On second though in stable we should probably just disable async
> > > > tfm allocations.
> > > > It's simpler. But this approach is still good for -next
> > > >
> > > >
> > > > Gilad
> > >
> > > I agree with Gilad, just disable async for now.
> > >
> >
> > How to do it? Can you help with the api name?
>
> *aead = crypto_alloc_aead("gcm(aes)", 0, CRYPTO_ALG_ASYNC);
>
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit
> hub.com%2Fktls%2Fnet_next_ktls%2Fcommit%2Ff3b9b402e755e4b0623fa8
> 3f88137173fc249f2d&data=02%7C01%7Cvakul.garg%40nxp.com%7Cf1c707
> 9af97e48c9e89308d568d1633a%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C636530170887502735&sdata=qTdwbuDsCRmK1UHAYiOcwfPC
> U%2FPXgKg%2BICh%2F2N0b9Nw%3D&reserved=0
>

The above patch you authored works fine. I tested it on my test platform.
I have nothing to contribute here.
Would you send it to stable kernel yourself or still want me to do it?


> > > If the flag MSG_DONTWAIT is set, we should be returning -EINPROGRESS
> > > and not wait for a response. I had started working on a patch for
> > > that, but it's pretty tricky to get right.
> >
> > Can you point me to your WIP code branch for this?
>
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit
> hub.com%2Fktls%2Fnet_next_ktls%2Fcommit%2F9cc839aa551ed972d148ec
> ebf353b25ee93543b9&data=02%7C01%7Cvakul.garg%40nxp.com%7Cf1c70
> 79af97e48c9e89308d568d1633a%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C636530170887502735&sdata=0ASwmPPXXSGCXpBGes9vBma
> y8Gojv0wSUGAOOOjBExc%3D&reserved=0
>
> > If MSG_DONTWAIT is not used, will it be sane if enqueue the crypto
> > request to accelerator and return to user space back so that user
> > space can send more plaintext data while crypto accelerator is working in
> parallel?
>
> Right, that's roughly what the above does. I believe the tricky unfinished part
> was getting poll() to work correctly if there is an async crypto request
> outstanding. Currently the tls poll() just relies on backpressure from
> do_tcp_sendpages.

I wanted to add async accelerator support for ktls where multiple crypto requests can be pipelined.
But since you are already doing it, I won't duplicate the efforts.
I would leverage your work on my platform, test it and try to contribute from here.