| Message ID | ZWWkDZRR33ypncn7@gondor.apana.org.au |
|---|---|
| State | Accepted |
| Commit | 67b164a871af1d736f131fd6fe78a610909f06f3 |
| Headers | show |
| Series | crypto: af_alg - Disallow multiple in-flight AIO requests | expand |
Hi Herbert, I have found a regression in userspace behaviour after this patch was merged into the 4.19.y kernel. The fix seems to involve backporting a few more changes. Could you review details below and confirm if this is the right approach? On Tue, Nov 28, 2023 at 04:25:49PM +0800, Herbert Xu wrote: >Having multiple in-flight AIO requests results in unpredictable >output because they all share the same IV. Fix this by only allowing >one request at a time. > >Fixes: 83094e5e9e49 ("crypto: af_alg - add async support to algif_aead") >Fixes: a596999b7ddf ("crypto: algif - change algif_skcipher to be asynchronous") >Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> >--- > crypto/af_alg.c | 14 +++++++++++++- > include/crypto/if_alg.h | 3 +++ > 2 files changed, 16 insertions(+), 1 deletion(-) This change got backported on the 4.19 kernel in January: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-4.19.y&id=19af0310c8767c993f2a5d5261e4df3f9f465ce1 Since then, I am seeing a regression in a simple openssl encoding test: openssl enc -k mysecret -aes-256-cbc -in plain.txt -out cipher.txt -engine afalg It fails intermittently with the message "error writing to file", but this error is a bit misleading, the actual problem is that the kernel returns -16 (EBUSY) on the encoding operation. This happens only in 4.19, and not under 5.10. The patch seems correct, however it seems we are missing a couple of other patches on 4.19: f3c802a1f3001 crypto: algif_aead - Only wake up when ctx->more is zero 21dfbcd1f5cbf crypto: algif_aead - fix uninitialized ctx->init I was able to cherry-pick those into 4.19.y, with just a minor conflict in one case. With those applied, the openssl command no longer fails. I suspect similar changes would be needed also in 5.4 kernel, however I neither checked that, nor have I run any tests on that version. Regards, -Ralph
[CCing the stable team, as it looks like two prerequisite changes for a patch already applied are missing in at least 4.19.y] On 15.03.24 18:55, Ralph Siemsen wrote: > > I have found a regression in userspace behaviour after this patch was > merged into the 4.19.y kernel. The fix seems to involve backporting a > few more changes. Could you review details below and confirm if this is > the right approach? FWIW, developers are totally free to not care about stable and longterm kernels series. Not sure if Herbert is among those developers, but it might explain why there is no reply yet. That's why I CCed the stable maintainers, strictly speaking they are responsible. > On Tue, Nov 28, 2023 at 04:25:49PM +0800, Herbert Xu wrote: >> Having multiple in-flight AIO requests results in unpredictable >> output because they all share the same IV. Fix this by only allowing >> one request at a time. > [...] > This change got backported on the 4.19 kernel in January: > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-4.19.y&id=19af0310c8767c993f2a5d5261e4df3f9f465ce1 > > Since then, I am seeCiao, ing a regression in a simple openssl encoding test: > > openssl enc -k mysecret -aes-256-cbc -in plain.txt -out cipher.txt > -engine afalg > > It fails intermittently with the message "error writing to file", but > this error is a bit misleading, the actual problem is that the kernel > returns -16 (EBUSY) on the encoding operation. > > This happens only in 4.19, and not under 5.10. The patch seems correct, > however it seems we are missing a couple of other patches on 4.19: > > f3c802a1f3001 crypto: algif_aead - Only wake up when ctx->more is zero > 21dfbcd1f5cbf crypto: algif_aead - fix uninitialized ctx->init > > I was able to cherry-pick those into 4.19.y, with just a minor conflict > in one case. With those applied, the openssl command no longer fails. Some feedback here from Herbert would of course be splendid, but maybe your tests are all the stable team needs to pick those up for a future 4.19.y release. > I suspect similar changes would be needed also in 5.4 kernel, however I > neither checked that, nor have I run any tests on that version. Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr If I did something stupid, please tell me, as explained on that page.
diff --git a/crypto/af_alg.c b/crypto/af_alg.c index ea6fb8e89d06..68cc9290cabe 100644 --- a/crypto/af_alg.c +++ b/crypto/af_alg.c @@ -1116,9 +1116,13 @@ EXPORT_SYMBOL_GPL(af_alg_sendmsg); void af_alg_free_resources(struct af_alg_async_req *areq) { struct sock *sk = areq->sk; + struct af_alg_ctx *ctx; af_alg_free_areq_sgls(areq); sock_kfree_s(sk, areq, areq->areqlen); + + ctx = alg_sk(sk)->private; + ctx->inflight = false; } EXPORT_SYMBOL_GPL(af_alg_free_resources); @@ -1188,11 +1192,19 @@ EXPORT_SYMBOL_GPL(af_alg_poll); struct af_alg_async_req *af_alg_alloc_areq(struct sock *sk, unsigned int areqlen) { - struct af_alg_async_req *areq = sock_kmalloc(sk, areqlen, GFP_KERNEL); + struct af_alg_ctx *ctx = alg_sk(sk)->private; + struct af_alg_async_req *areq; + /* Only one AIO request can be in flight. */ + if (ctx->inflight) + return ERR_PTR(-EBUSY); + + areq = sock_kmalloc(sk, areqlen, GFP_KERNEL); if (unlikely(!areq)) return ERR_PTR(-ENOMEM); + ctx->inflight = true; + areq->areqlen = areqlen; areq->sk = sk; areq->first_rsgl.sgl.sgt.sgl = areq->first_rsgl.sgl.sgl; diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h index ef8ce86b1f78..08b803a4fcde 100644 --- a/include/crypto/if_alg.h +++ b/include/crypto/if_alg.h @@ -136,6 +136,7 @@ struct af_alg_async_req { * recvmsg is invoked. * @init: True if metadata has been sent. * @len: Length of memory allocated for this data structure. + * @inflight: Non-zero when AIO requests are in flight. */ struct af_alg_ctx { struct list_head tsgl_list; @@ -154,6 +155,8 @@ struct af_alg_ctx { bool init; unsigned int len; + + unsigned int inflight; }; int af_alg_register_type(const struct af_alg_type *type);
Having multiple in-flight AIO requests results in unpredictable output because they all share the same IV. Fix this by only allowing one request at a time. Fixes: 83094e5e9e49 ("crypto: af_alg - add async support to algif_aead") Fixes: a596999b7ddf ("crypto: algif - change algif_skcipher to be asynchronous") Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> --- crypto/af_alg.c | 14 +++++++++++++- include/crypto/if_alg.h | 3 +++ 2 files changed, 16 insertions(+), 1 deletion(-)