| Message ID | 20230530141635.136968-11-dhowells@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | crypto, splice, net: Make AF_ALG handle sendmsg(MSG_SPLICE_PAGES) | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 3 of 3 maintainers |
| netdev/build_clang | success | Errors and warnings before: 8 this patch: 8 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/checkpatch | warning | CHECK: Lines should not end with a '(' |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Tue, May 30, 2023 at 03:16:34PM +0100, David Howells wrote: > > - if (limit > sk->sk_sndbuf) > - limit = sk->sk_sndbuf; > + /* Don't limit to ALG_MAX_PAGES if the pages are all already pinned. */ > + if (!user_backed_iter(&msg->msg_iter)) > + max_pages = INT_MAX; > + else > + max_pages = min_t(size_t, max_pages, > + DIV_ROUND_UP(sk->sk_sndbuf, PAGE_SIZE)); What's the purpose of relaxing this limit? Even if there is a reason for this shouldn't this be in a patch by itself? Thanks,
Herbert Xu <herbert@gondor.apana.org.au> wrote: > > - if (limit > sk->sk_sndbuf) > > - limit = sk->sk_sndbuf; > > + /* Don't limit to ALG_MAX_PAGES if the pages are all already pinned. */ > > + if (!user_backed_iter(&msg->msg_iter)) > > + max_pages = INT_MAX; If the iov_iter is a kernel-backed type (BVEC, KVEC, XARRAY) then (a) all the pages it refers to must already be pinned in memory and (b) the caller must have limited it in some way (splice is limited by the pipe capacity, for instance). In which case, it seems pointless taking more than one pass of the while loop if we can avoid it - at least from the point of view of memory handling; granted there might be other criteria such as hogging crypto offload hardware. > > + else > > + max_pages = min_t(size_t, max_pages, > > + DIV_ROUND_UP(sk->sk_sndbuf, PAGE_SIZE)); > > What's the purpose of relaxing this limit? If the iov_iter is a user-backed type (IOVEC or UBUF) then it's not relaxed. max_pages is ALG_MAX_PAGES here (actually, I should just move that here so that it's clearer). I am, however, applying the sk_sndbuf limit here also - there's no point extracting more pages than we need to if ALG_MAX_PAGES of whole pages would overrun the byte limit. > Even if there is a reason for this shouldn't this be in a patch by itself? I suppose I could do it as a follow-on patch; use ALG_MAX_PAGES and sk_sndbuf before that as for user-backed iterators. Actually, is it worth paying attention to sk_sndbuf for kernel-backed iterators? David
On Tue, Jun 06, 2023 at 10:24:55AM +0100, David Howells wrote: > > If the iov_iter is a user-backed type (IOVEC or UBUF) then it's not relaxed. > max_pages is ALG_MAX_PAGES here (actually, I should just move that here so > that it's clearer). Even if it's kernel memory they can't be freed during the hashing operation, which could be long if the amount is large (or the algo is slow). The reason for the limit here is to stop a malicious user from pinning an unlimited amount of memory by doing a hashing operation, IOW a DoS attack. So I think we should keep the limit as is. Cheers,
Herbert Xu <herbert@gondor.apana.org.au> wrote:
> So I think we should keep the limit as is.
Okay.
David
diff --git a/crypto/af_alg.c b/crypto/af_alg.c index e2fc9051ba39..b78a399d0e19 100644 --- a/crypto/af_alg.c +++ b/crypto/af_alg.c @@ -542,9 +542,14 @@ void af_alg_free_sg(struct af_alg_sgl *sgl) { int i; - if (sgl->need_unpin) - for (i = 0; i < sgl->sgt.nents; i++) - unpin_user_page(sg_page(&sgl->sgt.sgl[i])); + if (sgl->sgt.sgl) { + if (sgl->need_unpin) + for (i = 0; i < sgl->sgt.nents; i++) + unpin_user_page(sg_page(&sgl->sgt.sgl[i])); + if (sgl->sgt.sgl != sgl->sgl) + kvfree(sgl->sgt.sgl); + sgl->sgt.sgl = NULL; + } } EXPORT_SYMBOL_GPL(af_alg_free_sg); diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c index 16c69c4b9c62..2f7a98b0eae3 100644 --- a/crypto/algif_hash.c +++ b/crypto/algif_hash.c @@ -63,78 +63,106 @@ static void hash_free_result(struct sock *sk, struct hash_ctx *ctx) static int hash_sendmsg(struct socket *sock, struct msghdr *msg, size_t ignored) { - int limit = ALG_MAX_PAGES * PAGE_SIZE; struct sock *sk = sock->sk; struct alg_sock *ask = alg_sk(sk); struct hash_ctx *ctx = ask->private; - long copied = 0; + ssize_t copied = 0; + size_t len, max_pages = ALG_MAX_PAGES, npages; + bool continuing = ctx->more, need_init = false; int err; - if (limit > sk->sk_sndbuf) - limit = sk->sk_sndbuf; + /* Don't limit to ALG_MAX_PAGES if the pages are all already pinned. */ + if (!user_backed_iter(&msg->msg_iter)) + max_pages = INT_MAX; + else + max_pages = min_t(size_t, max_pages, + DIV_ROUND_UP(sk->sk_sndbuf, PAGE_SIZE)); lock_sock(sk); - if (!ctx->more) { + if (!continuing) { if ((msg->msg_flags & MSG_MORE)) hash_free_result(sk, ctx); - - err = crypto_wait_req(crypto_ahash_init(&ctx->req), &ctx->wait); - if (err) - goto unlock; + need_init = true; } ctx->more = false; while (msg_data_left(msg)) { - int len = msg_data_left(msg); - - if (len > limit) - len = limit; - ctx->sgl.sgt.sgl = ctx->sgl.sgl; ctx->sgl.sgt.nents = 0; ctx->sgl.sgt.orig_nents = 0; - len = extract_iter_to_sg(&msg->msg_iter, len, &ctx->sgl.sgt, - ALG_MAX_PAGES, 0); - if (len < 0) { - err = copied ? 0 : len; - goto unlock; + err = -EIO; + npages = iov_iter_npages(&msg->msg_iter, max_pages); + if (npages == 0) + goto unlock_free; + + if (npages > ARRAY_SIZE(ctx->sgl.sgl)) { + err = -ENOMEM; + ctx->sgl.sgt.sgl = + kvmalloc(array_size(npages, + sizeof(*ctx->sgl.sgt.sgl)), + GFP_KERNEL); + if (!ctx->sgl.sgt.sgl) + goto unlock_free; } - sg_mark_end(ctx->sgl.sgt.sgl + ctx->sgl.sgt.nents); + sg_init_table(ctx->sgl.sgl, npages); ctx->sgl.need_unpin = iov_iter_extract_will_pin(&msg->msg_iter); - ahash_request_set_crypt(&ctx->req, ctx->sgl.sgt.sgl, NULL, len); + err = extract_iter_to_sg(&msg->msg_iter, LONG_MAX, + &ctx->sgl.sgt, npages, 0); + if (err < 0) + goto unlock_free; + len = err; + sg_mark_end(ctx->sgl.sgt.sgl + ctx->sgl.sgt.nents - 1); - err = crypto_wait_req(crypto_ahash_update(&ctx->req), - &ctx->wait); - af_alg_free_sg(&ctx->sgl); - if (err) { - iov_iter_revert(&msg->msg_iter, len); - goto unlock; + if (!msg_data_left(msg)) { + err = hash_alloc_result(sk, ctx); + if (err) + goto unlock_free; } - copied += len; - } + ahash_request_set_crypt(&ctx->req, ctx->sgl.sgt.sgl, + ctx->result, len); - err = 0; + if (!msg_data_left(msg) && !continuing && + !(msg->msg_flags & MSG_MORE)) { + err = crypto_ahash_digest(&ctx->req); + } else { + if (need_init) { + err = crypto_wait_req( + crypto_ahash_init(&ctx->req), + &ctx->wait); + if (err) + goto unlock_free; + need_init = false; + } + + if (msg_data_left(msg) || (msg->msg_flags & MSG_MORE)) + err = crypto_ahash_update(&ctx->req); + else + err = crypto_ahash_finup(&ctx->req); + continuing = true; + } - ctx->more = msg->msg_flags & MSG_MORE; - if (!ctx->more) { - err = hash_alloc_result(sk, ctx); + err = crypto_wait_req(err, &ctx->wait); if (err) - goto unlock; + goto unlock_free; - ahash_request_set_crypt(&ctx->req, NULL, ctx->result, 0); - err = crypto_wait_req(crypto_ahash_final(&ctx->req), - &ctx->wait); + copied += len; + af_alg_free_sg(&ctx->sgl); } + ctx->more = msg->msg_flags & MSG_MORE; + err = 0; unlock: release_sock(sk); + return copied ?: err; - return err ?: copied; +unlock_free: + af_alg_free_sg(&ctx->sgl); + goto unlock; } static ssize_t hash_sendpage(struct socket *sock, struct page *page,
Make AF_ALG sendmsg() support MSG_SPLICE_PAGES in the hashing code. This causes pages to be spliced from the source iterator if possible. This allows ->sendpage() to be replaced by something that can handle multiple multipage folios in a single transaction. Signed-off-by: David Howells <dhowells@redhat.com> cc: Herbert Xu <herbert@gondor.apana.org.au> cc: "David S. Miller" <davem@davemloft.net> cc: Eric Dumazet <edumazet@google.com> cc: Jakub Kicinski <kuba@kernel.org> cc: Paolo Abeni <pabeni@redhat.com> cc: Jens Axboe <axboe@kernel.dk> cc: Matthew Wilcox <willy@infradead.org> cc: linux-crypto@vger.kernel.org cc: netdev@vger.kernel.org --- Notes: ver #2) - Fixed some checkpatch warnings. crypto/af_alg.c | 11 +++-- crypto/algif_hash.c | 104 ++++++++++++++++++++++++++++---------------- 2 files changed, 74 insertions(+), 41 deletions(-)