| Message ID | 9681d1febfec295449a62300938ed2ae66983f28.1694018970.git.sd@queasysnail.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tls: fix some issues with async encryption | expand |
On Wed, 6 Sep 2023 19:08:31 +0200 Sabrina Dubroca wrote: > --- a/net/tls/tls_sw.c > +++ b/net/tls/tls_sw.c > @@ -196,6 +196,9 @@ static void tls_decrypt_done(void *data, int err) > struct sock *sk; > int aead_size; > > + if (err == -EINPROGRESS) > + return; Maybe a comment here clarifying that caller got -EBUSY and the callback will fire again without an error? The flow is slightly counter- -intuitive. > @@ -443,6 +446,9 @@ static void tls_encrypt_done(void *data, int err) > struct sock *sk; > int pending; > > + if (err == -EINPROGRESS) > + return; Same here? Reviewed-by: Jakub Kicinski <kuba@kernel.org>
On Wed, Sep 06, 2023 at 07:08:31PM +0200, Sabrina Dubroca wrote: > Since we're setting the CRYPTO_TFM_REQ_MAY_BACKLOG flag on our > requests to the crypto API, crypto_aead_{encrypt,decrypt} can return > -EBUSY instead of -EINPROGRESS in valid situations. For example, when > the cryptd queue for AESNI is full (easy to trigger with an > artifically low cryptd.cryptd_max_cpu_qlen), requests will be enqueued Hi Sabrina, as it looks like there will be a v2, checkpatch --codespell, asked me to suggest: artifically -> artificially ...
Sabrina Dubroca <sd@queasysnail.net> wrote: > Since we're setting the CRYPTO_TFM_REQ_MAY_BACKLOG flag on our > requests to the crypto API, crypto_aead_{encrypt,decrypt} can return > -EBUSY instead of -EINPROGRESS in valid situations. For example, when > the cryptd queue for AESNI is full (easy to trigger with an > artifically low cryptd.cryptd_max_cpu_qlen), requests will be enqueued > to the backlog but still processed. In that case, the async callback > will also be called twice: first with err == -EINPROGRESS, which it > seems we can just ignore, then with err == 0. > > I've only tested this on AESNI with cryptd. > > Fixes: a54667f6728c ("tls: Add support for encryption using async offload accelerator") > Fixes: 94524d8fc965 ("net/tls: Add support for async decryption of tls records") > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> > --- > net/tls/tls_sw.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) You should only use MAY_BACKLOG if you can actually back off and stop issuing new requests. In that case you can only restart issuing new requests when the EINPROGRESS notification comes in. If that's not the case here you should drop MAY_BACKLOG altogether. Cheers,
Thanks for looking at this patch. In retrospect I should have cc'd you on it. 2023-09-08, 14:10:05 +0800, Herbert Xu wrote: > Sabrina Dubroca <sd@queasysnail.net> wrote: > > Since we're setting the CRYPTO_TFM_REQ_MAY_BACKLOG flag on our > > requests to the crypto API, crypto_aead_{encrypt,decrypt} can return > > -EBUSY instead of -EINPROGRESS in valid situations. For example, when > > the cryptd queue for AESNI is full (easy to trigger with an > > artifically low cryptd.cryptd_max_cpu_qlen), requests will be enqueued > > to the backlog but still processed. In that case, the async callback > > will also be called twice: first with err == -EINPROGRESS, which it > > seems we can just ignore, then with err == 0. > > > > I've only tested this on AESNI with cryptd. > > > > Fixes: a54667f6728c ("tls: Add support for encryption using async offload accelerator") > > Fixes: 94524d8fc965 ("net/tls: Add support for async decryption of tls records") > > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> > > --- > > net/tls/tls_sw.c | 23 +++++++++++++++-------- > > 1 file changed, 15 insertions(+), 8 deletions(-) > > You should only use MAY_BACKLOG if you can actually back off and > stop issuing new requests. In that case you can only restart > issuing new requests when the EINPROGRESS notification comes in. > > If that's not the case here you should drop MAY_BACKLOG altogether. Uh, ok, I didn't know that, thanks for explaining. When I was fixing this code I couldn't find a mention of what the expectations for MAY_BACKLOG are. Could you add a comment describing this in the headers (either for #define CRYPTO_TFM_REQ_MAY_BACKLOG or aead_request_set_callback, wherever is more appropriate). MAY_BACKLOG is used by both tls and tipc (talking only about networking) and neither seem to respect this need to back off. Jakub, I guess we should drop the CRYPTO_TFM_REQ_MAY_BACKLOG for net, and maybe consider adding it back (with the back off) in net-next. Probably not urgent considering that nobody seems to have run into this bug so far. But then we have to handle ENOSPC a bit more gracefully, because right now it looks like - on TX, we break the socket (tls_err_abort when tls_do_encryption returns an error) - on RX, we also break the socket, and we don't decrement decrypt_pending so the recv() call gets stuck Not sure how complex the changes would be, the sendmsg and recvmsg code is already a bit hard to follow.
On Fri, 8 Sep 2023 17:55:59 +0200 Sabrina Dubroca wrote: > Jakub, I guess we should drop the CRYPTO_TFM_REQ_MAY_BACKLOG for net, > and maybe consider adding it back (with the back off) in > net-next. Probably not urgent considering that nobody seems to have > run into this bug so far. Someone did mention it long time ago, but I don't recall the context :S I think it was something about the device queue filling up.. > But then we have to handle ENOSPC a bit more gracefully, because right > now it looks like > - on TX, we break the socket (tls_err_abort when tls_do_encryption returns > an error) > - on RX, we also break the socket, and we don't decrement > decrypt_pending so the recv() call gets stuck > > Not sure how complex the changes would be, the sendmsg and recvmsg > code is already a bit hard to follow. To keep it simple we can wait for all in-flight requests to drain if we hit EBUSY? Basically factor this bit out: spin_lock_bh(&ctx->decrypt_compl_lock); reinit_completion(&ctx->async_wait.completion); pending = atomic_read(&ctx->decrypt_pending); spin_unlock_bh(&ctx->decrypt_compl_lock); ret = 0; if (pending) ret = crypto_wait_req(-EINPROGRESS, &ctx->async_wait); and call it after we get EBUSY? We'll drain the pending queue all the way to empty, which may not be too great for throughput, but who cares - right now we don't handle EBUSY at all, so it must be extremely rare.
On Fri, Sep 08, 2023 at 02:26:02PM -0700, Jakub Kicinski wrote: > > and call it after we get EBUSY? We'll drain the pending queue all the > way to empty, which may not be too great for throughput, but who cares > - right now we don't handle EBUSY at all, so it must be extremely rare. EBUSY means that you're sending requests to the Crypto API faster than it can process them. If left unhandled you will eventually exhaust all memory. Cheers,
On Fri, Sep 08, 2023 at 05:55:59PM +0200, Sabrina Dubroca wrote: > > Uh, ok, I didn't know that, thanks for explaining. When I was fixing > this code I couldn't find a mention of what the expectations for > MAY_BACKLOG are. Could you add a comment describing this in the > headers (either for #define CRYPTO_TFM_REQ_MAY_BACKLOG or > aead_request_set_callback, wherever is more appropriate). MAY_BACKLOG > is used by both tls and tipc (talking only about networking) and > neither seem to respect this need to back off. Patches are welcome :) A bit of history: at the beginning we always dropped requests that we couldn't queue because the only user was IPsec so this is the expected behaviour. When storage crypto support was added there was a need for reliable handling of resource constraints so that's why MAY_BACKLOG was added. However, the expectation is obviously that you must stop sending new requests once you run into the resource constraint. > Jakub, I guess we should drop the CRYPTO_TFM_REQ_MAY_BACKLOG for net, > and maybe consider adding it back (with the back off) in > net-next. Probably not urgent considering that nobody seems to have > run into this bug so far. I think that would be the prudent action. Thanks,
2023-09-12, 12:43:32 +0800, Herbert Xu wrote: > On Fri, Sep 08, 2023 at 05:55:59PM +0200, Sabrina Dubroca wrote: > > > > Uh, ok, I didn't know that, thanks for explaining. When I was fixing > > this code I couldn't find a mention of what the expectations for > > MAY_BACKLOG are. Could you add a comment describing this in the > > headers (either for #define CRYPTO_TFM_REQ_MAY_BACKLOG or > > aead_request_set_callback, wherever is more appropriate). MAY_BACKLOG > > is used by both tls and tipc (talking only about networking) and > > neither seem to respect this need to back off. > > Patches are welcome :) Ok. I thought it'd be better if you wrote that patch since if I write it, it'll be a c/p (or rephrase) of what you wrote. But fine, I'll go ahead and do that :) > A bit of history: at the beginning we always dropped requests > that we couldn't queue because the only user was IPsec so this > is the expected behaviour. > > When storage crypto support was added there was a need for reliable > handling of resource constraints so that's why MAY_BACKLOG was added. > However, the expectation is obviously that you must stop sending new > requests once you run into the resource constraint. > > > Jakub, I guess we should drop the CRYPTO_TFM_REQ_MAY_BACKLOG for net, > > and maybe consider adding it back (with the back off) in > > net-next. Probably not urgent considering that nobody seems to have > > run into this bug so far. > > I think that would be the prudent action. We'd have to do pretty much what Jakub suggested [1] (handle ENOSPC by waiting for all current requests) and then resubmit the failed request. So I think keeping the MAY_BACKLOG flag and handling EBUSY this way is simpler. With this, we send one request to the backlog, then we wait until the queue drains. [1] https://lore.kernel.org/netdev/20230908142602.2ced0631@kernel.org/
On Tue, Sep 12, 2023 at 05:37:23PM +0200, Sabrina Dubroca wrote: > > We'd have to do pretty much what Jakub suggested [1] (handle ENOSPC by > waiting for all current requests) and then resubmit the failed > request. So I think keeping the MAY_BACKLOG flag and handling EBUSY > this way is simpler. With this, we send one request to the backlog, > then we wait until the queue drains. > > [1] https://lore.kernel.org/netdev/20230908142602.2ced0631@kernel.org/ You need to handle something like ENOSPC anyhow because the underlying driver may experience a real failure (e.g., someone unplugged the hardware) which would have pretty much the same effect of ENOSPC (i.e., an error with no retries). So if the tls code can't cope with that then it has to be fixed. Cheers,
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index 1ed4a611631f..4f3dd0403efb 100644 --- a/net/tls/tls_sw.c +++ b/net/tls/tls_sw.c @@ -196,6 +196,9 @@ static void tls_decrypt_done(void *data, int err) struct sock *sk; int aead_size; + if (err == -EINPROGRESS) + return; + aead_size = sizeof(*aead_req) + crypto_aead_reqsize(aead); aead_size = ALIGN(aead_size, __alignof__(*dctx)); dctx = (void *)((u8 *)aead_req + aead_size); @@ -261,7 +264,7 @@ static int tls_do_decryption(struct sock *sk, } ret = crypto_aead_decrypt(aead_req); - if (ret == -EINPROGRESS) { + if (ret == -EINPROGRESS || ret == -EBUSY) { if (darg->async) return 0; @@ -443,6 +446,9 @@ static void tls_encrypt_done(void *data, int err) struct sock *sk; int pending; + if (err == -EINPROGRESS) + return; + msg_en = &rec->msg_encrypted; sk = rec->sk; @@ -544,7 +550,7 @@ static int tls_do_encryption(struct sock *sk, atomic_inc(&ctx->encrypt_pending); rc = crypto_aead_encrypt(aead_req); - if (!rc || rc != -EINPROGRESS) { + if (!rc || (rc != -EINPROGRESS && rc != -EBUSY)) { atomic_dec(&ctx->encrypt_pending); sge->offset -= prot->prepend_size; sge->length += prot->prepend_size; @@ -552,7 +558,7 @@ static int tls_do_encryption(struct sock *sk, if (!rc) { WRITE_ONCE(rec->tx_ready, true); - } else if (rc != -EINPROGRESS) { + } else if (rc != -EINPROGRESS && rc != -EBUSY) { list_del(&rec->list); return rc; } @@ -779,7 +785,7 @@ static int tls_push_record(struct sock *sk, int flags, rc = tls_do_encryption(sk, tls_ctx, ctx, req, msg_pl->sg.size + prot->tail_size, i); if (rc < 0) { - if (rc != -EINPROGRESS) { + if (rc != -EINPROGRESS && rc != -EBUSY) { tls_err_abort(sk, -EBADMSG); if (split) { tls_ctx->pending_open_record_frags = true; @@ -990,7 +996,7 @@ static int tls_sw_sendmsg_locked(struct sock *sk, struct msghdr *msg, if (unlikely(msg->msg_controllen)) { ret = tls_process_cmsg(sk, msg, &record_type); if (ret) { - if (ret == -EINPROGRESS) + if (ret == -EINPROGRESS || ret == -EBUSY) num_async++; else if (ret != -EAGAIN) goto send_end; @@ -1071,7 +1077,7 @@ static int tls_sw_sendmsg_locked(struct sock *sk, struct msghdr *msg, record_type, &copied, msg->msg_flags); if (ret) { - if (ret == -EINPROGRESS) + if (ret == -EINPROGRESS || ret == -EBUSY) num_async++; else if (ret == -ENOMEM) goto wait_for_memory; @@ -1125,7 +1131,7 @@ static int tls_sw_sendmsg_locked(struct sock *sk, struct msghdr *msg, record_type, &copied, msg->msg_flags); if (ret) { - if (ret == -EINPROGRESS) + if (ret == -EINPROGRESS || ret == -EBUSY) num_async++; else if (ret == -ENOMEM) goto wait_for_memory; @@ -1248,6 +1254,7 @@ void tls_sw_splice_eof(struct socket *sock) goto unlock; retrying = true; goto retry; + case -EBUSY: case -EINPROGRESS: break; default: @@ -2106,7 +2113,7 @@ int tls_sw_recvmsg(struct sock *sk, __skb_queue_purge(&ctx->async_hold); if (ret) { - if (err >= 0 || err == -EINPROGRESS) + if (err >= 0 || err == -EINPROGRESS || err == -EBUSY) err = ret; decrypted = 0; goto end;
Since we're setting the CRYPTO_TFM_REQ_MAY_BACKLOG flag on our requests to the crypto API, crypto_aead_{encrypt,decrypt} can return -EBUSY instead of -EINPROGRESS in valid situations. For example, when the cryptd queue for AESNI is full (easy to trigger with an artifically low cryptd.cryptd_max_cpu_qlen), requests will be enqueued to the backlog but still processed. In that case, the async callback will also be called twice: first with err == -EINPROGRESS, which it seems we can just ignore, then with err == 0. I've only tested this on AESNI with cryptd. Fixes: a54667f6728c ("tls: Add support for encryption using async offload accelerator") Fixes: 94524d8fc965 ("net/tls: Add support for async decryption of tls records") Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> --- net/tls/tls_sw.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)