[QUESTION] Crypto queue handling

[QUESTION] Crypto queue handling

[Marek Vasut]

Hello,

I'm digging in crypto/algapi.c , in the crypto_enqueue_request() function. I 
don't quite understand how the backlog mechanism should work. According to [1], 
I suspect it should limit the amount of requests in the queue to $max_qlen and 
allow one more request to be enqueued into the $backlog ; and if there is more 
requests than $max_qlen, it should start dropping the requests ?

Inspecting the code, I see these situations:
1) qlen < max_qlen
   -> The request is enqueued, qlen is increased , returns -EINPROGRESS
2) qlen >= max_qlen
   -> The crypto_enqueue_request() returns -EBUSY in this case
   2a) request->flags has CRYPTO_TFM_REQ_MAY_BACKLOG unset
       => Request is not enqueued , qlen is not increased
   2b) request->flags has CRYPTO_TFM_REQ_MAY_BACKLOG set
       => Request is enqueued , qlen is increased

Therefore, I suspect if 2b) case happens repeatedly, the queue will grow 
indefinitelly. I'm not sure if this can happen, but I suspect that's really 
possible. My understanding is that the driver will call crypto_enqueue_request() 
and propagate it's return value to the upper layers. They upper layet can 
generate such a request that has CRYPTO_TFM_REQ_MAY_BACKLOG set .

But how shall the upper layers handle the -EBUSY return value from the crypto 
API calls? Shall they stop saturating the crypto API with requests ? How can the 
upper layers determine when can they resume sending crypto requests ? Shall they 
just try calling crypto_enqueue_request() again to see if it still returns -
EBUSY ?

Sorry if this is an obvious question and thanks for the help!

[1] http://permalink.gmane.org/gmane.linux.kernel.cryptoapi/735

Best regards,
Marek Vasut

Re: [QUESTION] Crypto queue handling

[Marek Vasut]

Hello again,

> Hello,
> 
> I'm digging in crypto/algapi.c , in the crypto_enqueue_request() function.
> I don't quite understand how the backlog mechanism should work. According
> to [1], I suspect it should limit the amount of requests in the queue to
> $max_qlen and allow one more request to be enqueued into the $backlog ;
> and if there is more requests than $max_qlen, it should start dropping the
> requests ?
> 
> Inspecting the code, I see these situations:
> 1) qlen < max_qlen
>    -> The request is enqueued, qlen is increased , returns -EINPROGRESS
> 2) qlen >= max_qlen
>    -> The crypto_enqueue_request() returns -EBUSY in this case
>    2a) request->flags has CRYPTO_TFM_REQ_MAY_BACKLOG unset
>        => Request is not enqueued , qlen is not increased
>    2b) request->flags has CRYPTO_TFM_REQ_MAY_BACKLOG set
>        => Request is enqueued , qlen is increased
> 
> Therefore, I suspect if 2b) case happens repeatedly, the queue will grow
> indefinitelly. I'm not sure if this can happen, but I suspect that's really
> possible. My understanding is that the driver will call
> crypto_enqueue_request() and propagate it's return value to the upper
> layers. They upper layet can generate such a request that has
> CRYPTO_TFM_REQ_MAY_BACKLOG set .
> 
> But how shall the upper layers handle the -EBUSY return value from the
> crypto API calls? Shall they stop saturating the crypto API with requests
> ? How can the upper layers determine when can they resume sending crypto
> requests ? Shall they just try calling crypto_enqueue_request() again to
> see if it still returns - EBUSY ?

There is one more thing which I don't quite understand with regards to the 
queues, it's this pattern one can see in every second crypto driver (I took this 
from bfin_crc.c):

302         struct crypto_async_request *async_req, *backlog;
[...]
311         spin_lock_irqsave(&crc->lock, flags);
[...]
318         backlog = crypto_get_backlog(&crc->queue);
319         async_req = crypto_dequeue_request(&crc->queue);
320         if (async_req)
321                 crc->busy = 1;
322         spin_unlock_irqrestore(&crc->lock, flags);
323 
324         if (!async_req)
325                 return ret;
326 
327         if (backlog)
328                 backlog->complete(backlog, -EINPROGRESS);

So, we call backlog->complete() , but who did actually ever process the request 
stored in the queue->backlog ? To me, it seems like the request was never 
processed, but we complete it.

I think this will work in case the ->backlog == ->list , since in that case we 
will basically complete the currently dequeued request. Will this also work in 
case ->backlog != ->list (that is, in case the qlen overflew max_qlen)? I'm not 
sure.

Can you please help me understand this ?

Thank you!

Best regards,
Marek Vasut

Re: [QUESTION] Crypto queue handling

[Herbert Xu]

On Mon, May 26, 2014 at 06:16:58PM +0200, Marek Vasut wrote:
>
> So, we call backlog->complete() , but who did actually ever process the request 
> stored in the queue->backlog ? To me, it seems like the request was never 
> processed, but we complete it.

Nobody.  This is meant to indicate to the user that the queue now has
space for submission.  The backlogged request must be resubmitted.

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: [QUESTION] Crypto queue handling

[Herbert Xu]

On Mon, May 26, 2014 at 05:58:19PM +0200, Marek Vasut wrote:
> Hello,
> 
> I'm digging in crypto/algapi.c , in the crypto_enqueue_request() function. I 
> don't quite understand how the backlog mechanism should work. According to [1], 
> I suspect it should limit the amount of requests in the queue to $max_qlen and 
> allow one more request to be enqueued into the $backlog ; and if there is more 
> requests than $max_qlen, it should start dropping the requests ?
> 
> Inspecting the code, I see these situations:
> 1) qlen < max_qlen
>    -> The request is enqueued, qlen is increased , returns -EINPROGRESS
> 2) qlen >= max_qlen
>    -> The crypto_enqueue_request() returns -EBUSY in this case
>    2a) request->flags has CRYPTO_TFM_REQ_MAY_BACKLOG unset
>        => Request is not enqueued , qlen is not increased
>    2b) request->flags has CRYPTO_TFM_REQ_MAY_BACKLOG set
>        => Request is enqueued , qlen is increased

The queue is per-tfm.  In this case you never want to share a
tfm between two users that do not agree on whether to have backlog
or not.

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: [QUESTION] Crypto queue handling

[Hsieh, Che-Min]

Herbert:

It is nice to know that the request queue is per tfm. We are currently on android-3.10.  I believe all the drivers under drivers/crypto/   don't follow this rule.  At the minimum, our driver qcrypto does not.  Other than backlogging function, does it break anything? How critical is it to fix the driver soon?

Thanks

Regards,
Chemin  

-----Original Message-----
From: linux-crypto-owner@vger.kernel.org [mailto:linux-crypto-owner@vger.kernel.org] On Behalf Of Herbert Xu
Sent: Friday, May 30, 2014 5:31 AM
To: Marek Vasut
Cc: linux-crypto@vger.kernel.org; Arnd Bergmann; Pantelis Antoniou
Subject: Re: [QUESTION] Crypto queue handling

On Mon, May 26, 2014 at 05:58:19PM +0200, Marek Vasut wrote:
> Hello,
> 
> I'm digging in crypto/algapi.c , in the crypto_enqueue_request() 
> function. I don't quite understand how the backlog mechanism should 
> work. According to [1], I suspect it should limit the amount of 
> requests in the queue to $max_qlen and allow one more request to be 
> enqueued into the $backlog ; and if there is more requests than $max_qlen, it should start dropping the requests ?
> 
> Inspecting the code, I see these situations:
> 1) qlen < max_qlen
>    -> The request is enqueued, qlen is increased , returns 
> -EINPROGRESS
> 2) qlen >= max_qlen
>    -> The crypto_enqueue_request() returns -EBUSY in this case
>    2a) request->flags has CRYPTO_TFM_REQ_MAY_BACKLOG unset
>        => Request is not enqueued , qlen is not increased
>    2b) request->flags has CRYPTO_TFM_REQ_MAY_BACKLOG set
>        => Request is enqueued , qlen is increased

The queue is per-tfm.  In this case you never want to share a tfm between two users that do not agree on whether to have backlog or not.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [QUESTION] Crypto queue handling

[Herbert Xu]

On Fri, May 30, 2014 at 01:33:06PM +0000, Hsieh, Che-Min wrote:
> 
> It is nice to know that the request queue is per tfm. We are currently on android-3.10.  I believe all the drivers under drivers/crypto/   don't follow this rule.  At the minimum, our driver qcrypto does not.  Other than backlogging function, does it break anything? How critical is it to fix the driver soon?

Sorry I don't know what I was smoking when I said that but it's
bollocks :)

Marek is quite correct that MAY_BACKLOG users can indefinitely
add to the backlog queue.  However, it is meant to be limited
to one entry per-tfm.

This is because the user is supposed to back off once they get
EBUSY, until they're notified once the backlog entry is popped
off (but not processed, it must be resubmitted).

We don't really do any checking right now as there aren't many
MAY_BACKLOG users.  But if it becomes a problem we can always
add a check to ensure each tfm does not add more than one backlog
request.

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: [QUESTION] Crypto queue handling

[Herbert Xu]

On Tue, Nov 11, 2014 at 08:04:03PM +0200, Nicolae Rosia wrote:
> On Fri, May 30, 2014 at 4:41 PM, Herbert Xu <herbert@gondor.apana.org.au>
> wrote:
> 
> > [...]
> > This is because the user is supposed to back off once they get
> > EBUSY, until they're notified once the backlog entry is popped
> > off (but not processed, it must be resubmitted).
> > [...]
> >
> 
> I'm trying to understand the backlog mechanism and after reading the source
> of crypto_enqueue_request and Stephan Mueller's example [1], my
> understanding is that the user does not have to queue again the backlogged
> request because when it returned -EBUSY it was added to the
> crypto_queue->list. Am I missing something?

You're right, the backlogged request does not need to be resubmitted.

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