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

[Stephan Müller]

Am Dienstag, 20. Juni 2017, 05:10:42 CEST schrieb Herbert Xu:

Hi Herbert,

> > +   int err = _skcipher_recvmsg(sock, msg, ignored, flags);
> > +
> > +   /*
> > +* This error covers -EIOCBQUEUED which implies that we can
> > +* only handle one AIO request. If the caller wants to have
> > +* multiple AIO requests in parallel, he must make multiple
> > +* separate AIO calls.
> > +*/
> > +   if (err < 0) {
> > +   ret = err;
> > +   goto out;
> 
> This looks like a semantic change.  The previous code would return
> the number of bytes already successfully processed in case of a
> subsequent error.  With your new code you will always return the
> error.

In the current code, the synchronous returns the processed bytes whereas the 
async case returns EIOCBQUEUED if this error occurs without returning the 
processed bytes.

Thus, would you like to change that code into

if (err < 0) {
if (err == -EIOCBQUEUED)
ret = err;

goto out;
}

?


> 
> > @@ -724,10 +737,9 @@ static unsigned int skcipher_poll(struct file *file,
> > struct socket *sock,> 
> > struct sock *sk = sock->sk;
> > struct alg_sock *ask = alg_sk(sk);
> > struct skcipher_ctx *ctx = ask->private;
> > 
> > -   unsigned int mask;
> > +   unsigned int mask = 0;
> > 
> > sock_poll_wait(file, sk_sleep(sk), wait);
> > 
> > -   mask = 0;
> > 
> > if (ctx->used)
> > 
> > mask |= POLLIN | POLLRDNORM;
> 
> Please remove this hunk as it has nothing to do with this patch.

Removed.

Thanks

Ciao
Stephan

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

[Herbert Xu]

On Sat, Jun 10, 2017 at 07:59:25PM +0200, Stephan Müller wrote:
>
> -static int skcipher_recvmsg_sync(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);
> - 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;
> + int ret = 0;
>  
>   lock_sock(sk);
>   while (msg_data_left(msg)) {
> - if (!ctx->used) {
> - err = skcipher_wait_for_data(sk, flags);
> - if (err)
> - goto unlock;
> + int err = _skcipher_recvmsg(sock, msg, ignored, flags);
> +
> + /*
> +  * This error covers -EIOCBQUEUED which implies that we can
> +  * only handle one AIO request. If the caller wants to have
> +  * multiple AIO requests in parallel, he must make multiple
> +  * separate AIO calls.
> +  */
> + if (err < 0) {
> + ret = err;
> + goto out;

This looks like a semantic change.  The previous code would return
the number of bytes already successfully processed in case of a
subsequent error.  With your new code you will always return the
error.

> @@ -724,10 +737,9 @@ static unsigned int skcipher_poll(struct file *file, 
> struct socket *sock,
>   struct sock *sk = sock->sk;
>   struct alg_sock *ask = alg_sk(sk);
>   struct skcipher_ctx *ctx = ask->private;
> - unsigned int mask;
> + unsigned int mask = 0;
>  
>   sock_poll_wait(file, sk_sleep(sk), wait);
> - mask = 0;
>  
>   if (ctx->used)
>   mask |= POLLIN | POLLRDNORM;

Please remove this hunk as it has nothing to do with this patch.

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

[PATCH v9 1/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 
---
 crypto/algif_skcipher.c | 567 
 1 file changed, 282 insertions(+), 285 deletions(-)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 43839b0..8d238a0 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -10,6 +10,21 @@
  * 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. As part of the recvmsg operation, the processed
+ * TX buffers are extracted from the TX SGL into a separate SGL.
+ *
+ * After the completion of the crypto operation, the RX SGL and the cipher
+ * request is released. The extracted TX SGL parts are released together with
+ * the RX SGL release.
  */
 
 #include 
@@ -24,109 +39,94 @@
 #include 
 #include 
 
-struct skcipher_sg_list {
+struct skcipher_tsgl {
struct list_head list;
-
int cur;
-
struct scatterlist sg[0];
 };
 
+struct skcipher_rsgl {
+   struct af_alg_sgl sgl;
+   struct list_head list;
+   size_t sg_num_bytes;
+};
+
+struct skcipher_async_req {
+   struct kiocb *iocb;
+   struct sock *sk;
+
+   struct skcipher_rsgl first_sgl;
+   struct list_head rsgl_list;
+
+   struct scatterlist *tsgl;
+   unsigned int tsgl_entries;
+
+   unsigned int areqlen;
+   struct skcipher_request req;
+};
+
 struct skcipher_tfm {
struct crypto_skcipher *skcipher;
bool has_key;
 };
 
 struct skcipher_ctx {
-   struct list_head tsgl;
-   struct af_alg_sgl rsgl;
+   struct list_head tsgl_list;
 
void *iv;
 
struct af_alg_completion completion;
 
-   atomic_t inflight;
size_t used;
+   size_t rcvused;
 
-   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;
-};
-
-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;
+   unsigned int len;
 };
 
-#define MAX_SGL_ENTS ((4096 - sizeof(struct skcipher_sg_list)) / \
+#define MAX_SGL_ENTS ((4096 - sizeof(struct skcipher_tsgl)) / \
  sizeof(struct scatterlist) - 1)
 
-static void skcipher_free_async_sgls(struct skcipher_async_req *sreq)
+static inline int skcipher_sndbuf(struct sock *sk)
 {
-   struct skcipher_async_rsgl *rsgl, *tmp;
-   struct scatterlist *sgl;
-   struct scatterlist *sg;
-   int i, n;
-
-   list_for_each_entry_safe(rsgl, tmp, >list, list) {
-   af_alg_free_sg(>sgl);
-   if (rsgl != >first_sgl)
-   kfree(rsgl);
-   }
-   sgl = sreq->tsg;
-   n = sg_nents(sgl);
-   for_each_sg(sgl, sg, n, i)
-   put_page(sg_page(sg));
+   struct alg_sock *ask = alg_sk(sk);
+   struct skcipher_ctx *ctx = ask->private;
 
-   kfree(sreq->tsg);
+   return max_t(int, max_t(int, sk->sk_sndbuf & PAGE_MASK, PAGE_SIZE) -
+ ctx->used, 0);
 }
 
-static void skcipher_async_cb(struct crypto_async_request *req, int err)
+static inline bool skcipher_writable(struct sock *sk)
 {
-