Re: [PATCH v7 00/19] simplify crypto wait for async op

[Gilad Ben-Yossef]

On Tue, Sep 5, 2017 at 2:23 PM, Harsh Jain  wrote:
> On Sun, Sep 3, 2017 at 11:47 AM, Gilad Ben-Yossef  wrote:
>> On Thu, Aug 31, 2017 at 3:31 PM, Harsh Jain  wrote:
>>> HI Gilad,
>>>
>>> I think we need an update in ESP also. Now EBUSY return means driver
>>> has accepted, Packet should not be dropped in
>>>
>>> esp_output_tail() function.
>>
>> Good catch. You are right and the same holds true for ah_output() in ah4.c.
>>
>> But I do wonder, the code there now treats -EBUSY as a special case
>> and returns NET_XMIT_DROP
>> but if an AEAD or AHASH transformation return some other error, like
>> -ENOMEM or -EINVAL shouldn't
>> we return NET_XMIT_DROP in that case too?
> I think we should not,  XMIT_DROP implies drop current packet only,
> later on when device is recovered from busy state, Upper layer
> protocol(TCP) will re-transmit the packet. It helps in flow control.
>>

I see. Makes sense.

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

Re: [PATCH v7 00/19] simplify crypto wait for async op

[Harsh Jain]

On Sun, Sep 3, 2017 at 11:47 AM, Gilad Ben-Yossef  wrote:
> On Thu, Aug 31, 2017 at 3:31 PM, Harsh Jain  wrote:
>> HI Gilad,
>>
>> I think we need an update in ESP also. Now EBUSY return means driver
>> has accepted, Packet should not be dropped in
>>
>> esp_output_tail() function.
>
> Good catch. You are right and the same holds true for ah_output() in ah4.c.
>
> But I do wonder, the code there now treats -EBUSY as a special case
> and returns NET_XMIT_DROP
> but if an AEAD or AHASH transformation return some other error, like
> -ENOMEM or -EINVAL shouldn't
> we return NET_XMIT_DROP in that case too?
I think we should not,  XMIT_DROP implies drop current packet only,
later on when device is recovered from busy state, Upper layer
protocol(TCP) will re-transmit the packet. It helps in flow control.
>
> Any ideas?
>
> Gilad
>
>>
>>
>> On Thu, Aug 24, 2017 at 7:48 PM, Gilad Ben-Yossef  
>> wrote:
>>> Many users of kernel async. crypto services have a pattern of
>>> starting an async. crypto op and than using a completion
>>> to wait for it to end.
>>>
>>> This patch set simplifies this common use case in two ways:
>>>
>>> First, by separating the return codes of the case where a
>>> request is queued to a backlog due to the provider being
>>> busy (-EBUSY) from the case the request has failed due
>>> to the provider being busy and backlogging is not enabled
>>> (-EAGAIN).
>>>
>>> Next, this change is than built on to create a generic API
>>> to wait for a async. crypto operation to complete.
>>>
>>> The end result is a smaller code base and an API that is
>>> easier to use and more difficult to get wrong.
>>>
>>> The patch set was boot tested on x86_64 and arm64 which
>>> at the very least tests the crypto users via testmgr and
>>> tcrypt but I do note that I do not have access to some
>>> of the HW whose drivers are modified nor do I claim I was
>>> able to test all of the corner cases.
>>>
>>> The patch set is based upon linux-next release tagged
>>> next-20170824.
>>>
>>> Changes from v6:
>>> - Fix brown paper bag compile error on marvell/cesa
>>>   code.
>>>
>>> Changes from v5:
>>> - Remove redundant new line as spotted by Jonathan
>>>   Cameron.
>>> - Reworded dm-verity change commit message to better
>>>   clarify potential issue averted by change as
>>>   pointed out by Mikulas Patocka.
>>>
>>> Changes from v4:
>>> - Rebase on top of latest algif changes from Stephan
>>>   Mueller.
>>> - Fix typo in ccp patch title.
>>>
>>> Changes from v3:
>>> - Instead of changing the return code to indicate
>>>   backlog queueing, change the return code to indicate
>>>   transient busy state, as suggested by Herbert Xu.
>>>
>>> Changes from v2:
>>> - Patch title changed from "introduce crypto wait for
>>>   async op" to better reflect the current state.
>>> - Rebase on top of latest linux-next.
>>> - Add a new return code of -EIOCBQUEUED for backlog
>>>   queueing, as suggested by Herbert Xu.
>>> - Transform more users to the new API.
>>> - Update the drbg change to account for new init as
>>>   indicated by Stephan Muller.
>>>
>>> Changes from v1:
>>> - Address review comments from Eric Biggers.
>>> - Separated out bug fixes of existing code and rebase
>>>   on top of that patch set.
>>> - Rename 'ecr' to 'wait' in fscrypto code.
>>> - Split patch introducing the new API from the change
>>>   moving over the algif code which it originated from
>>>   to the new API.
>>> - Inline crypto_wait_req().
>>> - Some code indentation fixes.
>>>
>>> Gilad Ben-Yossef (19):
>>>   crypto: change transient busy return code to -EAGAIN
>>>   crypto: ccp: use -EAGAIN for transient busy indication
>>>   crypto: remove redundant backlog checks on EBUSY
>>>   crypto: marvell/cesa: remove redundant backlog checks on EBUSY
>>>   crypto: introduce crypto wait for async op
>>>   crypto: move algif to generic async completion
>>>   crypto: move pub key to generic async completion
>>>   crypto: move drbg to generic async completion
>>>   crypto: move gcm to generic async completion
>>>   crypto: move testmgr to generic async completion
>>>   fscrypt: move to generic async completion
>>>   dm: move dm-verity to generic async completion
>>>   cifs: move to generic async completion
>>>   ima: move to generic async completion
>>>   crypto: tcrypt: move to generic async completion
>>>   crypto: talitos: move to generic async completion
>>>   crypto: qce: move to generic async completion
>>>   crypto: mediatek: move to generic async completion
>>>   crypto: adapt api sample to use async. op wait
>>>
>>>  Documentation/crypto/api-samples.rst |  52 ++---
>>>  crypto/af_alg.c  |  27 -
>>>  crypto/ahash.c   |  12 +--
>>>  crypto/algapi.c  |   6 +-
>>>  crypto/algif_aead.c  |   8 +-
>>>  crypto/algif_hash.c  |  50 +
>>>  crypto/algif_skcipher.c  |   9 +-
>>>  

Re: [PATCH v7 00/19] simplify crypto wait for async op

[Gilad Ben-Yossef]

On Thu, Aug 31, 2017 at 3:31 PM, Harsh Jain  wrote:
> HI Gilad,
>
> I think we need an update in ESP also. Now EBUSY return means driver
> has accepted, Packet should not be dropped in
>
> esp_output_tail() function.

Good catch. You are right and the same holds true for ah_output() in ah4.c.

But I do wonder, the code there now treats -EBUSY as a special case
and returns NET_XMIT_DROP
but if an AEAD or AHASH transformation return some other error, like
-ENOMEM or -EINVAL shouldn't
we return NET_XMIT_DROP in that case too?

Any ideas?

Gilad

>
>
> On Thu, Aug 24, 2017 at 7:48 PM, Gilad Ben-Yossef  wrote:
>> Many users of kernel async. crypto services have a pattern of
>> starting an async. crypto op and than using a completion
>> to wait for it to end.
>>
>> This patch set simplifies this common use case in two ways:
>>
>> First, by separating the return codes of the case where a
>> request is queued to a backlog due to the provider being
>> busy (-EBUSY) from the case the request has failed due
>> to the provider being busy and backlogging is not enabled
>> (-EAGAIN).
>>
>> Next, this change is than built on to create a generic API
>> to wait for a async. crypto operation to complete.
>>
>> The end result is a smaller code base and an API that is
>> easier to use and more difficult to get wrong.
>>
>> The patch set was boot tested on x86_64 and arm64 which
>> at the very least tests the crypto users via testmgr and
>> tcrypt but I do note that I do not have access to some
>> of the HW whose drivers are modified nor do I claim I was
>> able to test all of the corner cases.
>>
>> The patch set is based upon linux-next release tagged
>> next-20170824.
>>
>> Changes from v6:
>> - Fix brown paper bag compile error on marvell/cesa
>>   code.
>>
>> Changes from v5:
>> - Remove redundant new line as spotted by Jonathan
>>   Cameron.
>> - Reworded dm-verity change commit message to better
>>   clarify potential issue averted by change as
>>   pointed out by Mikulas Patocka.
>>
>> Changes from v4:
>> - Rebase on top of latest algif changes from Stephan
>>   Mueller.
>> - Fix typo in ccp patch title.
>>
>> Changes from v3:
>> - Instead of changing the return code to indicate
>>   backlog queueing, change the return code to indicate
>>   transient busy state, as suggested by Herbert Xu.
>>
>> Changes from v2:
>> - Patch title changed from "introduce crypto wait for
>>   async op" to better reflect the current state.
>> - Rebase on top of latest linux-next.
>> - Add a new return code of -EIOCBQUEUED for backlog
>>   queueing, as suggested by Herbert Xu.
>> - Transform more users to the new API.
>> - Update the drbg change to account for new init as
>>   indicated by Stephan Muller.
>>
>> Changes from v1:
>> - Address review comments from Eric Biggers.
>> - Separated out bug fixes of existing code and rebase
>>   on top of that patch set.
>> - Rename 'ecr' to 'wait' in fscrypto code.
>> - Split patch introducing the new API from the change
>>   moving over the algif code which it originated from
>>   to the new API.
>> - Inline crypto_wait_req().
>> - Some code indentation fixes.
>>
>> Gilad Ben-Yossef (19):
>>   crypto: change transient busy return code to -EAGAIN
>>   crypto: ccp: use -EAGAIN for transient busy indication
>>   crypto: remove redundant backlog checks on EBUSY
>>   crypto: marvell/cesa: remove redundant backlog checks on EBUSY
>>   crypto: introduce crypto wait for async op
>>   crypto: move algif to generic async completion
>>   crypto: move pub key to generic async completion
>>   crypto: move drbg to generic async completion
>>   crypto: move gcm to generic async completion
>>   crypto: move testmgr to generic async completion
>>   fscrypt: move to generic async completion
>>   dm: move dm-verity to generic async completion
>>   cifs: move to generic async completion
>>   ima: move to generic async completion
>>   crypto: tcrypt: move to generic async completion
>>   crypto: talitos: move to generic async completion
>>   crypto: qce: move to generic async completion
>>   crypto: mediatek: move to generic async completion
>>   crypto: adapt api sample to use async. op wait
>>
>>  Documentation/crypto/api-samples.rst |  52 ++---
>>  crypto/af_alg.c  |  27 -
>>  crypto/ahash.c   |  12 +--
>>  crypto/algapi.c  |   6 +-
>>  crypto/algif_aead.c  |   8 +-
>>  crypto/algif_hash.c  |  50 +
>>  crypto/algif_skcipher.c  |   9 +-
>>  crypto/api.c |  13 +++
>>  crypto/asymmetric_keys/public_key.c  |  28 +
>>  crypto/cryptd.c  |   4 +-
>>  crypto/cts.c |   6 +-
>>  crypto/drbg.c|  36 ++-
>>  crypto/gcm.c |  32 ++
>>  crypto/lrw.c |   8 +-
>>  crypto/rsa-pkcs1pad.c|  16 +--
>>  

Re: [PATCH v7 00/19] simplify crypto wait for async op

[Harsh Jain]

HI Gilad,

I think we need an update in ESP also. Now EBUSY return means driver
has accepted, Packet should not be dropped in

esp_output_tail() function.

.

Regards
Harsh Jain



On Thu, Aug 24, 2017 at 7:48 PM, Gilad Ben-Yossef  wrote:
> Many users of kernel async. crypto services have a pattern of
> starting an async. crypto op and than using a completion
> to wait for it to end.
>
> This patch set simplifies this common use case in two ways:
>
> First, by separating the return codes of the case where a
> request is queued to a backlog due to the provider being
> busy (-EBUSY) from the case the request has failed due
> to the provider being busy and backlogging is not enabled
> (-EAGAIN).
>
> Next, this change is than built on to create a generic API
> to wait for a async. crypto operation to complete.
>
> The end result is a smaller code base and an API that is
> easier to use and more difficult to get wrong.
>
> The patch set was boot tested on x86_64 and arm64 which
> at the very least tests the crypto users via testmgr and
> tcrypt but I do note that I do not have access to some
> of the HW whose drivers are modified nor do I claim I was
> able to test all of the corner cases.
>
> The patch set is based upon linux-next release tagged
> next-20170824.
>
> Changes from v6:
> - Fix brown paper bag compile error on marvell/cesa
>   code.
>
> Changes from v5:
> - Remove redundant new line as spotted by Jonathan
>   Cameron.
> - Reworded dm-verity change commit message to better
>   clarify potential issue averted by change as
>   pointed out by Mikulas Patocka.
>
> Changes from v4:
> - Rebase on top of latest algif changes from Stephan
>   Mueller.
> - Fix typo in ccp patch title.
>
> Changes from v3:
> - Instead of changing the return code to indicate
>   backlog queueing, change the return code to indicate
>   transient busy state, as suggested by Herbert Xu.
>
> Changes from v2:
> - Patch title changed from "introduce crypto wait for
>   async op" to better reflect the current state.
> - Rebase on top of latest linux-next.
> - Add a new return code of -EIOCBQUEUED for backlog
>   queueing, as suggested by Herbert Xu.
> - Transform more users to the new API.
> - Update the drbg change to account for new init as
>   indicated by Stephan Muller.
>
> Changes from v1:
> - Address review comments from Eric Biggers.
> - Separated out bug fixes of existing code and rebase
>   on top of that patch set.
> - Rename 'ecr' to 'wait' in fscrypto code.
> - Split patch introducing the new API from the change
>   moving over the algif code which it originated from
>   to the new API.
> - Inline crypto_wait_req().
> - Some code indentation fixes.
>
> Gilad Ben-Yossef (19):
>   crypto: change transient busy return code to -EAGAIN
>   crypto: ccp: use -EAGAIN for transient busy indication
>   crypto: remove redundant backlog checks on EBUSY
>   crypto: marvell/cesa: remove redundant backlog checks on EBUSY
>   crypto: introduce crypto wait for async op
>   crypto: move algif to generic async completion
>   crypto: move pub key to generic async completion
>   crypto: move drbg to generic async completion
>   crypto: move gcm to generic async completion
>   crypto: move testmgr to generic async completion
>   fscrypt: move to generic async completion
>   dm: move dm-verity to generic async completion
>   cifs: move to generic async completion
>   ima: move to generic async completion
>   crypto: tcrypt: move to generic async completion
>   crypto: talitos: move to generic async completion
>   crypto: qce: move to generic async completion
>   crypto: mediatek: move to generic async completion
>   crypto: adapt api sample to use async. op wait
>
>  Documentation/crypto/api-samples.rst |  52 ++---
>  crypto/af_alg.c  |  27 -
>  crypto/ahash.c   |  12 +--
>  crypto/algapi.c  |   6 +-
>  crypto/algif_aead.c  |   8 +-
>  crypto/algif_hash.c  |  50 +
>  crypto/algif_skcipher.c  |   9 +-
>  crypto/api.c |  13 +++
>  crypto/asymmetric_keys/public_key.c  |  28 +
>  crypto/cryptd.c  |   4 +-
>  crypto/cts.c |   6 +-
>  crypto/drbg.c|  36 ++-
>  crypto/gcm.c |  32 ++
>  crypto/lrw.c |   8 +-
>  crypto/rsa-pkcs1pad.c|  16 +--
>  crypto/tcrypt.c  |  84 +--
>  crypto/testmgr.c | 204 
> ---
>  crypto/xts.c |   8 +-
>  drivers/crypto/ccp/ccp-crypto-main.c |   8 +-
>  drivers/crypto/ccp/ccp-dev.c |   7 +-
>  drivers/crypto/marvell/cesa.c|   3 +-
>  drivers/crypto/marvell/cesa.h|   2 +-
>  drivers/crypto/mediatek/mtk-aes.c|  31 +-
>  drivers/crypto/qce/sha.c |  30 +-
>