[PATCH] crypto: doc - clarify hash callbacks state machine

[PATCH] crypto: doc - clarify hash callbacks state machine

[Horia Geantă]

Even though it doesn't make too much sense, it is perfectly legal to:
- call .init() and then (as many times) .update()
- subseqently _not_ call any of .final(), .finup() or .export()

Update documentation since this is an important issue to consider
from resource management perspective.

Link: https://lkml.kernel.org/r/20180222114741.GA27631@gondor.apana.org.au
Signed-off-by: Horia Geantă <horia.geanta@nxp.com>
---
 Documentation/crypto/devel-algos.rst | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/crypto/devel-algos.rst b/Documentation/crypto/devel-algos.rst
index 66f50d32dcec..0f4617019227 100644
--- a/Documentation/crypto/devel-algos.rst
+++ b/Documentation/crypto/devel-algos.rst
@@ -236,6 +236,14 @@ when used from another part of the kernel.
                                |
                                '---------------> HASH2
 
+Note that it is perfectly legal to:
+- call .init() and then (as many times) .update()
+- subseqently _not_ call any of .final(), .finup() or .export()
+
+In other words mind the resource allocation and clean-up,
+since this basically means no resources can remain allocated
+after a call to .init() or .update().
+
 
 Specifics Of Asynchronous HASH Transformation
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-- 
2.16.2

Re: [PATCH] crypto: doc - clarify hash callbacks state machine

[Herbert Xu]

On Mon, Mar 05, 2018 at 12:39:45PM +0200, Horia Geantă wrote:
> Even though it doesn't make too much sense, it is perfectly legal to:
> - call .init() and then (as many times) .update()
> - subseqently _not_ call any of .final(), .finup() or .export()

Actually it makes perfect sense, because there can be an arbitrary
number of requests for a given tfm.  There is no requirement that
you must finalise the first request before submitting new ones.

IOW there can be an arbitrary number of outstanding requests even
without the user intentionally abandoning any hash request.

So please modify your commit description.

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

Re: [PATCH] crypto: doc - clarify hash callbacks state machine

[Horia Geantă]

On 3/16/2018 5:16 PM, Herbert Xu wrote:
> On Mon, Mar 05, 2018 at 12:39:45PM +0200, Horia Geantă wrote:
>> Even though it doesn't make too much sense, it is perfectly legal to:
>> - call .init() and then (as many times) .update()
>> - subseqently _not_ call any of .final(), .finup() or .export()
> 
> Actually it makes perfect sense, because there can be an arbitrary
> number of requests for a given tfm.  There is no requirement that
> you must finalise the first request before submitting new ones.
> 
> IOW there can be an arbitrary number of outstanding requests even
> without the user intentionally abandoning any hash request.
> 
The fact that there can be multiple requests in parallel (for a given tfm) is a
different topic.
Each request object has its state in its own state machine, independent from the
other request objects.
I assume this is clear enough.

Why I wanted to underline is that "abandoning" a hash request is allowed (even
though doing this is at least questionable), thus implementations must take
special care not to leak resources in this case.

If you think the commit message should be updated, then probably so should the
documentation update.

Thanks,
Horia

Re: [PATCH] crypto: doc - clarify hash callbacks state machine

[Herbert Xu]

On Mon, Mar 19, 2018 at 06:39:50AM +0000, Horia Geantă wrote:
>
> The fact that there can be multiple requests in parallel (for a given tfm) is a
> different topic.
> Each request object has its state in its own state machine, independent from the
> other request objects.
> I assume this is clear enough.

My point is that all of the state associated with a request needs
to be stored in the request object.  If you're start storing things
in the driver/hardware, then things will get ugly one way or another.

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: [PATCH] crypto: doc - clarify hash callbacks state machine

[Horia Geantă]

On 3/19/2018 11:25 AM, Herbert Xu wrote:
> On Mon, Mar 19, 2018 at 06:39:50AM +0000, Horia Geantă wrote:
>>
>> The fact that there can be multiple requests in parallel (for a given tfm) is a
>> different topic.
>> Each request object has its state in its own state machine, independent from the
>> other request objects.
>> I assume this is clear enough.
> 
> My point is that all of the state associated with a request needs
> to be stored in the request object.  If you're start storing things
> in the driver/hardware, then things will get ugly one way or another.
> 
Agree, the request state should be stored in the request object; I am not
debating that.

Still there are limitations even when keeping state in the request object.
For e.g. an implementation cannot DMA map a buffer for the entire lifetime of a
request object, because this lifetime is unknown - user can "abandon" the object
after a few .update() calls, or even after .init(). By "abandon" I mean not call
_ever_ any of .final(), .finup() or .export() on the object.

The only solution to avoid leaks in this case is to repeatedly DMA map & unmap
the buffer.
IOW, if one wants to load/save HW state in a buffer after an .update() and to
instruct the crypto engine to do this operation, the following steps are involved:
-gpp: DMA map the buffer, get its IOVA
-gpp: program the crypto engine with IOVA, wait for crypto engine's signal
-crypto engine: load HW state from buffer, perform the partial hash, save HW
state in buffer, signal gpp
-gpp: DMA unmap the buffer

I'd say this is pretty inefficient, yet I don't see an alternative.

Good or bad, the documentation should reflect this limitation - hence this patch.

Thanks,
Horia

Re: [PATCH] crypto: doc - clarify hash callbacks state machine

[Herbert Xu]

On Mon, Mar 19, 2018 at 11:04:24AM +0000, Horia Geantă wrote:
>
> The only solution to avoid leaks in this case is to repeatedly DMA map & unmap
> the buffer.
> IOW, if one wants to load/save HW state in a buffer after an .update() and to
> instruct the crypto engine to do this operation, the following steps are involved:
> -gpp: DMA map the buffer, get its IOVA
> -gpp: program the crypto engine with IOVA, wait for crypto engine's signal
> -crypto engine: load HW state from buffer, perform the partial hash, save HW
> state in buffer, signal gpp
> -gpp: DMA unmap the buffer

What buffer are you talking about here? Is it the hash state?

If it's the hash state and assuming DMA mapping was slow enough
on your platform, you could solve it by maintaining a fixed set
of hash states that are rotated through the actual hash requests.

Let's say you allocate n such hash states which are always mapped,
then if there are less than n hash requests outstanding, you could
directly use the mapped hash states in the requests.

If there are more than n, then you simply copy in/copy out for each
hash operation.

The number n limits how many operations can be pending to be
processed by the hardware.

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

[PATCH v2] crypto: doc - clarify hash callbacks state machine

[Horia Geantă]

Add a note that it is perfectly legal to "abandon" a request object:
- call .init() and then (as many times) .update()
- _not_ call any of .final(), .finup() or .export() at any point in
  future

Link: https://lkml.kernel.org/r/20180222114741.GA27631@gondor.apana.org.au
Signed-off-by: Horia Geantă <horia.geanta@nxp.com>
---
 Documentation/crypto/devel-algos.rst | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/crypto/devel-algos.rst b/Documentation/crypto/devel-algos.rst
index 66f50d32dcec..c45c6f400dbd 100644
--- a/Documentation/crypto/devel-algos.rst
+++ b/Documentation/crypto/devel-algos.rst
@@ -236,6 +236,14 @@ when used from another part of the kernel.
                                |
                                '---------------> HASH2
 
+Note that it is perfectly legal to "abandon" a request object:
+- call .init() and then (as many times) .update()
+- _not_ call any of .final(), .finup() or .export() at any point in future
+
+In other words implementations should mind the resource allocation and clean-up.
+No resources related to request objects should remain allocated after a call
+to .init() or .update(), since there might be no chance to free them.
+
 
 Specifics Of Asynchronous HASH Transformation
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-- 
2.16.2

Re: [PATCH v2] crypto: doc - clarify hash callbacks state machine

[Kamil Konieczny]

On 20.03.2018 08:56, Horia Geantă wrote:
> Add a note that it is perfectly legal to "abandon" a request object:
> - call .init() and then (as many times) .update()
> - _not_ call any of .final(), .finup() or .export() at any point in
>   future
> 
> Link: https://lkml.kernel.org/r/20180222114741.GA27631@gondor.apana.org.au
> Signed-off-by: Horia Geantă <horia.geanta@nxp.com>
> ---
>  Documentation/crypto/devel-algos.rst | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/crypto/devel-algos.rst b/Documentation/crypto/devel-algos.rst
> index 66f50d32dcec..c45c6f400dbd 100644
> --- a/Documentation/crypto/devel-algos.rst
> +++ b/Documentation/crypto/devel-algos.rst
> @@ -236,6 +236,14 @@ when used from another part of the kernel.
>                                 |
>                                 '---------------> HASH2
>  
> +Note that it is perfectly legal to "abandon" a request object:
> +- call .init() and then (as many times) .update()
> +- _not_ call any of .final(), .finup() or .export() at any point in future
> +
> +In other words implementations should mind the resource allocation and clean-up.
> +No resources related to request objects should remain allocated after a call
-- ^^^^^^^^^^^^
> +to .init() or .update(), since there might be no chance to free them.

is it for crypto api  users or for drivers ?

the creator of request context is responsible for alloc and destroy,
so why there are no chance of free ?

-- 
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland

Re: [PATCH v2] crypto: doc - clarify hash callbacks state machine

[Horia Geantă]

On 3/20/2018 10:50 AM, Kamil Konieczny wrote:
> On 20.03.2018 08:56, Horia Geantă wrote:
>> Add a note that it is perfectly legal to "abandon" a request object:
>> - call .init() and then (as many times) .update()
>> - _not_ call any of .final(), .finup() or .export() at any point in
>>   future
>>
>> Link: https://lkml.kernel.org/r/20180222114741.ga27...@gondor.apana.org.au
>> Signed-off-by: Horia Geantă <horia.geanta@nxp.com>
>> ---
>>  Documentation/crypto/devel-algos.rst | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/Documentation/crypto/devel-algos.rst b/Documentation/crypto/devel-algos.rst
>> index 66f50d32dcec..c45c6f400dbd 100644
>> --- a/Documentation/crypto/devel-algos.rst
>> +++ b/Documentation/crypto/devel-algos.rst
>> @@ -236,6 +236,14 @@ when used from another part of the kernel.
>>                                 |
>>                                 '---------------> HASH2
>>  
>> +Note that it is perfectly legal to "abandon" a request object:
>> +- call .init() and then (as many times) .update()
>> +- _not_ call any of .final(), .finup() or .export() at any point in future
>> +
>> +In other words implementations should mind the resource allocation and clean-up.
>> +No resources related to request objects should remain allocated after a call
> -- ^^^^^^^^^^^^
>> +to .init() or .update(), since there might be no chance to free them.
> 
> is it for crypto api  users or for drivers ?
> 
For drivers / providers (below crypto API).

> the creator of request context is responsible for alloc and destroy,
> so why there are no chance of free ?
> 
Hash request object (including request context) is allocated by the user /
client by means of ahash_request_alloc(), and later on freed using
ahash_request_free().
I don't see a problem with this.

However, besides the memory allocated for the request context, other resources
(related to the request) might be needed by the driver.
I provided an example of needing to DMA map a buffer (to load/store HW state
from/to crypto engine), and I am not happy with either solutions:
-DMA map & unmap after each .update()
-Herbert's proposal to use a convoluted DMA mapping scheme

Another example: dynamic memory allocation might be needed beyond what's
available in request context, i.e. driver might not have apriori all the
information needed to inform the tfm about required memory using
crypto_ahash_set_reqsize().

This happens due to the semantics of the crypto API, which allows the user to
initialize a request object and drop it without getting a result (final or
partial hash).
I don't see what below use case is good for, maybe just for benchmarking:
req = ahash_request_alloc();
[...]
crypto_ahash_init(req);
crypto_ahash_update(req);
ahash_request_free(req);

Horia

Re: [PATCH v2] crypto: doc - clarify hash callbacks state machine

[Herbert Xu]

On Tue, Mar 20, 2018 at 09:56:12AM +0200, Horia Geantă wrote:
> Add a note that it is perfectly legal to "abandon" a request object:
> - call .init() and then (as many times) .update()
> - _not_ call any of .final(), .finup() or .export() at any point in
>   future
> 
> Link: https://lkml.kernel.org/r/20180222114741.GA27631@gondor.apana.org.au
> Signed-off-by: Horia Geantă <horia.geanta@nxp.com>

Patch applied.  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