[v8,01/20] crypto: change transient busy return code to -EAGAIN

Message ID 1504615144-29770-2-git-send-email-gilad@benyossef.com
State New
Headers show
Series
  • simplify crypto wait for async op
Related show

Commit Message

Gilad Ben-Yossef Sept. 5, 2017, 12:38 p.m.
The crypto API was using the -EBUSY return value to indicate
both a hard failure to submit a crypto operation into a
transformation provider when the latter was busy and the backlog
mechanism was not enabled as well as a notification that the
operation was queued into the backlog when the backlog mechanism
was enabled.

Having the same return code indicate two very different conditions
depending on a flag is both error prone and requires extra runtime
check like the following to discern between the cases:

	if (err == -EINPROGRESS ||
	    (err == -EBUSY && (ahash_request_flags(req) &
			       CRYPTO_TFM_REQ_MAY_BACKLOG)))

This patch changes the return code used to indicate a crypto op
failed due to the transformation provider being transiently busy
to -EAGAIN.

Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>

---
 crypto/algapi.c     |  6 ++++--
 crypto/algif_hash.c | 20 +++++++++++++++++---
 crypto/cryptd.c     |  4 +---
 3 files changed, 22 insertions(+), 8 deletions(-)

-- 
2.1.4

Comments

Herbert Xu Oct. 7, 2017, 3:05 a.m. | #1
On Tue, Sep 05, 2017 at 03:38:40PM +0300, Gilad Ben-Yossef wrote:
>

> diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c

> index 5e92bd2..3b3c154 100644

> --- a/crypto/algif_hash.c

> +++ b/crypto/algif_hash.c

> @@ -39,6 +39,20 @@ struct algif_hash_tfm {

>  	bool has_key;

>  };

>  

> +/* Previous versions of crypto_* ops used to return -EBUSY

> + * rather than -EAGAIN to indicate being tied up. The in

> + * kernel API changed but we don't want to break the user

> + * space API. As only the hash user interface exposed this

> + * error ever to the user, do the translation here.

> + */

> +static inline int crypto_user_err(int err)

> +{

> +	if (err == -EAGAIN)

> +		return -EBUSY;

> +

> +	return err;


I don't see the need to carry along this baggage.  Does anyone
in user-space actually rely on EBUSY?

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
Herbert Xu Oct. 11, 2017, 6:26 a.m. | #2
On Sat, Oct 07, 2017 at 10:51:42AM +0300, Gilad Ben-Yossef wrote:
> On Sat, Oct 7, 2017 at 6:05 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:

> > On Tue, Sep 05, 2017 at 03:38:40PM +0300, Gilad Ben-Yossef wrote:

> >>

> >> diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c

> >> index 5e92bd2..3b3c154 100644

> >> --- a/crypto/algif_hash.c

> >> +++ b/crypto/algif_hash.c

> >> @@ -39,6 +39,20 @@ struct algif_hash_tfm {

> >>       bool has_key;

> >>  };

> >>

> >> +/* Previous versions of crypto_* ops used to return -EBUSY

> >> + * rather than -EAGAIN to indicate being tied up. The in

> >> + * kernel API changed but we don't want to break the user

> >> + * space API. As only the hash user interface exposed this

> >> + * error ever to the user, do the translation here.

> >> + */

> >> +static inline int crypto_user_err(int err)

> >> +{

> >> +     if (err == -EAGAIN)

> >> +             return -EBUSY;

> >> +

> >> +     return err;

> >

> > I don't see the need to carry along this baggage.  Does anyone

> > in user-space actually rely on EBUSY?

> 

> 

> I am not aware of anyone who does. I was just trying to avoid

> changing the user ABI.

> 

> Shall I roll a new revision without this patch?


Yes please.  I'd rather not carry this around for eternity unless
it was actually required.

Thanks,
-- 
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

Patch

diff --git a/crypto/algapi.c b/crypto/algapi.c
index aa699ff..916bee3 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -897,9 +897,11 @@  int crypto_enqueue_request(struct crypto_queue *queue,
 	int err = -EINPROGRESS;
 
 	if (unlikely(queue->qlen >= queue->max_qlen)) {
-		err = -EBUSY;
-		if (!(request->flags & CRYPTO_TFM_REQ_MAY_BACKLOG))
+		if (!(request->flags & CRYPTO_TFM_REQ_MAY_BACKLOG)) {
+			err = -EAGAIN;
 			goto out;
+		}
+		err = -EBUSY;
 		if (queue->backlog == &queue->list)
 			queue->backlog = &request->list;
 	}
diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
index 5e92bd2..3b3c154 100644
--- a/crypto/algif_hash.c
+++ b/crypto/algif_hash.c
@@ -39,6 +39,20 @@  struct algif_hash_tfm {
 	bool has_key;
 };
 
+/* Previous versions of crypto_* ops used to return -EBUSY
+ * rather than -EAGAIN to indicate being tied up. The in
+ * kernel API changed but we don't want to break the user
+ * space API. As only the hash user interface exposed this
+ * error ever to the user, do the translation here.
+ */
+static inline int crypto_user_err(int err)
+{
+	if (err == -EAGAIN)
+		return -EBUSY;
+
+	return err;
+}
+
 static int hash_alloc_result(struct sock *sk, struct hash_ctx *ctx)
 {
 	unsigned ds;
@@ -136,7 +150,7 @@  static int hash_sendmsg(struct socket *sock, struct msghdr *msg,
 unlock:
 	release_sock(sk);
 
-	return err ?: copied;
+	return err ? crypto_user_err(err) : copied;
 }
 
 static ssize_t hash_sendpage(struct socket *sock, struct page *page,
@@ -188,7 +202,7 @@  static ssize_t hash_sendpage(struct socket *sock, struct page *page,
 unlock:
 	release_sock(sk);
 
-	return err ?: size;
+	return err ? crypto_user_err(err) : size;
 }
 
 static int hash_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
@@ -236,7 +250,7 @@  static int hash_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
 	hash_free_result(sk, ctx);
 	release_sock(sk);
 
-	return err ?: len;
+	return err ? crypto_user_err(err) : len;
 }
 
 static int hash_accept(struct socket *sock, struct socket *newsock, int flags,
diff --git a/crypto/cryptd.c b/crypto/cryptd.c
index 0508c48..d1dbdce 100644
--- a/crypto/cryptd.c
+++ b/crypto/cryptd.c
@@ -137,16 +137,14 @@  static int cryptd_enqueue_request(struct cryptd_queue *queue,
 	int cpu, err;
 	struct cryptd_cpu_queue *cpu_queue;
 	atomic_t *refcnt;
-	bool may_backlog;
 
 	cpu = get_cpu();
 	cpu_queue = this_cpu_ptr(queue->cpu_queue);
 	err = crypto_enqueue_request(&cpu_queue->queue, request);
 
 	refcnt = crypto_tfm_ctx(request->tfm);
-	may_backlog = request->flags & CRYPTO_TFM_REQ_MAY_BACKLOG;
 
-	if (err == -EBUSY && !may_backlog)
+	if (err == -EAGAIN)
 		goto out_put_cpu;
 
 	queue_work_on(cpu, kcrypto_wq, &cpu_queue->work);