"BUG: scheduling while atomic" in atmel-aes on Linux v4.14-rc6

Hello,

While running the kcapi test suite on a SAMA5D2 Xplained board with a v4.14-rc6 kernel, I encountered the following error:

# kcapi -x 9 -e -c "cbc(aes)" -i 00000000000000000000000000000000 -k
00000000000 0000000000000000000000000000000000000 -p
1b077a6af4b7f98229de786d7516b639 BUG: scheduling while atomic:
kcapi/926/0x00000100
CPU: 0 PID: 926 Comm: kcapi Not tainted 4.14.0-rc6 #2
Hardware name: Atmel SAMA5
[<c010caac>] (unwind_backtrace) from [<c010a9d8>] (show_stack+0x10/0x14)
[<c010a9d8>] (show_stack) from [<c013a914>] (__schedule_bug+0x60/0x80)
[<c013a914>] (__schedule_bug) from [<c074ac70>] (__schedule+0x368/0x3fc)
[<c074ac70>] (__schedule) from [<c074ad74>] (schedule+0x40/0xa0)
[<c074ad74>] (schedule) from [<c05cb96c>] (__lock_sock+0x78/0xb0)
[<c05cb96c>] (__lock_sock) from [<c05cddd0>] (lock_sock_nested+0x48/0x50)
[<c05cddd0>] (lock_sock_nested) from [<c030eec8>]
(af_alg_async_cb+0x20/0x80) [<c030eec8>] (af_alg_async_cb) from
[<c0573b70>]
(atmel_aes_transfer_complete+0x38/0x68)
[<c0573b70>] (atmel_aes_transfer_complete) from [<c0121258>]
(tasklet_action+0x68/0xb4)
[<c0121258>] (tasklet_action) from [<c010158c>] (__do_softirq+0xc4/0x250)
[<c010158c>] (__do_softirq) from [<c01215f0>] (irq_exit+0xfc/0x130)
[<c01215f0>] (irq_exit) from [<c014a208>] (__handle_domain_irq+0x58/0xa8)
[<c014a208>] (__handle_domain_irq) from [<c010b60c>] (__irq_svc+0x6c/0x90)
[<c010b60c>] (__irq_svc) from [<c0310280>] (skcipher_recvmsg+0x2d8/0x318)
[<c0310280>] (skcipher_recvmsg) from [<c05c8794>] (sock_read_iter+0x88/0xc8)
[<c05c8794>] (sock_read_iter) from [<c01f1940>]
(aio_read.constprop.3+0xcc/0x178)
[<c01f1940>] (aio_read.constprop.3) from [<c01f2968>]
(SyS_io_submit+0x540/0x644)
[<c01f2968>] (SyS_io_submit) from [<c0107480>] (ret_fast_syscall+0x0/0x48)

After bisecting, I determined that it appeared during the 4.14 merge window, with the following commit:

e870456d8e7c crypto: algif_skcipher - overhaul memory management

Am Mittwoch, 25. Oktober 2017, 17:26:31 CET schrieb Romain Izard:

Hi Romain,

the patch below should cover the issue you see. Would you mind testing it?

Thanks
Stephan

---8<---

The code paths protected by the socket-lock do not use or modify the socket in a non-atomic fashion. The actions pertaining the socket do not even need to be handled as an atomic operation. Thus, the socket-lock can be safely ignored.

This fixes a bug regarding scheduling in atomic as the callback function may be invoked in interrupt context.

Fixes 2d97591ef43d0 ("crypto: af_alg - consolidation of duplicate code")

Reported-by Romain Izard <[email protected]>

Signed-off-by Stephan Mueller <[email protected]>

---
 crypto/af_alg.c | 4 ----
 1 file changed, 4 deletions(-)

crypto/af_alg.c +0 -5

index 337cf382718e..a41f08642eee 100644
@@ -1063,8 +1063,6 @@ void af_alg_async_cb(struct crypto_async_request *_req, int err)
        struct kiocb *iocb = areq->iocb;
        unsigned int resultlen;
 
-       lock_sock(sk);
-
        /* Buffer size written by crypto operation. */
        resultlen = areq->outlen;
 

@@ -1073,8 +1071,6 @@ void af_alg_async_cb(struct crypto_async_request *_req, int err)
        __sock_put(sk);
 
        iocb->ki_complete(iocb, err ? err : resultlen, 0);
-
-       release_sock(sk);
 }
 EXPORT_SYMBOL_GPL(af_alg_async_cb);
 
--

2.13.6

Am Mittwoch, 25. Oktober 2017, 17:26:31 CET schrieb Romain Izard:

Hi Romain,

the patch below should cover the issue you see. Would you mind testing it?

Thanks
Stephan

---8<---

The code paths protected by the socket-lock do not use or modify the socket in a non-atomic fashion. The actions pertaining the socket do not even need to be handled as an atomic operation. Thus, the socket-lock can be safely ignored.

Quoted 11 lines

Am Mittwoch, 25. Oktober 2017, 17:26:31 CET schrieb Romain Izard:

Hi Romain,

the patch below should cover the issue you see. Would you mind testing it?

Thanks
Stephan

---8<---

Quoted 4 lines

The code paths protected by the socket-lock do not use or modify the socket in a non-atomic fashion. The actions pertaining the socket do not even need to be handled as an atomic operation. Thus, the socket-lock can be safely ignored.

Are you sure about that? In particular is the callback function still sane without the socket lock if a concurrent recvmsg/sendmsg call is made?

Your fixes header is wrong too as the locks weren't introduced in that commit, they just got moved around.

Quoted 5 lines

The code paths protected by the socket-lock do not use or modify the socket in a non-atomic fashion. The actions pertaining the socket do not even need to be handled as an atomic operation. Thus, the socket-lock can be safely ignored.

Are you sure about that? In particular is the callback function still sane without the socket lock if a concurrent recvmsg/sendmsg call is made?

Your fixes header is wrong too as the locks weren't introduced in that commit, they just got moved around.

Am Freitag, 3. November 2017, 14:20:16 CET schrieb Herbert Xu:

Quoted 3 lines

Are you sure about that? In particular is the callback function still sane without the socket lock if a concurrent recvmsg/sendmsg call is made?

I reviewed the code again and I cannot find a reason for keeping the lock. All we need to ensure is that the socket exists. This is ensured with the refcount of the socket released by __sock_put().

Quoted 5 lines

Am Freitag, 3. November 2017, 14:20:16 CET schrieb Herbert Xu:

Quoted 3 lines

Are you sure about that? In particular is the callback function still sane without the socket lock if a concurrent recvmsg/sendmsg call is made?

Quoted 3 lines

I reviewed the code again and I cannot find a reason for keeping the lock. All we need to ensure is that the socket exists. This is ensured with the refcount of the socket released by __sock_put().

OK, I can't see why we need a lock there either. However, the call to __sock_put looks suspicious. Why isn't this using sock_put?

Also the sock_hold on the caller side looks buggy. Surely it needs to be made before we even call the encrypt/decrypt functions rather than after it returns EINPROGRESS at which point it may well be too late?

Where I am not fully sure is whether af_alg_async_cb is called in any case. I.e. when we invoke an AIO operation with a cipher that completes synchronously (e.g. AES-NI), is this callback triggered?

crypto/algif_aead.c +1 -0

 	if (msg->msg_iocb && !is_sync_kiocb(msg->msg_iocb)) {
 		/* AIO operation */
+		sock_hold(sk);
 		areq->iocb = msg->msg_iocb;
 		aead_request_set_callback(&areq->cra_u.aead_req,
 					  CRYPTO_TFM_REQ_MAY_BACKLOG,
 					  af_alg_async_cb, areq);

The code paths protected by the socket-lock do not use or modify the socket in a non-atomic fashion. The actions pertaining the socket do not even need to be handled as an atomic operation. Thus, the socket-lock can be safely ignored.

This fixes a bug regarding scheduling in atomic as the callback function may be invoked in interrupt context.

In addition, the sock_hold is moved before the AIO encrypt/decrypt operation to ensure that the socket is always present. This avoids a tiny race window where the socket is unprotected and yet used by the AIO operation.

Finally, the release of resources for a crypto operation is moved into a common function of af_alg_free_resources.

Fixes e870456d8e7c8 ("crypto: algif_skcipher - overhaul memory management")

Fixes d887c52d6ae43 ("crypto: algif_aead - overhaul memory management")

Reported-by Romain Izard <[email protected]>

Signed-off-by Stephan Mueller <[email protected]>

---
 crypto/af_alg.c         | 21 ++++++++++++++-------
 crypto/algif_aead.c     | 23 ++++++++++++-----------
 crypto/algif_skcipher.c | 23 ++++++++++++-----------
 include/crypto/if_alg.h |  1 +
 4 files changed, 39 insertions(+), 29 deletions(-)

crypto/af_alg.c +14 -7

index 85cea9de324a..358749c38894 100644
@@ -1021,6 +1021,18 @@ ssize_t af_alg_sendpage(struct socket *sock, struct page *page,
 EXPORT_SYMBOL_GPL(af_alg_sendpage);
 
 /**
+ * af_alg_free_resources - release resources required for crypto request
+ */
+void af_alg_free_resources(struct af_alg_async_req *areq)
+{
+       struct sock *sk = areq->sk;
+
+       af_alg_free_areq_sgls(areq);
+       sock_kfree_s(sk, areq, areq->areqlen);
+}
+EXPORT_SYMBOL_GPL(af_alg_free_resources);
+
+/**
  * af_alg_async_cb - AIO callback handler
  *
  * This handler cleans up the struct af_alg_async_req upon completion of the

@@ -1036,18 +1048,13 @@ void af_alg_async_cb(struct crypto_async_request *_req, int err)
        struct kiocb *iocb = areq->iocb;
        unsigned int resultlen;
 
-       lock_sock(sk);
-
        /* Buffer size written by crypto operation. */
        resultlen = areq->outlen;
 
-       af_alg_free_areq_sgls(areq);
-       sock_kfree_s(sk, areq, areq->areqlen);
-       __sock_put(sk);
+       af_alg_free_resources(areq);
+       sock_put(sk);
 
        iocb->ki_complete(iocb, err ? err : resultlen, 0);
-
-       release_sock(sk);
 }
 EXPORT_SYMBOL_GPL(af_alg_async_cb);
 

crypto/algif_aead.c +12 -11

index aacae0837aff..db9378a45296 100644
@@ -268,12 +268,23 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
 
        if (msg->msg_iocb && !is_sync_kiocb(msg->msg_iocb)) {
                /* AIO operation */
+               sock_hold(sk);
                areq->iocb = msg->msg_iocb;
                aead_request_set_callback(&areq->cra_u.aead_req,
                                          CRYPTO_TFM_REQ_MAY_BACKLOG,
                                          af_alg_async_cb, areq);
                err = ctx->enc ? crypto_aead_encrypt(&areq->cra_u.aead_req) :
                                 crypto_aead_decrypt(&areq->cra_u.aead_req);
+
+               /* AIO operation in progress */
+               if (err == -EINPROGRESS || err == -EBUSY) {
+                       /* Remember output size that will be generated. */
+                       areq->outlen = outlen;
+
+                       return -EIOCBQUEUED;
+               }
+
+               sock_put(sk);
        } else {
                /* Synchronous operation */
                aead_request_set_callback(&areq->cra_u.aead_req,

@@ -285,19 +296,9 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
                                &ctx->wait);
        }
 
-       /* AIO operation in progress */
-       if (err == -EINPROGRESS) {
-               sock_hold(sk);
-
-               /* Remember output size that will be generated. */
-               areq->outlen = outlen;
-
-               return -EIOCBQUEUED;
-       }
 
 free:
-       af_alg_free_areq_sgls(areq);
-       sock_kfree_s(sk, areq, areq->areqlen);
+       af_alg_free_resources(areq);
 
        return err ? err : outlen;
 }

crypto/algif_skcipher.c +12 -11

index 9954b078f0b9..30cff827dd8f 100644
@@ -117,6 +117,7 @@ static int _skcipher_recvmsg(struct socket *sock, struct msghdr *msg,
 
        if (msg->msg_iocb && !is_sync_kiocb(msg->msg_iocb)) {
                /* AIO operation */
+               sock_hold(sk);
                areq->iocb = msg->msg_iocb;
                skcipher_request_set_callback(&areq->cra_u.skcipher_req,
                                              CRYPTO_TFM_REQ_MAY_SLEEP,

@@ -124,6 +125,16 @@ static int _skcipher_recvmsg(struct socket *sock, struct msghdr *msg,
                err = ctx->enc ?
                        crypto_skcipher_encrypt(&areq->cra_u.skcipher_req) :
                        crypto_skcipher_decrypt(&areq->cra_u.skcipher_req);
+
+               /* AIO operation in progress */
+               if (err == -EINPROGRESS || err == -EBUSY) {
+                       /* Remember output size that will be generated. */
+                       areq->outlen = len;
+
+                       return -EIOCBQUEUED;
+               }
+
+               sock_put(sk);
        } else {
                /* Synchronous operation */
                skcipher_request_set_callback(&areq->cra_u.skcipher_req,

@@ -136,19 +147,9 @@ static int _skcipher_recvmsg(struct socket *sock, struct msghdr *msg,
                                                 &ctx->wait);
        }
 
-       /* AIO operation in progress */
-       if (err == -EINPROGRESS) {
-               sock_hold(sk);
-
-               /* Remember output size that will be generated. */
-               areq->outlen = len;
-
-               return -EIOCBQUEUED;
-       }
 
 free:
-       af_alg_free_areq_sgls(areq);
-       sock_kfree_s(sk, areq, areq->areqlen);
+       af_alg_free_resources(areq);
 
        return err ? err : len;
 }

include/crypto/if_alg.h +1 -1

index 6abf0a3604dc..38d9c5861ed8 100644
@@ -242,6 +242,7 @@ int af_alg_sendmsg(struct socket *sock, struct msghdr *msg, size_t size,
                   unsigned int ivsize);
 ssize_t af_alg_sendpage(struct socket *sock, struct page *page,
                        int offset, size_t size, int flags);
+void af_alg_free_resources(struct af_alg_async_req *areq);
 void af_alg_async_cb(struct crypto_async_request *_req, int err);
 unsigned int af_alg_poll(struct file *file, struct socket *sock,
                         poll_table *wait);
--

2.13.6

The code paths protected by the socket-lock do not use or modify the socket in a non-atomic fashion. The actions pertaining the socket do not even need to be handled as an atomic operation. Thus, the socket-lock can be safely ignored.

This fixes a bug regarding scheduling in atomic as the callback function may be invoked in interrupt context.

In addition, the sock_hold is moved before the AIO encrypt/decrypt operation to ensure that the socket is always present. This avoids a tiny race window where the socket is unprotected and yet used by the AIO operation.

Finally, the release of resources for a crypto operation is moved into a common function of af_alg_free_resources.

Fixes e870456d8e7c8 ("crypto: algif_skcipher - overhaul memory management")

Fixes d887c52d6ae43 ("crypto: algif_aead - overhaul memory management")

Reported-by Romain Izard <[email protected]>

Signed-off-by Stephan Mueller <[email protected]>

Quoted 16 lines

The code paths protected by the socket-lock do not use or modify the socket in a non-atomic fashion. The actions pertaining the socket do not even need to be handled as an atomic operation. Thus, the socket-lock can be safely ignored.

This fixes a bug regarding scheduling in atomic as the callback function may be invoked in interrupt context.

In addition, the sock_hold is moved before the AIO encrypt/decrypt operation to ensure that the socket is always present. This avoids a tiny race window where the socket is unprotected and yet used by the AIO operation.

Finally, the release of resources for a crypto operation is moved into a common function of af_alg_free_resources.

Quoted 4 lines

Fixes e870456d8e7c8 ("crypto: algif_skcipher - overhaul memory

management") Fixes: d887c52d6ae43 ("crypto: algif_aead - overhaul memory management") Reported-by: Romain Izard <[email protected]>

Signed-off-by Stephan Mueller <[email protected]>

Patch applied. Thanks.

Am Freitag, 24. November 2017, 08:37:39 CET schrieb Herbert Xu:

Hi Herbert,

Quoted 22 lines · Stephan M?ller

Quoted 20 lines

The code paths protected by the socket-lock do not use or modify the socket in a non-atomic fashion. The actions pertaining the socket do not even need to be handled as an atomic operation. Thus, the socket-lock can be safely ignored.

This fixes a bug regarding scheduling in atomic as the callback function may be invoked in interrupt context.

In addition, the sock_hold is moved before the AIO encrypt/decrypt operation to ensure that the socket is always present. This avoids a tiny race window where the socket is unprotected and yet used by the AIO operation.

Finally, the release of resources for a crypto operation is moved into a common function of af_alg_free_resources.

Fixes e870456d8e7c8 ("crypto: algif_skcipher - overhaul memory

management") Fixes: d887c52d6ae43 ("crypto: algif_aead - overhaul memory management") Reported-by: Romain Izard <[email protected]>

Signed-off-by Stephan Mueller <[email protected]>

Quoted 1 line

Patch applied. Thanks.

Thanks a lot.

Would it make sense to feed it to stable?

Ciao
Stephan

Would it make sense to feed it to stable?