Re: [PATCH v3 01/28] crypto: change backlog return code to -EIOCBQUEUED
On Mon, Jul 3, 2017 at 3:35 PM, Herbert Xuwrote: > On Sun, Jul 02, 2017 at 05:41:43PM +0300, Gilad Ben-Yossef wrote: >> 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 >> was queued in the backlog to -EIOCBQUEUED, thus resolving both >> issues. >> >> Signed-off-by: Gilad Ben-Yossef > > So you're changing the success case from EBUSY to EIOCBQUEUED. > This results in a lot of churn as you have to change every single > driver and caller. > > How about changing the error case to use something other than > EBUSY instead? That was indeed my first thought - I wanted to change EBUSY to EAGAIN. It might even be a more descriptive error message since the failure is transient. What stopped me was that I saw the algif interface passes this EBUSY value to user space. I don't know of any software that depends on this value but I'm really averse to changing user space API. Of course, we can just plant a (ret != AGAIN : ? EBUSY) in the algif interface return to user space code path but that felt like a kludge to me. And it doesn't really save any churn, I think - I'd still have to visit all the places that use the flags value to distinguish between the two EBUSY use cases and I need to visit most places that check for EBUSY as backlog indication because I'm replacing the check with the generic replacement, so it doesn't look like in the end it will be a smaller change. Having said that, if you prefer me to replace the error case I'd happily do that. Thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/28] crypto: change backlog return code to -EIOCBQUEUED
On Sun, Jul 02, 2017 at 05:41:43PM +0300, Gilad Ben-Yossef wrote: > 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 > was queued in the backlog to -EIOCBQUEUED, thus resolving both > issues. > > Signed-off-by: Gilad Ben-YossefSo you're changing the success case from EBUSY to EIOCBQUEUED. This results in a lot of churn as you have to change every single driver and caller. How about changing the error case to use something other than EBUSY instead? Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 01/28] crypto: change backlog return code to -EIOCBQUEUED
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 was queued in the backlog to -EIOCBQUEUED, thus resolving both issues. Signed-off-by: Gilad Ben-Yossef--- crypto/af_alg.c | 2 +- crypto/ahash.c | 12 +++- crypto/algapi.c | 6 -- crypto/asymmetric_keys/public_key.c | 2 +- crypto/chacha20poly1305.c | 2 +- crypto/cryptd.c | 4 +--- crypto/cts.c| 6 ++ crypto/drbg.c | 2 +- crypto/gcm.c| 2 +- crypto/lrw.c| 8 ++-- crypto/rsa-pkcs1pad.c | 16 crypto/tcrypt.c | 6 +++--- crypto/testmgr.c| 12 ++-- crypto/xts.c| 8 ++-- 14 files changed, 32 insertions(+), 56 deletions(-) diff --git a/crypto/af_alg.c b/crypto/af_alg.c index 3556d8e..c67daba 100644 --- a/crypto/af_alg.c +++ b/crypto/af_alg.c @@ -484,7 +484,7 @@ int af_alg_wait_for_completion(int err, struct af_alg_completion *completion) { switch (err) { case -EINPROGRESS: - case -EBUSY: + case -EIOCBQUEUED: wait_for_completion(>completion); reinit_completion(>completion); err = completion->err; diff --git a/crypto/ahash.c b/crypto/ahash.c index 826cd7a..65d08db 100644 --- a/crypto/ahash.c +++ b/crypto/ahash.c @@ -334,9 +334,7 @@ static int ahash_op_unaligned(struct ahash_request *req, return err; err = op(req); - if (err == -EINPROGRESS || - (err == -EBUSY && (ahash_request_flags(req) & - CRYPTO_TFM_REQ_MAY_BACKLOG))) + if (err == -EINPROGRESS || err == -EIOCBQUEUED) return err; ahash_restore_req(req, err); @@ -394,9 +392,7 @@ static int ahash_def_finup_finish1(struct ahash_request *req, int err) req->base.complete = ahash_def_finup_done2; err = crypto_ahash_reqtfm(req)->final(req); - if (err == -EINPROGRESS || - (err == -EBUSY && (ahash_request_flags(req) & - CRYPTO_TFM_REQ_MAY_BACKLOG))) + if (err == -EINPROGRESS || err == -EIOCBQUEUED) return err; out: @@ -432,9 +428,7 @@ static int ahash_def_finup(struct ahash_request *req) return err; err = tfm->update(req); - if (err == -EINPROGRESS || - (err == -EBUSY && (ahash_request_flags(req) & - CRYPTO_TFM_REQ_MAY_BACKLOG))) + if (err == -EINPROGRESS || err == -EIOCBQUEUED) return err; return ahash_def_finup_finish1(req, err); diff --git a/crypto/algapi.c b/crypto/algapi.c index e4cc761..3bfd1fa 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 = -EBUSY; goto out; + } + err = -EIOCBQUEUED; if (queue->backlog == >list) queue->backlog = >list; } diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c index 3cd6e12..3fad1fd 100644 --- a/crypto/asymmetric_keys/public_key.c +++ b/crypto/asymmetric_keys/public_key.c @@ -141,7 +141,7 @@ int public_key_verify_signature(const struct public_key *pkey, * signature and returns that to us. */ ret = crypto_akcipher_verify(req); - if ((ret == -EINPROGRESS) || (ret == -EBUSY)) { + if ((ret == -EINPROGRESS) || (ret == -EIOCBQUEUED)) { wait_for_completion(); ret = compl.err; } diff --git a/crypto/chacha20poly1305.c b/crypto/chacha20poly1305.c index db1bc31..e0e2785 100644 --- a/crypto/chacha20poly1305.c +++ b/crypto/chacha20poly1305.c @@ -79,7 +79,7 @@ static inline void async_done_continue(struct aead_request *req, int err, if (!err)