[PATCH 0/2] crypto: AF_ALG memory management fix

[PATCH 0/2] crypto: AF_ALG memory management fix

[Stephan Müller]

Hi Herbert,

please find attached memory management updates to

- simplify the code: the old AIO memory management is very
  complex and seemingly very fragile -- the update now
  eliminates all reported bugs in the skcipher and AEAD
  interfaces which allowed the kernel to be crashed by
  an unprivileged user

- streamline the code: there is one code path for AIO and sync
  operation; the code between algif_skcipher and algif_aead
  is very similar (if that patch set is accepted, I volunteer
  to reduce code duplication by moving service operations
  into af_alg.c and to further unify the TX SGL handling)

- unify the AIO and sync operation which only differ in the
  kernel crypto API callback and whether to wait for the
  crypto operation or not

- fix all reported bugs regarding the handling of multiple
  IOCBs.

The following testing was performed:

- stress testing to verify that no memleaks exist

- testing using Tadeusz Struck AIO test tool (see
  https://github.com/tstruk/afalg_async_test) -- the AEAD test
  is not applicable any more due to the changed user space
  interface; the skcipher test works once the user space
  interface change is honored in the test code

- using the libkcapi test suite, all tests including the
  originally failing ones (AIO with multiple IOCBs) work now --
  the current libkcapi code artificially limits the AEAD
  operation to one IOCB. After altering the libkcapi code
  to allow multiple IOCBs, the testing works flawless.

Stephan Mueller (2):
  crypto: aead AF_ALG - overhaul memory management
  crypto: skcipher AF_ALG - overhaul memory management

 crypto/algif_aead.c     | 439 ++++++++++++++++++++
+---------------------------
 crypto/algif_skcipher.c | 438 +++++++++++++++++++----------------------------
 2 files changed, 369 insertions(+), 508 deletions(-)

-- 
2.9.3

[PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Stephan Müller]

The updated memory management is described in the top part of the code.
As one benefit of the changed memory management, the AIO and synchronous
operation is now implemented in one common function. The AF_ALG
operation uses the async kernel crypto API interface for each cipher
operation. Thus, the only difference between the AIO and sync operation
types visible from user space is:

1. the callback function to be invoked when the asynchronous operation
   is completed

2. whether to wait for the completion of the kernel crypto API operation
   or not

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/algif_aead.c | 439 +++++++++++++++++++++++-----------------------------
 1 file changed, 195 insertions(+), 244 deletions(-)

diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index f849311..3ca68fb 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -11,6 +11,25 @@
  * under the terms of the GNU General Public License as published by the Free
  * Software Foundation; either version 2 of the License, or (at your option)
  * any later version.
+ *
+ * The following concept of the memory management is used:
+ *
+ * The kernel maintains two SGLs, the TX SGL and the RX SGL. The TX SGL is
+ * filled by user space with the data submitted via sendpage/sendmsg. Filling
+ * up the TX SGL does not cause a crypto operation -- the data will only be
+ * tracked by the kernel. Upon receipt of one recvmsg call, the caller must
+ * provide a buffer which is tracked with the RX SGL.
+ *
+ * During the processing of the recvmsg operation, the cipher request is
+ * allocated and prepared. To support multiple recvmsg operations operating
+ * on one TX SGL, an offset pointer into the TX SGL is maintained. The TX SGL
+ * that is used for the crypto request is scatterwalk_ffwd by the offset
+ * pointer to obtain the start address the crypto operation shall use for
+ * the input data.
+ *
+ * After the completion of the crypto operation, the RX SGL and the cipher
+ * request is released. The TX SGL is released once all parts of it are
+ * processed.
  */
 
 #include <crypto/internal/aead.h>
@@ -24,7 +43,7 @@
 #include <linux/net.h>
 #include <net/sock.h>
 
-struct aead_sg_list {
+struct aead_async_tsgl {
 	unsigned int cur;
 	struct scatterlist sg[ALG_MAX_PAGES];
 };
@@ -35,34 +54,38 @@ struct aead_async_rsgl {
 };
 
 struct aead_async_req {
-	struct scatterlist *tsgl;
-	struct aead_async_rsgl first_rsgl;
-	struct list_head list;
 	struct kiocb *iocb;
-	unsigned int tsgls;
-	char iv[];
+	struct sock *sk;
+
+	struct aead_async_rsgl first_rsgl;	/* First RX SG */
+	struct list_head list;			/* Track RX SGs */
+
+	unsigned int areqlen;			/* Length of this data struct */
+	struct aead_request aead_req;		/* req ctx trails this struct */
 };
 
 struct aead_ctx {
-	struct aead_sg_list tsgl;
-	struct aead_async_rsgl first_rsgl;
-	struct list_head list;
+	struct aead_async_tsgl tsgl;
 
 	void *iv;
+	size_t aead_assoclen;
 
-	struct af_alg_completion completion;
+	struct af_alg_completion completion;	/* sync work queue */
 
-	unsigned long used;
+	unsigned int inflight;			/* outstanding AIO ops */
+	size_t used;				/* TX bytes sent to kernel */
+	size_t processed;			/* number of bytes processed */
 
-	unsigned int len;
 	bool more;
 	bool merge;
 	bool enc;
 
-	size_t aead_assoclen;
-	struct aead_request aead_req;
+	unsigned int len;			/* length data structure */
+	struct crypto_aead *aead_tfm;
 };
 
+static DECLARE_WAIT_QUEUE_HEAD(aead_aio_finish_wait);
+
 static inline int aead_sndbuf(struct sock *sk)
 {
 	struct alg_sock *ask = alg_sk(sk);
@@ -79,34 +102,43 @@ static inline bool aead_writable(struct sock *sk)
 
 static inline bool aead_sufficient_data(struct aead_ctx *ctx)
 {
-	unsigned as = crypto_aead_authsize(crypto_aead_reqtfm(&ctx->aead_req));
+	unsigned as = crypto_aead_authsize(ctx->aead_tfm);
 
 	/*
 	 * The minimum amount of memory needed for an AEAD cipher is
 	 * the AAD and in case of decryption the tag.
+	 *
+	 * Also, sufficient data must be available after disregarding the
+	 * already processed data.
 	 */
-	return ctx->used >= ctx->aead_assoclen + (ctx->enc ? 0 : as);
+	return ctx->used >= ctx->aead_assoclen + (ctx->enc ? 0 : as) +
+			    ctx->processed;
 }
 
 static void aead_reset_ctx(struct aead_ctx *ctx)
 {
-	struct aead_sg_list *sgl = &ctx->tsgl;
+	struct aead_async_tsgl *sgl = &ctx->tsgl;
 
 	sg_init_table(sgl->sg, ALG_MAX_PAGES);
 	sgl->cur = 0;
 	ctx->used = 0;
+	ctx->processed = 0;
 	ctx->more = 0;
 	ctx->merge = 0;
 }
 
-static void aead_put_sgl(struct sock *sk)
+static void aead_pull_tsgl(struct sock *sk, bool force)
 {
 	struct alg_sock *ask = alg_sk(sk);
 	struct aead_ctx *ctx = ask->private;
-	struct aead_sg_list *sgl = &ctx->tsgl;
+	struct aead_async_tsgl *sgl = &ctx->tsgl;
 	struct scatterlist *sg = sgl->sg;
 	unsigned int i;
 
+	/* leave provided data in tact if we did not finish AIO work */
+	if (!force && ctx->used > ctx->processed)
+		return;
+
 	for (i = 0; i < sgl->cur; i++) {
 		if (!sg_page(sg + i))
 			continue;
@@ -117,6 +149,19 @@ static void aead_put_sgl(struct sock *sk)
 	aead_reset_ctx(ctx);
 }
 
+static void aead_free_rsgl(struct aead_async_req *areq)
+{
+	struct sock *sk = areq->sk;
+	struct aead_async_rsgl *rsgl, *tmp;
+
+	list_for_each_entry_safe(rsgl, tmp, &areq->list, list) {
+		af_alg_free_sg(&rsgl->sgl);
+		if (rsgl != &areq->first_rsgl)
+			sock_kfree_s(sk, rsgl, sizeof(*rsgl));
+		list_del(&rsgl->list);
+	}
+}
+
 static void aead_wmem_wakeup(struct sock *sk)
 {
 	struct socket_wq *wq;
@@ -189,9 +234,8 @@ static int aead_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 	struct sock *sk = sock->sk;
 	struct alg_sock *ask = alg_sk(sk);
 	struct aead_ctx *ctx = ask->private;
-	unsigned ivsize =
-		crypto_aead_ivsize(crypto_aead_reqtfm(&ctx->aead_req));
-	struct aead_sg_list *sgl = &ctx->tsgl;
+	unsigned ivsize = crypto_aead_ivsize(ctx->aead_tfm);
+	struct aead_async_tsgl *sgl = &ctx->tsgl;
 	struct af_alg_control con = {};
 	long copied = 0;
 	bool enc = 0;
@@ -258,7 +302,7 @@ static int aead_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 
 		if (!aead_writable(sk)) {
 			/* user space sent too much data */
-			aead_put_sgl(sk);
+			aead_pull_tsgl(sk, 1);
 			err = -EMSGSIZE;
 			goto unlock;
 		}
@@ -269,7 +313,7 @@ static int aead_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 			size_t plen = 0;
 
 			if (sgl->cur >= ALG_MAX_PAGES) {
-				aead_put_sgl(sk);
+				aead_pull_tsgl(sk, 1);
 				err = -E2BIG;
 				goto unlock;
 			}
@@ -305,7 +349,7 @@ static int aead_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 
 	ctx->more = msg->msg_flags & MSG_MORE;
 	if (!ctx->more && !aead_sufficient_data(ctx)) {
-		aead_put_sgl(sk);
+		aead_pull_tsgl(sk, 1);
 		err = -EMSGSIZE;
 	}
 
@@ -322,7 +366,7 @@ static ssize_t aead_sendpage(struct socket *sock, struct page *page,
 	struct sock *sk = sock->sk;
 	struct alg_sock *ask = alg_sk(sk);
 	struct aead_ctx *ctx = ask->private;
-	struct aead_sg_list *sgl = &ctx->tsgl;
+	struct aead_async_tsgl *sgl = &ctx->tsgl;
 	int err = -EINVAL;
 
 	if (flags & MSG_SENDPAGE_NOTLAST)
@@ -340,7 +384,7 @@ static ssize_t aead_sendpage(struct socket *sock, struct page *page,
 
 	if (!aead_writable(sk)) {
 		/* user space sent too much data */
-		aead_put_sgl(sk);
+		aead_pull_tsgl(sk, 1);
 		err = -EMSGSIZE;
 		goto unlock;
 	}
@@ -357,7 +401,7 @@ static ssize_t aead_sendpage(struct socket *sock, struct page *page,
 done:
 	ctx->more = flags & MSG_MORE;
 	if (!ctx->more && !aead_sufficient_data(ctx)) {
-		aead_put_sgl(sk);
+		aead_pull_tsgl(sk, 1);
 		err = -EMSGSIZE;
 	}
 
@@ -368,190 +412,48 @@ static ssize_t aead_sendpage(struct socket *sock, struct page *page,
 	return err ?: size;
 }
 
-#define GET_ASYM_REQ(req, tfm) (struct aead_async_req *) \
-		((char *)req + sizeof(struct aead_request) + \
-		 crypto_aead_reqsize(tfm))
-
- #define GET_REQ_SIZE(tfm) sizeof(struct aead_async_req) + \
-	crypto_aead_reqsize(tfm) + crypto_aead_ivsize(tfm) + \
-	sizeof(struct aead_request)
-
 static void aead_async_cb(struct crypto_async_request *_req, int err)
 {
-	struct sock *sk = _req->data;
+	struct aead_async_req *areq = _req->data;
+	struct sock *sk = areq->sk;
 	struct alg_sock *ask = alg_sk(sk);
 	struct aead_ctx *ctx = ask->private;
-	struct crypto_aead *tfm = crypto_aead_reqtfm(&ctx->aead_req);
-	struct aead_request *req = aead_request_cast(_req);
-	struct aead_async_req *areq = GET_ASYM_REQ(req, tfm);
-	struct scatterlist *sg = areq->tsgl;
-	struct aead_async_rsgl *rsgl;
 	struct kiocb *iocb = areq->iocb;
-	unsigned int i, reqlen = GET_REQ_SIZE(tfm);
 
-	list_for_each_entry(rsgl, &areq->list, list) {
-		af_alg_free_sg(&rsgl->sgl);
-		if (rsgl != &areq->first_rsgl)
-			sock_kfree_s(sk, rsgl, sizeof(*rsgl));
-	}
+	lock_sock(sk);
 
-	for (i = 0; i < areq->tsgls; i++)
-		put_page(sg_page(sg + i));
+	BUG_ON(!ctx->inflight);
 
-	sock_kfree_s(sk, areq->tsgl, sizeof(*areq->tsgl) * areq->tsgls);
-	sock_kfree_s(sk, req, reqlen);
+	aead_free_rsgl(areq);
+	sock_kfree_s(sk, areq, areq->areqlen);
+	aead_pull_tsgl(sk, 0);
 	__sock_put(sk);
+	ctx->inflight--;
 	iocb->ki_complete(iocb, err, err);
-}
-
-static int aead_recvmsg_async(struct socket *sock, struct msghdr *msg,
-			      int flags)
-{
-	struct sock *sk = sock->sk;
-	struct alg_sock *ask = alg_sk(sk);
-	struct aead_ctx *ctx = ask->private;
-	struct crypto_aead *tfm = crypto_aead_reqtfm(&ctx->aead_req);
-	struct aead_async_req *areq;
-	struct aead_request *req = NULL;
-	struct aead_sg_list *sgl = &ctx->tsgl;
-	struct aead_async_rsgl *last_rsgl = NULL, *rsgl;
-	unsigned int as = crypto_aead_authsize(tfm);
-	unsigned int i, reqlen = GET_REQ_SIZE(tfm);
-	int err = -ENOMEM;
-	unsigned long used;
-	size_t outlen = 0;
-	size_t usedpages = 0;
 
-	lock_sock(sk);
-	if (ctx->more) {
-		err = aead_wait_for_data(sk, flags);
-		if (err)
-			goto unlock;
-	}
-
-	if (!aead_sufficient_data(ctx))
-		goto unlock;
-
-	used = ctx->used;
-	if (ctx->enc)
-		outlen = used + as;
-	else
-		outlen = used - as;
-
-	req = sock_kmalloc(sk, reqlen, GFP_KERNEL);
-	if (unlikely(!req))
-		goto unlock;
-
-	areq = GET_ASYM_REQ(req, tfm);
-	memset(&areq->first_rsgl, '\0', sizeof(areq->first_rsgl));
-	INIT_LIST_HEAD(&areq->list);
-	areq->iocb = msg->msg_iocb;
-	memcpy(areq->iv, ctx->iv, crypto_aead_ivsize(tfm));
-	aead_request_set_tfm(req, tfm);
-	aead_request_set_ad(req, ctx->aead_assoclen);
-	aead_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
-				  aead_async_cb, sk);
-	used -= ctx->aead_assoclen;
-
-	/* take over all tx sgls from ctx */
-	areq->tsgl = sock_kmalloc(sk,
-				  sizeof(*areq->tsgl) * max_t(u32, sgl->cur, 1),
-				  GFP_KERNEL);
-	if (unlikely(!areq->tsgl))
-		goto free;
-
-	sg_init_table(areq->tsgl, max_t(u32, sgl->cur, 1));
-	for (i = 0; i < sgl->cur; i++)
-		sg_set_page(&areq->tsgl[i], sg_page(&sgl->sg[i]),
-			    sgl->sg[i].length, sgl->sg[i].offset);
-
-	areq->tsgls = sgl->cur;
-
-	/* create rx sgls */
-	while (outlen > usedpages && iov_iter_count(&msg->msg_iter)) {
-		size_t seglen = min_t(size_t, iov_iter_count(&msg->msg_iter),
-				      (outlen - usedpages));
-
-		if (list_empty(&areq->list)) {
-			rsgl = &areq->first_rsgl;
-
-		} else {
-			rsgl = sock_kmalloc(sk, sizeof(*rsgl), GFP_KERNEL);
-			if (unlikely(!rsgl)) {
-				err = -ENOMEM;
-				goto free;
-			}
-		}
-		rsgl->sgl.npages = 0;
-		list_add_tail(&rsgl->list, &areq->list);
-
-		/* make one iovec available as scatterlist */
-		err = af_alg_make_sg(&rsgl->sgl, &msg->msg_iter, seglen);
-		if (err < 0)
-			goto free;
-
-		usedpages += err;
-
-		/* chain the new scatterlist with previous one */
-		if (last_rsgl)
-			af_alg_link_sg(&last_rsgl->sgl, &rsgl->sgl);
-
-		last_rsgl = rsgl;
-
-		iov_iter_advance(&msg->msg_iter, err);
-	}
-
-	/* ensure output buffer is sufficiently large */
-	if (usedpages < outlen) {
-		err = -EINVAL;
-		goto unlock;
-	}
-
-	aead_request_set_crypt(req, areq->tsgl, areq->first_rsgl.sgl.sg, used,
-			       areq->iv);
-	err = ctx->enc ? crypto_aead_encrypt(req) : crypto_aead_decrypt(req);
-	if (err) {
-		if (err == -EINPROGRESS) {
-			sock_hold(sk);
-			err = -EIOCBQUEUED;
-			aead_reset_ctx(ctx);
-			goto unlock;
-		} else if (err == -EBADMSG) {
-			aead_put_sgl(sk);
-		}
-		goto free;
-	}
-	aead_put_sgl(sk);
-
-free:
-	list_for_each_entry(rsgl, &areq->list, list) {
-		af_alg_free_sg(&rsgl->sgl);
-		if (rsgl != &areq->first_rsgl)
-			sock_kfree_s(sk, rsgl, sizeof(*rsgl));
-	}
-	if (areq->tsgl)
-		sock_kfree_s(sk, areq->tsgl, sizeof(*areq->tsgl) * areq->tsgls);
-	if (req)
-		sock_kfree_s(sk, req, reqlen);
-unlock:
-	aead_wmem_wakeup(sk);
 	release_sock(sk);
-	return err ? err : outlen;
+
+	wake_up_interruptible(&aead_aio_finish_wait);
 }
 
-static int aead_recvmsg_sync(struct socket *sock, struct msghdr *msg, int flags)
+static int aead_recvmsg(struct socket *sock, struct msghdr *msg, size_t ignored,
+			int flags)
 {
 	struct sock *sk = sock->sk;
 	struct alg_sock *ask = alg_sk(sk);
 	struct aead_ctx *ctx = ask->private;
-	unsigned as = crypto_aead_authsize(crypto_aead_reqtfm(&ctx->aead_req));
-	struct aead_sg_list *sgl = &ctx->tsgl;
+	struct crypto_aead *tfm = ctx->aead_tfm;
+	unsigned int as = crypto_aead_authsize(tfm);
+	unsigned int areqlen =
+		sizeof(struct aead_async_req) + crypto_aead_reqsize(tfm);
+	struct aead_async_tsgl *tsgl = &ctx->tsgl;
+	struct aead_async_req *areq;
 	struct aead_async_rsgl *last_rsgl = NULL;
-	struct aead_async_rsgl *rsgl, *tmp;
+	struct scatterlist tsgl_head[2], *tsgl_head_p;
 	int err = -EINVAL;
-	unsigned long used = 0;
-	size_t outlen = 0;
-	size_t usedpages = 0;
+	size_t used = 0;		/* [in]  TX bufs to be en/decrypted */
+	size_t outlen = 0;		/* [out] RX bufs produced by kernel */
+	size_t usedpages = 0;		/* [in]  RX bufs to be used from user */
 
 	lock_sock(sk);
 
@@ -566,8 +468,11 @@ static int aead_recvmsg_sync(struct socket *sock, struct msghdr *msg, int flags)
 			goto unlock;
 	}
 
-	/* data length provided by caller via sendmsg/sendpage */
-	used = ctx->used;
+	/*
+	 * Data length provided by caller via sendmsg/sendpage that has not
+	 * yet been processed.
+	 */
+	used = ctx->used - ctx->processed;
 
 	/*
 	 * Make sure sufficient data is present -- note, the same check is
@@ -600,86 +505,131 @@ static int aead_recvmsg_sync(struct socket *sock, struct msghdr *msg, int flags)
 	 */
 	used -= ctx->aead_assoclen;
 
-	/* convert iovecs of output buffers into scatterlists */
+	/* Allocate cipher request for current operation. */
+	areq = sock_kmalloc(sk, areqlen, GFP_KERNEL);
+	if (unlikely(!areq)) {
+		err = -ENOMEM;
+		goto unlock;
+	}
+	areq->areqlen = areqlen;
+	areq->sk = sk;
+	INIT_LIST_HEAD(&areq->list);
+
+	/* convert iovecs of output buffers into RX SGL */
 	while (outlen > usedpages && iov_iter_count(&msg->msg_iter)) {
+		struct aead_async_rsgl *rsgl;
 		size_t seglen = min_t(size_t, iov_iter_count(&msg->msg_iter),
 				      (outlen - usedpages));
 
-		if (list_empty(&ctx->list)) {
-			rsgl = &ctx->first_rsgl;
+		if (list_empty(&areq->list)) {
+			rsgl = &areq->first_rsgl;
 		} else {
 			rsgl = sock_kmalloc(sk, sizeof(*rsgl), GFP_KERNEL);
 			if (unlikely(!rsgl)) {
 				err = -ENOMEM;
-				goto unlock;
+				goto free;
 			}
 		}
+
 		rsgl->sgl.npages = 0;
-		list_add_tail(&rsgl->list, &ctx->list);
+		list_add_tail(&rsgl->list, &areq->list);
 
 		/* make one iovec available as scatterlist */
 		err = af_alg_make_sg(&rsgl->sgl, &msg->msg_iter, seglen);
 		if (err < 0)
-			goto unlock;
-		usedpages += err;
+			goto free;
+
 		/* chain the new scatterlist with previous one */
 		if (last_rsgl)
 			af_alg_link_sg(&last_rsgl->sgl, &rsgl->sgl);
 
 		last_rsgl = rsgl;
-
+		usedpages += err;
 		iov_iter_advance(&msg->msg_iter, err);
 	}
 
-	/* ensure output buffer is sufficiently large */
+	/*
+	 * Ensure output buffer is sufficiently large. If the caller provides
+	 * less buffer space, only use the relative required input size. This
+	 * allows AIO operation where the caller sent all data to be processed
+	 * and the AIO operation performs the operation on the different chunks
+	 * of the input data.
+	 */
 	if (usedpages < outlen) {
-		err = -EINVAL;
-		goto unlock;
-	}
+		size_t less = outlen - usedpages;
 
-	sg_mark_end(sgl->sg + sgl->cur - 1);
-	aead_request_set_crypt(&ctx->aead_req, sgl->sg, ctx->first_rsgl.sgl.sg,
-			       used, ctx->iv);
-	aead_request_set_ad(&ctx->aead_req, ctx->aead_assoclen);
+		if (used < less) {
+			err = -EINVAL;
+			goto free;
+		}
+		used -= less;
+		outlen -= less;
+	}
 
-	err = af_alg_wait_for_completion(ctx->enc ?
-					 crypto_aead_encrypt(&ctx->aead_req) :
-					 crypto_aead_decrypt(&ctx->aead_req),
+	sg_mark_end(tsgl->sg + tsgl->cur - 1);
+
+	/* Get the head of the SGL we want to process */
+	tsgl_head_p = scatterwalk_ffwd(tsgl_head, tsgl->sg, ctx->processed);
+	BUG_ON(!tsgl_head_p);
+
+	/* Initialize the crypto operation */
+	aead_request_set_crypt(&areq->aead_req, tsgl_head_p,
+			       areq->first_rsgl.sgl.sg, used, ctx->iv);
+	aead_request_set_ad(&areq->aead_req, ctx->aead_assoclen);
+	aead_request_set_tfm(&areq->aead_req, tfm);
+
+	if (msg->msg_iocb && !is_sync_kiocb(msg->msg_iocb)) {
+		/* AIO operation */
+		areq->iocb = msg->msg_iocb;
+		aead_request_set_callback(&areq->aead_req,
+					  CRYPTO_TFM_REQ_MAY_BACKLOG,
+					  aead_async_cb, areq);
+		err = ctx->enc ? crypto_aead_encrypt(&areq->aead_req) :
+				 crypto_aead_decrypt(&areq->aead_req);
+	} else {
+		/* Synchronous operation */
+		aead_request_set_callback(&areq->aead_req,
+					  CRYPTO_TFM_REQ_MAY_BACKLOG,
+					  af_alg_complete, &ctx->completion);
+		err = af_alg_wait_for_completion(ctx->enc ?
+					 crypto_aead_encrypt(&areq->aead_req) :
+					 crypto_aead_decrypt(&areq->aead_req),
 					 &ctx->completion);
+	}
 
 	if (err) {
-		/* EBADMSG implies a valid cipher operation took place */
-		if (err == -EBADMSG)
-			aead_put_sgl(sk);
+		/* AIO operation in progress */
+		if (err == -EINPROGRESS) {
+			sock_hold(sk);
+			err = -EIOCBQUEUED;
 
-		goto unlock;
+			/* Remember the TX bytes that were processed. */
+			ctx->processed += ctx->enc ? (outlen - as) :
+						     (outlen + as);
+			ctx->inflight++;
+
+			goto unlock;
+		}
+		/* EBADMSG implies a valid cipher operation took place */
+		else if (err != -EBADMSG)
+			goto free;
 	}
 
-	aead_put_sgl(sk);
-	err = 0;
+	/* Remember the TX bytes that were processed. */
+	ctx->processed += ctx->enc ? (outlen - as) : (outlen + as);
+
+free:
+	aead_free_rsgl(areq);
+	if (areq)
+		sock_kfree_s(sk, areq, areqlen);
+	aead_pull_tsgl(sk, 0);
 
 unlock:
-	list_for_each_entry_safe(rsgl, tmp, &ctx->list, list) {
-		af_alg_free_sg(&rsgl->sgl);
-		if (rsgl != &ctx->first_rsgl)
-			sock_kfree_s(sk, rsgl, sizeof(*rsgl));
-		list_del(&rsgl->list);
-	}
-	INIT_LIST_HEAD(&ctx->list);
 	aead_wmem_wakeup(sk);
 	release_sock(sk);
-
 	return err ? err : outlen;
 }
 
-static int aead_recvmsg(struct socket *sock, struct msghdr *msg, size_t ignored,
-			int flags)
-{
-	return (msg->msg_iocb && !is_sync_kiocb(msg->msg_iocb)) ?
-		aead_recvmsg_async(sock, msg, flags) :
-		aead_recvmsg_sync(sock, msg, flags);
-}
-
 static unsigned int aead_poll(struct file *file, struct socket *sock,
 			      poll_table *wait)
 {
@@ -746,11 +696,14 @@ static void aead_sock_destruct(struct sock *sk)
 {
 	struct alg_sock *ask = alg_sk(sk);
 	struct aead_ctx *ctx = ask->private;
-	unsigned int ivlen = crypto_aead_ivsize(
-				crypto_aead_reqtfm(&ctx->aead_req));
+	unsigned int ivlen = crypto_aead_ivsize(ctx->aead_tfm);
 
 	WARN_ON(atomic_read(&sk->sk_refcnt) != 0);
-	aead_put_sgl(sk);
+
+	/* Suspend caller if AIO operations are in flight. */
+	wait_event_interruptible(aead_aio_finish_wait, (ctx->inflight == 0));
+
+	aead_pull_tsgl(sk, 1);
 	sock_kzfree_s(sk, ctx->iv, ivlen);
 	sock_kfree_s(sk, ctx, ctx->len);
 	af_alg_release_parent(sk);
@@ -760,7 +713,7 @@ static int aead_accept_parent(void *private, struct sock *sk)
 {
 	struct aead_ctx *ctx;
 	struct alg_sock *ask = alg_sk(sk);
-	unsigned int len = sizeof(*ctx) + crypto_aead_reqsize(private);
+	unsigned int len = sizeof(*ctx);
 	unsigned int ivlen = crypto_aead_ivsize(private);
 
 	ctx = sock_kmalloc(sk, len, GFP_KERNEL);
@@ -777,20 +730,18 @@ static int aead_accept_parent(void *private, struct sock *sk)
 
 	ctx->len = len;
 	ctx->used = 0;
+	ctx->processed = 0;
 	ctx->more = 0;
 	ctx->merge = 0;
 	ctx->enc = 0;
+	ctx->inflight = 0;
 	ctx->tsgl.cur = 0;
 	ctx->aead_assoclen = 0;
 	af_alg_init_completion(&ctx->completion);
 	sg_init_table(ctx->tsgl.sg, ALG_MAX_PAGES);
-	INIT_LIST_HEAD(&ctx->list);
 
 	ask->private = ctx;
-
-	aead_request_set_tfm(&ctx->aead_req, private);
-	aead_request_set_callback(&ctx->aead_req, CRYPTO_TFM_REQ_MAY_BACKLOG,
-				  af_alg_complete, &ctx->completion);
+	ctx->aead_tfm = private;
 
 	sk->sk_destruct = aead_sock_destruct;
 
-- 
2.9.3

[PATCH 2/2] crypto: skcipher AF_ALG - overhaul memory management

[Stephan Müller]

The updated memory management is described in the top part of the code.
As one benefit of the changed memory management, the AIO and synchronous
operation is now implemented in one common function. The AF_ALG
operation uses the async kernel crypto API interface for each cipher
operation. Thus, the only difference between the AIO and sync operation
types visible from user space is:

1. the callback function to be invoked when the asynchronous operation
   is completed

2. whether to wait for the completion of the kernel crypto API operation
   or not

In addition, the code structure is adjusted to match the structure of
algif_aead for easier code assessment.

The user space interface changed slightly as follows: the old AIO
operation returned zero upon success and < 0 in case of an error to user
space. As all other AF_ALG interfaces (including the sync skcipher
interface) returned the number of processed bytes upon success and < 0
in case of an error, the new skcipher interface (regardless of AIO or
sync) returns the number of processed bytes in case of success.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/algif_skcipher.c | 438 +++++++++++++++++++-----------------------------
 1 file changed, 174 insertions(+), 264 deletions(-)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index a9e79d8..437e60a 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -10,6 +10,24 @@
  * Software Foundation; either version 2 of the License, or (at your option)
  * any later version.
  *
+ * The following concept of the memory management is used:
+ *
+ * The kernel maintains two SGLs, the TX SGL and the RX SGL. The TX SGL is
+ * filled by user space with the data submitted via sendpage/sendmsg. Filling
+ * up the TX SGL does not cause a crypto operation -- the data will only be
+ * tracked by the kernel. Upon receipt of one recvmsg call, the caller must
+ * provide a buffer which is tracked with the RX SGL.
+ *
+ * During the processing of the recvmsg operation, the cipher request is
+ * allocated and prepared. To support multiple recvmsg operations operating
+ * on one TX SGL, an offset pointer into the TX SGL is maintained. The TX SGL
+ * that is used for the crypto request is scatterwalk_ffwd by the offset
+ * pointer to obtain the start address the crypto operation shall use for
+ * the input data.
+ *
+ * After the completion of the crypto operation, the RX SGL and the cipher
+ * request is released. The TX SGL is released once all parts of it are
+ * processed.
  */
 
 #include <crypto/scatterwalk.h>
@@ -31,6 +49,22 @@ struct skcipher_sg_list {
 	struct scatterlist sg[0];
 };
 
+struct skcipher_async_rsgl {
+	struct af_alg_sgl sgl;
+	struct list_head list;
+};
+
+struct skcipher_async_req {
+	struct kiocb *iocb;
+	struct sock *sk;
+
+	struct skcipher_async_rsgl first_sgl;
+	struct list_head list;
+
+	unsigned int areqlen;
+	struct skcipher_request req;
+};
+
 struct skcipher_tfm {
 	struct crypto_skcipher *skcipher;
 	bool has_key;
@@ -44,65 +78,22 @@ struct skcipher_ctx {
 
 	struct af_alg_completion completion;
 
-	atomic_t inflight;
+	unsigned int inflight;
 	size_t used;
+	size_t processed;
 
-	unsigned int len;
 	bool more;
 	bool merge;
 	bool enc;
 
-	struct skcipher_request req;
-};
-
-struct skcipher_async_rsgl {
-	struct af_alg_sgl sgl;
-	struct list_head list;
+	unsigned int len;
 };
 
-struct skcipher_async_req {
-	struct kiocb *iocb;
-	struct skcipher_async_rsgl first_sgl;
-	struct list_head list;
-	struct scatterlist *tsg;
-	atomic_t *inflight;
-	struct skcipher_request req;
-};
+static DECLARE_WAIT_QUEUE_HEAD(skcipher_aio_finish_wait);
 
 #define MAX_SGL_ENTS ((4096 - sizeof(struct skcipher_sg_list)) / \
 		      sizeof(struct scatterlist) - 1)
 
-static void skcipher_free_async_sgls(struct skcipher_async_req *sreq)
-{
-	struct skcipher_async_rsgl *rsgl, *tmp;
-	struct scatterlist *sgl;
-	struct scatterlist *sg;
-	int i, n;
-
-	list_for_each_entry_safe(rsgl, tmp, &sreq->list, list) {
-		af_alg_free_sg(&rsgl->sgl);
-		if (rsgl != &sreq->first_sgl)
-			kfree(rsgl);
-	}
-	sgl = sreq->tsg;
-	n = sg_nents(sgl);
-	for_each_sg(sgl, sg, n, i)
-		put_page(sg_page(sg));
-
-	kfree(sreq->tsg);
-}
-
-static void skcipher_async_cb(struct crypto_async_request *req, int err)
-{
-	struct skcipher_async_req *sreq = req->data;
-	struct kiocb *iocb = sreq->iocb;
-
-	atomic_dec(sreq->inflight);
-	skcipher_free_async_sgls(sreq);
-	kzfree(sreq);
-	iocb->ki_complete(iocb, err, err);
-}
-
 static inline int skcipher_sndbuf(struct sock *sk)
 {
 	struct alg_sock *ask = alg_sk(sk);
@@ -117,7 +108,7 @@ static inline bool skcipher_writable(struct sock *sk)
 	return PAGE_SIZE <= skcipher_sndbuf(sk);
 }
 
-static int skcipher_alloc_sgl(struct sock *sk)
+static int skcipher_alloc_tsgl(struct sock *sk)
 {
 	struct alg_sock *ask = alg_sk(sk);
 	struct skcipher_ctx *ctx = ask->private;
@@ -147,7 +138,7 @@ static int skcipher_alloc_sgl(struct sock *sk)
 	return 0;
 }
 
-static void skcipher_pull_sgl(struct sock *sk, size_t used, int put)
+static void skcipher_pull_tsgl(struct sock *sk, size_t used)
 {
 	struct alg_sock *ask = alg_sk(sk);
 	struct skcipher_ctx *ctx = ask->private;
@@ -171,30 +162,35 @@ static void skcipher_pull_sgl(struct sock *sk, size_t used, int put)
 
 			used -= plen;
 			ctx->used -= plen;
+			ctx->processed -= plen;
 
 			if (sg[i].length)
 				return;
-			if (put)
-				put_page(sg_page(sg + i));
+
+			put_page(sg_page(sg + i));
 			sg_assign_page(sg + i, NULL);
 		}
 
 		list_del(&sgl->list);
-		sock_kfree_s(sk, sgl,
-			     sizeof(*sgl) + sizeof(sgl->sg[0]) *
-					    (MAX_SGL_ENTS + 1));
+		sock_kfree_s(sk, sgl, sizeof(*sgl) + sizeof(sgl->sg[0]) *
+						     (MAX_SGL_ENTS + 1));
 	}
 
 	if (!ctx->used)
 		ctx->merge = 0;
 }
 
-static void skcipher_free_sgl(struct sock *sk)
+static void skcipher_free_rsgl(struct skcipher_async_req *areq)
 {
-	struct alg_sock *ask = alg_sk(sk);
-	struct skcipher_ctx *ctx = ask->private;
+	struct sock *sk = areq->sk;
+	struct skcipher_async_rsgl *rsgl, *tmp;
 
-	skcipher_pull_sgl(sk, ctx->used, 1);
+	list_for_each_entry_safe(rsgl, tmp, &areq->list, list) {
+		af_alg_free_sg(&rsgl->sgl);
+		if (rsgl != &areq->first_sgl)
+			sock_kfree_s(sk, rsgl, sizeof(*rsgl));
+		list_del(&rsgl->list);
+	}
 }
 
 static int skcipher_wait_for_wmem(struct sock *sk, unsigned flags)
@@ -378,7 +374,7 @@ static int skcipher_sendmsg(struct socket *sock, struct msghdr *msg,
 
 		len = min_t(unsigned long, len, skcipher_sndbuf(sk));
 
-		err = skcipher_alloc_sgl(sk);
+		err = skcipher_alloc_tsgl(sk);
 		if (err)
 			goto unlock;
 
@@ -453,7 +449,7 @@ static ssize_t skcipher_sendpage(struct socket *sock, struct page *page,
 			goto unlock;
 	}
 
-	err = skcipher_alloc_sgl(sk);
+	err = skcipher_alloc_tsgl(sk);
 	if (err)
 		goto unlock;
 
@@ -479,25 +475,33 @@ static ssize_t skcipher_sendpage(struct socket *sock, struct page *page,
 	return err ?: size;
 }
 
-static int skcipher_all_sg_nents(struct skcipher_ctx *ctx)
+static void skcipher_async_cb(struct crypto_async_request *req, int err)
 {
-	struct skcipher_sg_list *sgl;
-	struct scatterlist *sg;
-	int nents = 0;
+	struct skcipher_async_req *areq = req->data;
+	struct sock *sk = areq->sk;
+	struct alg_sock *ask = alg_sk(sk);
+	struct skcipher_ctx *ctx = ask->private;
+	struct kiocb *iocb = areq->iocb;
 
-	list_for_each_entry(sgl, &ctx->tsgl, list) {
-		sg = sgl->sg;
+	lock_sock(sk);
 
-		while (!sg->length)
-			sg++;
+	BUG_ON(!ctx->inflight);
 
-		nents += sg_nents(sg);
-	}
-	return nents;
+	skcipher_free_rsgl(areq);
+	sock_kfree_s(sk, areq, areq->areqlen);
+	skcipher_pull_tsgl(sk, areq->req.cryptlen);
+	__sock_put(sk);
+	ctx->inflight--;
+
+	iocb->ki_complete(iocb, err, err);
+
+	release_sock(sk);
+
+	wake_up_interruptible(&skcipher_aio_finish_wait);
 }
 
-static int skcipher_recvmsg_async(struct socket *sock, struct msghdr *msg,
-				  int flags)
+static int skcipher_recvmsg(struct socket *sock, struct msghdr *msg,
+			    size_t ignored, int flags)
 {
 	struct sock *sk = sock->sk;
 	struct alg_sock *ask = alg_sk(sk);
@@ -506,215 +510,131 @@ static int skcipher_recvmsg_async(struct socket *sock, struct msghdr *msg,
 	struct skcipher_ctx *ctx = ask->private;
 	struct skcipher_tfm *skc = pask->private;
 	struct crypto_skcipher *tfm = skc->skcipher;
-	struct skcipher_sg_list *sgl;
-	struct scatterlist *sg;
-	struct skcipher_async_req *sreq;
-	struct skcipher_request *req;
+	unsigned bs = crypto_skcipher_blocksize(tfm);
+	unsigned int areqlen = sizeof(struct skcipher_async_req) +
+			       crypto_skcipher_reqsize(tfm);
+	struct skcipher_sg_list *tsgl;
+	struct skcipher_async_req *areq;
 	struct skcipher_async_rsgl *last_rsgl = NULL;
-	unsigned int txbufs = 0, len = 0, tx_nents;
-	unsigned int reqsize = crypto_skcipher_reqsize(tfm);
-	unsigned int ivsize = crypto_skcipher_ivsize(tfm);
+	struct scatterlist tsgl_head[2], *tsgl_head_p;
 	int err = -ENOMEM;
-	bool mark = false;
-	char *iv;
-
-	sreq = kzalloc(sizeof(*sreq) + reqsize + ivsize, GFP_KERNEL);
-	if (unlikely(!sreq))
-		goto out;
-
-	req = &sreq->req;
-	iv = (char *)(req + 1) + reqsize;
-	sreq->iocb = msg->msg_iocb;
-	INIT_LIST_HEAD(&sreq->list);
-	sreq->inflight = &ctx->inflight;
+	size_t len = 0;
 
 	lock_sock(sk);
-	tx_nents = skcipher_all_sg_nents(ctx);
-	sreq->tsg = kcalloc(tx_nents, sizeof(*sg), GFP_KERNEL);
-	if (unlikely(!sreq->tsg))
+
+	/* Allocate cipher request for current operation. */
+	areq = sock_kmalloc(sk, areqlen, GFP_KERNEL);
+	if (unlikely(!areq))
 		goto unlock;
-	sg_init_table(sreq->tsg, tx_nents);
-	memcpy(iv, ctx->iv, ivsize);
-	skcipher_request_set_tfm(req, tfm);
-	skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP,
-				      skcipher_async_cb, sreq);
+	areq->areqlen = areqlen;
+	areq->sk = sk;
+	INIT_LIST_HEAD(&areq->list);
 
-	while (iov_iter_count(&msg->msg_iter)) {
+ 	/* convert iovecs of output buffers into RX SGL */
+	while (len < ctx->used && iov_iter_count(&msg->msg_iter)) {
 		struct skcipher_async_rsgl *rsgl;
-		int used;
+		size_t seglen;
 
 		if (!ctx->used) {
 			err = skcipher_wait_for_data(sk, flags);
 			if (err)
 				goto free;
 		}
-		sgl = list_first_entry(&ctx->tsgl,
-				       struct skcipher_sg_list, list);
-		sg = sgl->sg;
 
-		while (!sg->length)
-			sg++;
-
-		used = min_t(unsigned long, ctx->used,
-			     iov_iter_count(&msg->msg_iter));
-		used = min_t(unsigned long, used, sg->length);
-
-		if (txbufs == tx_nents) {
-			struct scatterlist *tmp;
-			int x;
-			/* Ran out of tx slots in async request
-			 * need to expand */
-			tmp = kcalloc(tx_nents * 2, sizeof(*tmp),
-				      GFP_KERNEL);
-			if (!tmp) {
-				err = -ENOMEM;
-				goto free;
-			}
+		seglen = min_t(size_t, ctx->used,
+			       iov_iter_count(&msg->msg_iter));
 
-			sg_init_table(tmp, tx_nents * 2);
-			for (x = 0; x < tx_nents; x++)
-				sg_set_page(&tmp[x], sg_page(&sreq->tsg[x]),
-					    sreq->tsg[x].length,
-					    sreq->tsg[x].offset);
-			kfree(sreq->tsg);
-			sreq->tsg = tmp;
-			tx_nents *= 2;
-			mark = true;
-		}
-		/* Need to take over the tx sgl from ctx
-		 * to the asynch req - these sgls will be freed later */
-		sg_set_page(sreq->tsg + txbufs++, sg_page(sg), sg->length,
-			    sg->offset);
-
-		if (list_empty(&sreq->list)) {
-			rsgl = &sreq->first_sgl;
-			list_add_tail(&rsgl->list, &sreq->list);
+		if (list_empty(&areq->list)) {
+			rsgl = &areq->first_sgl;
 		} else {
-			rsgl = kmalloc(sizeof(*rsgl), GFP_KERNEL);
+			rsgl = sock_kmalloc(sk, sizeof(*rsgl), GFP_KERNEL);
 			if (!rsgl) {
 				err = -ENOMEM;
 				goto free;
 			}
-			list_add_tail(&rsgl->list, &sreq->list);
 		}
 
-		used = af_alg_make_sg(&rsgl->sgl, &msg->msg_iter, used);
-		err = used;
-		if (used < 0)
+		rsgl->sgl.npages = 0;
+		list_add_tail(&rsgl->list, &areq->list);
+
+		/* make one iovec available as scatterlist */
+		err = af_alg_make_sg(&rsgl->sgl, &msg->msg_iter, seglen);
+		if (err < 0)
 			goto free;
+
+		/* chain the new scatterlist with previous one */
 		if (last_rsgl)
 			af_alg_link_sg(&last_rsgl->sgl, &rsgl->sgl);
 
 		last_rsgl = rsgl;
-		len += used;
-		skcipher_pull_sgl(sk, used, 0);
-		iov_iter_advance(&msg->msg_iter, used);
+		len += err;
+		iov_iter_advance(&msg->msg_iter, err);
 	}
 
-	if (mark)
-		sg_mark_end(sreq->tsg + txbufs - 1);
+	/* Process only as much RX buffers for which we have TX data */
+	if (len > ctx->used)
+		len = ctx->used;
+
+	/*
+	 * If more buffers are to be expected to be processed, process only
+	 * full block size buffers.
+	 */
+	if (ctx->more || len < ctx->used)
+		len -= len % bs;
+
+	tsgl = list_first_entry(&ctx->tsgl, struct skcipher_sg_list, list);
+	/* Get the head of the SGL we want to process */
+	tsgl_head_p = scatterwalk_ffwd(tsgl_head, tsgl->sg, ctx->processed);
+	BUG_ON(!tsgl_head_p);
+
+	/* Initialize the crypto operation */
+	skcipher_request_set_tfm(&areq->req, tfm);
+	skcipher_request_set_crypt(&areq->req, tsgl_head_p,
+				   areq->first_sgl.sgl.sg, len, ctx->iv);
+
+	if (msg->msg_iocb && !is_sync_kiocb(msg->msg_iocb)) {
+		/* AIO operation */
+		areq->iocb = msg->msg_iocb;
+		skcipher_request_set_callback(&areq->req,
+					      CRYPTO_TFM_REQ_MAY_SLEEP,
+					      skcipher_async_cb, areq);
+		err = ctx->enc ? crypto_skcipher_encrypt(&areq->req) :
+				 crypto_skcipher_decrypt(&areq->req);
+	} else {
+		/* Synchronous operation */
+		skcipher_request_set_callback(&areq->req,
+					      CRYPTO_TFM_REQ_MAY_SLEEP |
+					      CRYPTO_TFM_REQ_MAY_BACKLOG,
+					      af_alg_complete,
+					      &ctx->completion);
+		err = af_alg_wait_for_completion(ctx->enc ?
+					crypto_skcipher_encrypt(&areq->req) :
+					crypto_skcipher_decrypt(&areq->req),
+						 &ctx->completion);
+	}
 
-	skcipher_request_set_crypt(req, sreq->tsg, sreq->first_sgl.sgl.sg,
-				   len, iv);
-	err = ctx->enc ? crypto_skcipher_encrypt(req) :
-			 crypto_skcipher_decrypt(req);
+	/* AIO operation in progress */
 	if (err == -EINPROGRESS) {
-		atomic_inc(&ctx->inflight);
+		sock_hold(sk);
 		err = -EIOCBQUEUED;
-		sreq = NULL;
+		ctx->inflight++;
+		/* Remember the TX bytes that were processed. */
+		ctx->processed += len;
 		goto unlock;
-	}
-free:
-	skcipher_free_async_sgls(sreq);
-unlock:
-	skcipher_wmem_wakeup(sk);
-	release_sock(sk);
-	kzfree(sreq);
-out:
-	return err;
-}
-
-static int skcipher_recvmsg_sync(struct socket *sock, struct msghdr *msg,
-				 int flags)
-{
-	struct sock *sk = sock->sk;
-	struct alg_sock *ask = alg_sk(sk);
-	struct sock *psk = ask->parent;
-	struct alg_sock *pask = alg_sk(psk);
-	struct skcipher_ctx *ctx = ask->private;
-	struct skcipher_tfm *skc = pask->private;
-	struct crypto_skcipher *tfm = skc->skcipher;
-	unsigned bs = crypto_skcipher_blocksize(tfm);
-	struct skcipher_sg_list *sgl;
-	struct scatterlist *sg;
-	int err = -EAGAIN;
-	int used;
-	long copied = 0;
-
-	lock_sock(sk);
-	while (msg_data_left(msg)) {
-		if (!ctx->used) {
-			err = skcipher_wait_for_data(sk, flags);
-			if (err)
-				goto unlock;
-		}
-
-		used = min_t(unsigned long, ctx->used, msg_data_left(msg));
-
-		used = af_alg_make_sg(&ctx->rsgl, &msg->msg_iter, used);
-		err = used;
-		if (err < 0)
-			goto unlock;
-
-		if (ctx->more || used < ctx->used)
-			used -= used % bs;
-
-		err = -EINVAL;
-		if (!used)
-			goto free;
-
-		sgl = list_first_entry(&ctx->tsgl,
-				       struct skcipher_sg_list, list);
-		sg = sgl->sg;
-
-		while (!sg->length)
-			sg++;
-
-		skcipher_request_set_crypt(&ctx->req, sg, ctx->rsgl.sg, used,
-					   ctx->iv);
-
-		err = af_alg_wait_for_completion(
-				ctx->enc ?
-					crypto_skcipher_encrypt(&ctx->req) :
-					crypto_skcipher_decrypt(&ctx->req),
-				&ctx->completion);
+	} else if (!err)
+		/* Remember the TX bytes that were processed. */
+		ctx->processed += len;
 
 free:
-		af_alg_free_sg(&ctx->rsgl);
-
-		if (err)
-			goto unlock;
-
-		copied += used;
-		skcipher_pull_sgl(sk, used, 1);
-		iov_iter_advance(&msg->msg_iter, used);
-	}
-
-	err = 0;
+	skcipher_free_rsgl(areq);
+	if (areq)
+		sock_kfree_s(sk, areq, areqlen);
+	skcipher_pull_tsgl(sk, len);
 
 unlock:
 	skcipher_wmem_wakeup(sk);
 	release_sock(sk);
-
-	return copied ?: err;
-}
-
-static int skcipher_recvmsg(struct socket *sock, struct msghdr *msg,
-			    size_t ignored, int flags)
-{
-	return (msg->msg_iocb && !is_sync_kiocb(msg->msg_iocb)) ?
-		skcipher_recvmsg_async(sock, msg, flags) :
-		skcipher_recvmsg_sync(sock, msg, flags);
+	return err ? err : len;
 }
 
 static unsigned int skcipher_poll(struct file *file, struct socket *sock,
@@ -894,26 +814,20 @@ static int skcipher_setkey(void *private, const u8 *key, unsigned int keylen)
 	return err;
 }
 
-static void skcipher_wait(struct sock *sk)
-{
-	struct alg_sock *ask = alg_sk(sk);
-	struct skcipher_ctx *ctx = ask->private;
-	int ctr = 0;
-
-	while (atomic_read(&ctx->inflight) && ctr++ < 100)
-		msleep(100);
-}
-
 static void skcipher_sock_destruct(struct sock *sk)
 {
 	struct alg_sock *ask = alg_sk(sk);
 	struct skcipher_ctx *ctx = ask->private;
-	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(&ctx->req);
+	struct sock *psk = ask->parent;
+	struct alg_sock *pask = alg_sk(psk);
+	struct skcipher_tfm *skc = pask->private;
+	struct crypto_skcipher *tfm = skc->skcipher;
 
-	if (atomic_read(&ctx->inflight))
-		skcipher_wait(sk);
+	/* Suspend caller if AIO operations are in flight. */
+	wait_event_interruptible(skcipher_aio_finish_wait,
+				 (ctx->inflight == 0));
 
-	skcipher_free_sgl(sk);
+	skcipher_pull_tsgl(sk, ctx->used);
 	sock_kzfree_s(sk, ctx->iv, crypto_skcipher_ivsize(tfm));
 	sock_kfree_s(sk, ctx, ctx->len);
 	af_alg_release_parent(sk);
@@ -925,7 +839,7 @@ static int skcipher_accept_parent_nokey(void *private, struct sock *sk)
 	struct alg_sock *ask = alg_sk(sk);
 	struct skcipher_tfm *tfm = private;
 	struct crypto_skcipher *skcipher = tfm->skcipher;
-	unsigned int len = sizeof(*ctx) + crypto_skcipher_reqsize(skcipher);
+	unsigned int len = sizeof(*ctx);
 
 	ctx = sock_kmalloc(sk, len, GFP_KERNEL);
 	if (!ctx)
@@ -943,19 +857,15 @@ static int skcipher_accept_parent_nokey(void *private, struct sock *sk)
 	INIT_LIST_HEAD(&ctx->tsgl);
 	ctx->len = len;
 	ctx->used = 0;
+	ctx->processed = 0;
+	ctx->inflight = 0;
 	ctx->more = 0;
 	ctx->merge = 0;
 	ctx->enc = 0;
-	atomic_set(&ctx->inflight, 0);
 	af_alg_init_completion(&ctx->completion);
 
 	ask->private = ctx;
 
-	skcipher_request_set_tfm(&ctx->req, skcipher);
-	skcipher_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_SLEEP |
-						 CRYPTO_TFM_REQ_MAY_BACKLOG,
-				      af_alg_complete, &ctx->completion);
-
 	sk->sk_destruct = skcipher_sock_destruct;
 
 	return 0;
-- 
2.9.3

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Herbert Xu]

On Sun, Dec 25, 2016 at 06:15:06PM +0100, Stephan Müller wrote:
>
> + * The following concept of the memory management is used:
> + *
> + * The kernel maintains two SGLs, the TX SGL and the RX SGL. The TX SGL is
> + * filled by user space with the data submitted via sendpage/sendmsg. Filling
> + * up the TX SGL does not cause a crypto operation -- the data will only be
> + * tracked by the kernel. Upon receipt of one recvmsg call, the caller must
> + * provide a buffer which is tracked with the RX SGL.
> + *
> + * During the processing of the recvmsg operation, the cipher request is
> + * allocated and prepared. To support multiple recvmsg operations operating
> + * on one TX SGL, an offset pointer into the TX SGL is maintained. The TX SGL
> + * that is used for the crypto request is scatterwalk_ffwd by the offset
> + * pointer to obtain the start address the crypto operation shall use for
> + * the input data.

I think this is overcomplicating things.  The async interface
should be really simple.  It should be exactly the same as the
sync interface, except that completion is out-of-line.

So there should be no mixing of SGLs from different requests.
 Just start with a clean slate after each recvmsg regardless of
whether it's sync or async.

The only difference in the async case is that you need to keep a
reference to the old pages and free them upon completion.  But this
should in no way interfere with subsequent requests.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Stephan Müller]

Am Donnerstag, 12. Januar 2017, 23:51:28 CET schrieb Herbert Xu:

Hi Herbert,

> On Sun, Dec 25, 2016 at 06:15:06PM +0100, Stephan Müller wrote:
> > + * The following concept of the memory management is used:
> > + *
> > + * The kernel maintains two SGLs, the TX SGL and the RX SGL. The TX SGL
> > is
> > + * filled by user space with the data submitted via sendpage/sendmsg.
> > Filling + * up the TX SGL does not cause a crypto operation -- the data
> > will only be + * tracked by the kernel. Upon receipt of one recvmsg call,
> > the caller must + * provide a buffer which is tracked with the RX SGL.
> > + *
> > + * During the processing of the recvmsg operation, the cipher request is
> > + * allocated and prepared. To support multiple recvmsg operations
> > operating + * on one TX SGL, an offset pointer into the TX SGL is
> > maintained. The TX SGL + * that is used for the crypto request is
> > scatterwalk_ffwd by the offset + * pointer to obtain the start address
> > the crypto operation shall use for + * the input data.
> 
> I think this is overcomplicating things.  The async interface
> should be really simple.  It should be exactly the same as the
> sync interface, except that completion is out-of-line.
> 
> So there should be no mixing of SGLs from different requests.
>  Just start with a clean slate after each recvmsg regardless of
> whether it's sync or async.
> 
> The only difference in the async case is that you need to keep a
> reference to the old pages and free them upon completion.  But this
> should in no way interfere with subsequent requests.

That would mean that we would only support one IOCB.

At least with algif_skcipher, having multiple IOCBs would reduce the number of 
system calls user space needs to make for multiple plaintext / ciphertext 
blocks. But then, with the use of IOVECs, user space could provide all input 
data with one system call anyway.

Ok, I will update the patch as suggested.
> 
> Cheers,



Ciao
Stephan

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Stephan Müller]

Am Donnerstag, 12. Januar 2017, 16:56:04 CET schrieb Stephan Müller:

Hi Herbert,

> 
> That would mean that we would only support one IOCB.

As we also need to be standards compliant, would it be appropriate to only 
support one IOCB? I think this is a significant difference to other AIO 
operations like for networking.

Ciao
Stephan

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Herbert Xu]

On Thu, Jan 12, 2017 at 05:05:03PM +0100, Stephan Müller wrote:
> Am Donnerstag, 12. Januar 2017, 16:56:04 CET schrieb Stephan Müller:
> 
> Hi Herbert,
> 
> > 
> > That would mean that we would only support one IOCB.
> 
> As we also need to be standards compliant, would it be appropriate to only 
> support one IOCB? I think this is a significant difference to other AIO 
> operations like for networking.

Why would we be limited to one IOCB?
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Stephan Müller]

Am Freitag, 13. Januar 2017, 00:07:39 CET schrieb Herbert Xu:

Hi Herbert,

> On Thu, Jan 12, 2017 at 05:05:03PM +0100, Stephan Müller wrote:
> > Am Donnerstag, 12. Januar 2017, 16:56:04 CET schrieb Stephan Müller:
> > 
> > Hi Herbert,
> > 
> > > That would mean that we would only support one IOCB.
> > 
> > As we also need to be standards compliant, would it be appropriate to only
> > support one IOCB? I think this is a significant difference to other AIO
> > operations like for networking.
> 
> Why would we be limited to one IOCB?

Each IOCB would transpire into an independent, separate recvmsg invocation 
without an additional sendmsg/sendpage operation. Thus, in order to support 
multiple IOCBs, all data the multiple recvmsg invocations will operate on must 
be injected into the kernel beforehand.

Ciao
Stephan

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Herbert Xu]

On Thu, Jan 12, 2017 at 05:10:14PM +0100, Stephan Müller wrote:
>
> Each IOCB would transpire into an independent, separate recvmsg invocation 
> without an additional sendmsg/sendpage operation. Thus, in order to support 
> multiple IOCBs, all data the multiple recvmsg invocations will operate on must 
> be injected into the kernel beforehand.

I don't understand, what's wrong with:

sendmsg(fd, ...)
aio_read(iocb1)
sendmsg(fd, ...)
aio_read(iocb2)

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Stephan Müller]

Am Freitag, 13. Januar 2017, 00:17:59 CET schrieb Herbert Xu:

Hi Herbert,

> On Thu, Jan 12, 2017 at 05:10:14PM +0100, Stephan Müller wrote:
> > Each IOCB would transpire into an independent, separate recvmsg invocation
> > without an additional sendmsg/sendpage operation. Thus, in order to
> > support
> > multiple IOCBs, all data the multiple recvmsg invocations will operate on
> > must be injected into the kernel beforehand.
> 
> I don't understand, what's wrong with:
> 
> sendmsg(fd, ...)
> aio_read(iocb1)
> sendmsg(fd, ...)
> aio_read(iocb2)

Sure, that works. But here you limit yourself to one IOCB per aio_read. But 
aio_read supports multiple IOCBs in one invocation. And this is the issue I am 
considering.

Ciao
Stephan

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Herbert Xu]

On Thu, Jan 12, 2017 at 05:19:57PM +0100, Stephan Müller wrote:
>
> > I don't understand, what's wrong with:
> > 
> > sendmsg(fd, ...)
> > aio_read(iocb1)
> > sendmsg(fd, ...)
> > aio_read(iocb2)
> 
> Sure, that works. But here you limit yourself to one IOCB per aio_read. But 
> aio_read supports multiple IOCBs in one invocation. And this is the issue I am 
> considering.

Not really.  You just lay it out in the same way with lio_listio.
That is, a write followed by read, etc.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Stephan Müller]

Am Freitag, 13. Januar 2017, 18:21:45 CET schrieb Herbert Xu:

Hi Herbert,

> On Thu, Jan 12, 2017 at 05:19:57PM +0100, Stephan Müller wrote:
> > > I don't understand, what's wrong with:
> > > 
> > > sendmsg(fd, ...)
> > > aio_read(iocb1)
> > > sendmsg(fd, ...)
> > > aio_read(iocb2)
> > 
> > Sure, that works. But here you limit yourself to one IOCB per aio_read.
> > But
> > aio_read supports multiple IOCBs in one invocation. And this is the issue
> > I am considering.
> 
> Not really.  You just lay it out in the same way with lio_listio.
> That is, a write followed by read, etc.

According to the man page of lio_listio(3) the provided AIO operations are 
executed in an unspecified order. I would infer from that statement that even 
if an order of write / read / write / read is defined by the caller, this 
order may not be followed by the kernel. Thus we would need to consider the 
case that in the end, algif has to process the order of write / write / read / 
read or any other order.

Besides, the crashes I reported for the current AIO implementation in 
algif_aead and algif_skcipher are always triggered when invoking an aio_read 
with two or more IOCBs. The most important aspect I want to cover with the 
patch set is to stop crashing the kernel.

Ciao
Stephan

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Herbert Xu]

On Fri, Jan 13, 2017 at 11:49:05AM +0100, Stephan Müller wrote:
> 
> According to the man page of lio_listio(3) the provided AIO operations are 
> executed in an unspecified order. I would infer from that statement that even 
> if an order of write / read / write / read is defined by the caller, this 
> order may not be followed by the kernel. Thus we would need to consider the 
> case that in the end, algif has to process the order of write / write / read / 
> read or any other order.

Well if ordering is not guaranteed that I don't see how your code
can work either.  Or am I missing something?

> Besides, the crashes I reported for the current AIO implementation in 
> algif_aead and algif_skcipher are always triggered when invoking an aio_read 
> with two or more IOCBs. The most important aspect I want to cover with the 
> patch set is to stop crashing the kernel.

Please stop adding new features and just fix the existing crash.
Once we have that covered and backported to stable then we can
start addressing new features.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Stephan Müller]

Am Freitag, 13. Januar 2017, 19:03:35 CET schrieb Herbert Xu:

Hi Herbert,

> On Fri, Jan 13, 2017 at 11:49:05AM +0100, Stephan Müller wrote:
> > According to the man page of lio_listio(3) the provided AIO operations are
> > executed in an unspecified order. I would infer from that statement that
> > even if an order of write / read / write / read is defined by the caller,
> > this order may not be followed by the kernel. Thus we would need to
> > consider the case that in the end, algif has to process the order of
> > write / write / read / read or any other order.
> 
> Well if ordering is not guaranteed that I don't see how your code
> can work either.  Or am I missing something?

The patch simply stores all data it gets from sendmsg in the src SGL. In 
addition it maintains an offset pointer into that src SGLs.

When the recvmsg call comes in and the dst SGL is prepared, it simply takes as 
much data from the src SGL as needed to cover the request defined by the dst 
SGL. After completing that operation, the offset pointer is moved forward to 
point to a yet unused part of the src SGL. If another recvmsg comes in without 
an intermediate sendmsg, it simply starts using the data from the src SGL 
starting from the offset.

Therefore, the code should now be able to handle a write / write / read / read 
scenario. Or it can handle, say, a write(32 bytes) / read (16 bytes) / read 
(16 bytes). At least my tests covered a successful testing of that scenario 
which always crashed the kernel before.

Ciao
Stephan

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Herbert Xu]

On Fri, Jan 13, 2017 at 12:10:02PM +0100, Stephan Müller wrote:
>
> > Well if ordering is not guaranteed that I don't see how your code
> > can work either.  Or am I missing something?
> 
> The patch simply stores all data it gets from sendmsg in the src SGL. In 
> addition it maintains an offset pointer into that src SGLs.
> 
> When the recvmsg call comes in and the dst SGL is prepared, it simply takes as 
> much data from the src SGL as needed to cover the request defined by the dst 
> SGL. After completing that operation, the offset pointer is moved forward to 
> point to a yet unused part of the src SGL. If another recvmsg comes in without 
> an intermediate sendmsg, it simply starts using the data from the src SGL 
> starting from the offset.
> 
> Therefore, the code should now be able to handle a write / write / read / read 
> scenario. Or it can handle, say, a write(32 bytes) / read (16 bytes) / read 
> (16 bytes). At least my tests covered a successful testing of that scenario 
> which always crashed the kernel before.

Are you making separate read calls or just a single one? If you're
making separate calls, then this is no differnt to just doing
write/read pairs.  You're not saving any overhead.

If you're making a single call, what guarantees the ordering?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Stephan Müller]

Am Freitag, 13. Januar 2017, 19:12:59 CET schrieb Herbert Xu:

Hi Herbert,

> On Fri, Jan 13, 2017 at 12:10:02PM +0100, Stephan Müller wrote:
> > > Well if ordering is not guaranteed that I don't see how your code
> > > can work either.  Or am I missing something?
> > 
> > The patch simply stores all data it gets from sendmsg in the src SGL. In
> > addition it maintains an offset pointer into that src SGLs.
> > 
> > When the recvmsg call comes in and the dst SGL is prepared, it simply
> > takes as much data from the src SGL as needed to cover the request
> > defined by the dst SGL. After completing that operation, the offset
> > pointer is moved forward to point to a yet unused part of the src SGL. If
> > another recvmsg comes in without an intermediate sendmsg, it simply
> > starts using the data from the src SGL starting from the offset.
> > 
> > Therefore, the code should now be able to handle a write / write / read /
> > read scenario. Or it can handle, say, a write(32 bytes) / read (16 bytes)
> > / read (16 bytes). At least my tests covered a successful testing of that
> > scenario which always crashed the kernel before.
> 
> Are you making separate read calls or just a single one? If you're
> making separate calls, then this is no differnt to just doing
> write/read pairs.  You're not saving any overhead.

I make one read call.
> 
> If you're making a single call, what guarantees the ordering?

Technically, io_submit is the syscall that triggers the recvmsg. Are you 
saying that this syscall does not maintain ordering? At least the man page 
does not add any hints that it would not (unlike the lio_list man page).

Ciao
Stephan

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Herbert Xu]

On Fri, Jan 13, 2017 at 12:16:27PM +0100, Stephan Müller wrote:
>
> > If you're making a single call, what guarantees the ordering?
> 
> Technically, io_submit is the syscall that triggers the recvmsg. Are you 
> saying that this syscall does not maintain ordering? At least the man page 
> does not add any hints that it would not (unlike the lio_list man page).

The code certainly does.  But my point is that you can do the
same thing using the current API.  Just make your list be pairs
of write/read and it should work.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Stephan Müller]

Am Freitag, 13. Januar 2017, 19:25:39 CET schrieb Herbert Xu:

Hi Herbert,

> On Fri, Jan 13, 2017 at 12:16:27PM +0100, Stephan Müller wrote:
> > > If you're making a single call, what guarantees the ordering?
> > 
> > Technically, io_submit is the syscall that triggers the recvmsg. Are you
> > saying that this syscall does not maintain ordering? At least the man page
> > does not add any hints that it would not (unlike the lio_list man page).
> 
> The code certainly does.  But my point is that you can do the
> same thing using the current API.  Just make your list be pairs
> of write/read and it should work.

How shall these pairs come into existence? The read/write system calls may 
come at unspecified times with potentially dissimilar buffer lengths.


Ciao
Stephan

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Stephan Müller]

Am Freitag, 13. Januar 2017, 19:25:39 CET schrieb Herbert Xu:

Hi Herbert,

> On Fri, Jan 13, 2017 at 12:16:27PM +0100, Stephan Müller wrote:
> > > If you're making a single call, what guarantees the ordering?
> > 
> > Technically, io_submit is the syscall that triggers the recvmsg. Are you
> > saying that this syscall does not maintain ordering? At least the man page
> > does not add any hints that it would not (unlike the lio_list man page).
> 
> The code certainly does.  But my point is that you can do the
> same thing using the current API.  Just make your list be pairs
> of write/read and it should work.

Over the weekend I spend more time looking into the implementation of 
io_submit and its data structures exchanged between user space and kernel 
space.

During that review I was unable to find any way how an io_submit can be linked 
to a sendmsg/sendpage operation. Therefore, I am unable to see how the 
suggested TX/RX SGL pair can come into existence during sendmsg time.

A TX/RX SGL pair can only come into existence during recvmsg that is triggered 
by io_submit when the kernel learns about the amount of data it shall process. 
That means that for sendmsg all the kernel can do is store the provided data 
in a serial fashion in the TX SGL. During recvmsg, the kernel then takes the 
required data from the TX SGL and processes it so that the output can be 
stored in the RX SGL.

Note, the kernel has to handle dissimilar sendmsg/recvmsg invocations, e.g. 
sendmsg(16 bytes), sendmsg(20 bytes), sendmsg(12bytes), recvmsg(32bytes), 
recvmsg(16 bytes). As we cannot link the sendmsg to the recvmsg calls, all the 
kernel can do is to collect the data during sendmsg and process the parts 
requested during recvmsg.

With the patch set, I exactly do that. I have one TX SGL that is simply filled 
with data as user space uses sendmsg to send data. At the time the recvmsg is 
invoked and the kernel sees how much buffer the caller provides and thus knows 
how much data it can process from the TX SGL (assuming that the kernel shall 
fill the entire recvmsg buffer as much as possible), it performs the crypto 
operation.

After the kernel processed (parts of) the TX SGL data, it could now free up 
the processed SGs in that SGL. That is already performed in the algif_skcipher 
interface, but not in the algif_aead (there, the TX SGL is only freed after 
all its components are processed). Please note that as I mentioned in the 
intro part to the patch, I only tried to fix the RX SGL handling. If my 
approach is accepted, I volunteer to port the algif_skcipher TX SGL handling 
to algif_aead so that in algif_aead the TX SG entries are freed once they are 
processed. Furthermore, I see that now there is huge code duplication 
regarding the RX/TX SGL handling between algif_skcipher and algif_aead which 
can than be handled with common service functions. But again, such work makes 
only sense if the initial approach discussed above and presented with this 
first patch set is accepted.

Ciao
Stephan