| Message ID | 20180131160437.6583-1-vakul.garg@nxp.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [PATCHv2] tls: Add support for encryption using async offload accelerator | expand |
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 <gilad@benyossef.com> wrote: >> Hi Vakul, >> >> On Wed, Jan 31, 2018 at 12:36 PM, Vakul Garg <vakul.garg@nxp.com> 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.
From: Vakul Garg <vakul.garg@nxp.com> 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 <vakul.garg@nxp.com> > --- > v1-v2: > - Used crypto_wait_req() to wait for async operation completion > - Passed CRYPTO_TFM_REQ_MAY_BACKLOG to crypto_aead_encrypt Applied.
> -----Original Message----- > From: Dave Watson [mailto:davejwatson@fb.com] > Sent: Wednesday, January 31, 2018 8:52 PM > To: Vakul Garg <vakul.garg@nxp.com> > Cc: linux-crypto@vger.kernel.org; ilyal@mellanox.com; > aviadye@mellanox.com; davem@davemloft.net; netdev@vger.kernel.org; > Gilad Ben-Yossef <gilad@benyossef.com> > 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 > <gilad@benyossef.com> wrote: > >> Hi Vakul, > >> > >> On Wed, Jan 31, 2018 at 12:36 PM, Vakul Garg <vakul.garg@nxp.com> > 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?
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.
On Wed, Jan 31, 2018 at 7:34 PM, Dave Watson <davejwatson@fb.com> 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
> -----Original Message----- > From: Dave Watson [mailto:davejwatson@fb.com] > Sent: Wednesday, January 31, 2018 11:05 PM > To: Vakul Garg <vakul.garg@nxp.com> > Cc: linux-crypto@vger.kernel.org; ilyal@mellanox.com; > aviadye@mellanox.com; davem@davemloft.net; netdev@vger.kernel.org; > Gilad Ben-Yossef <gilad@benyossef.com> > 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.
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;
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 <vakul.garg@nxp.com> --- 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(-)