First four patches add empty hash export and import functions to each driver,
with the same behaviour as in crypto framework. The last one drops them from
crypto framework. Last one for ahash.c depends on all previous.
Changes in v3:
added change for bfin_crc.c
make this a patchset, instead of unreleated patches
make commit message more descriptive
Kamil Konieczny (5):
crypto: mxs-dcp: Add empty hash export and import
crypto: n2_core: Add empty hash export and import
crypto: ux500/hash: Add empty export and import
crypto: bfin_crc: Add empty hash export and import
crypto: ahash.c: Require export/import in ahash
crypto/ahash.c | 18 ++----------------
drivers/crypto/bfin_crc.c | 12 ++++++++++++
drivers/crypto/mxs-dcp.c | 14 ++++++++++++++
drivers/crypto/n2_core.c | 12 ++++++++++++
drivers/crypto/ux500/hash/hash_core.c | 18 ++++++++++++++++++
5 files changed, 58 insertions(+), 16 deletions(-)
--
2.15.0
Crypto framework requires export/import in async hash. If driver do not
implement them, wrapper functions in framework will be used, and it will
cause error during ahash alg registration (unless one disables crypto
internal tests). To make change in framework and expose this requirement,
I will remove wrappers from crypto/ahash.c , but this can broke code which
depends on them. Add empty hash export and import, with the same behaviour
as in framework and expose this directly in driver. This can also prevent
OOPS when config option in Cryptographic API 'Disable run-time self tests'
will be enabled.
Signed-off-by: Kamil Konieczny <[email protected]>
---
drivers/crypto/mxs-dcp.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/crypto/mxs-dcp.c b/drivers/crypto/mxs-dcp.c
index 764be3e6933c..a10c418d4e5c 100644
--- a/drivers/crypto/mxs-dcp.c
+++ b/drivers/crypto/mxs-dcp.c
@@ -759,6 +759,16 @@ static int dcp_sha_digest(struct ahash_request *req)
return dcp_sha_finup(req);
}
+static int dcp_sha_noimport(struct ahash_request *req, const void *in)
+{
+ return -ENOSYS;
+}
+
+static int dcp_sha_noexport(struct ahash_request *req, void *out)
+{
+ return -ENOSYS;
+}
+
static int dcp_sha_cra_init(struct crypto_tfm *tfm)
{
crypto_ahash_set_reqsize(__crypto_ahash_cast(tfm),
@@ -829,6 +839,8 @@ static struct ahash_alg dcp_sha1_alg = {
.final = dcp_sha_final,
.finup = dcp_sha_finup,
.digest = dcp_sha_digest,
+ .import = dcp_sha_noimport,
+ .export = dcp_sha_noexport,
.halg = {
.digestsize = SHA1_DIGEST_SIZE,
.base = {
@@ -853,6 +865,8 @@ static struct ahash_alg dcp_sha256_alg = {
.final = dcp_sha_final,
.finup = dcp_sha_finup,
.digest = dcp_sha_digest,
+ .import = dcp_sha_noimport,
+ .export = dcp_sha_noexport,
.halg = {
.digestsize = SHA256_DIGEST_SIZE,
.base = {
--
2.15.0
Crypto framework requires export/import in async hash. If driver do not
implement them, wrapper functions in framework will be used, and it will
cause error during ahash alg registration (unless one disables crypto
internal tests). To make change in framework and expose this requirement,
I will remove wrappers from crypto/ahash.c , but this can broke code which
depends on them. Add empty hash export and import, with the same behaviour
as in framework and expose this directly in driver. This can also prevent
OOPS when config option in Cryptographic API 'Disable run-time self tests'
will be enabled.
Signed-off-by: Kamil Konieczny <[email protected]>
---
drivers/crypto/n2_core.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/crypto/n2_core.c b/drivers/crypto/n2_core.c
index 662e709812cc..80e9c842aad4 100644
--- a/drivers/crypto/n2_core.c
+++ b/drivers/crypto/n2_core.c
@@ -359,6 +359,16 @@ static int n2_hash_async_finup(struct ahash_request *req)
return crypto_ahash_finup(&rctx->fallback_req);
}
+static int n2_hash_async_noimport(struct ahash_request *req, const void *in)
+{
+ return -ENOSYS;
+}
+
+static int n2_hash_async_noexport(struct ahash_request *req, void *out)
+{
+ return -ENOSYS;
+}
+
static int n2_hash_cra_init(struct crypto_tfm *tfm)
{
const char *fallback_driver_name = crypto_tfm_alg_name(tfm);
@@ -1467,6 +1477,8 @@ static int __n2_register_one_ahash(const struct n2_hash_tmpl *tmpl)
ahash->final = n2_hash_async_final;
ahash->finup = n2_hash_async_finup;
ahash->digest = n2_hash_async_digest;
+ ahash->export = n2_hash_async_noexport;
+ ahash->import = n2_hash_async_noimport;
halg = &ahash->halg;
halg->digestsize = tmpl->digest_size;
--
2.15.0
Export and import are mandatory in async hash. As drivers were
rewritten, drop empty wrappers and correct init of ahash transformation.
Signed-off-by: Kamil Konieczny <[email protected]>
---
crypto/ahash.c | 18 ++----------------
1 file changed, 2 insertions(+), 16 deletions(-)
diff --git a/crypto/ahash.c b/crypto/ahash.c
index 3a35d67de7d9..c3cce508c1d4 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -434,16 +434,6 @@ static int ahash_def_finup(struct ahash_request *req)
return ahash_def_finup_finish1(req, err);
}
-static int ahash_no_export(struct ahash_request *req, void *out)
-{
- return -ENOSYS;
-}
-
-static int ahash_no_import(struct ahash_request *req, const void *in)
-{
- return -ENOSYS;
-}
-
static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
{
struct crypto_ahash *hash = __crypto_ahash_cast(tfm);
@@ -451,8 +441,6 @@ static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
hash->setkey = ahash_nosetkey;
hash->has_setkey = false;
- hash->export = ahash_no_export;
- hash->import = ahash_no_import;
if (tfm->__crt_alg->cra_type != &crypto_ahash_type)
return crypto_init_shash_ops_async(tfm);
@@ -462,15 +450,13 @@ static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
hash->final = alg->final;
hash->finup = alg->finup ?: ahash_def_finup;
hash->digest = alg->digest;
+ hash->export = alg->export;
+ hash->import = alg->import;
if (alg->setkey) {
hash->setkey = alg->setkey;
hash->has_setkey = true;
}
- if (alg->export)
- hash->export = alg->export;
- if (alg->import)
- hash->import = alg->import;
return 0;
}
--
2.15.0
Crypto framework requires export/import in async hash. If driver do not
implement them, wrapper functions in framework will be used, and it will
cause error during ahash alg registration (unless one disables crypto
internal tests). To make change in framework and expose this requirement,
I will remove wrappers from crypto/ahash.c , but this can broke code which
depends on them.
Add empty hash export and import, with the same behaviour as in framework
and expose this directly in driver. This can also prevent OOPS when config
option in Cryptographic API 'Disable run-time self tests' will be enabled.
Signed-off-by: Kamil Konieczny <[email protected]>
---
drivers/crypto/bfin_crc.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/crypto/bfin_crc.c b/drivers/crypto/bfin_crc.c
index a118b9bed669..65a8e07835e8 100644
--- a/drivers/crypto/bfin_crc.c
+++ b/drivers/crypto/bfin_crc.c
@@ -450,6 +450,16 @@ static int bfin_crypto_crc_digest(struct ahash_request *req)
return bfin_crypto_crc_finup(req);
}
+static int bfin_crypto_crc_noimport(struct ahash_request *req, const void *in)
+{
+ return -ENOSYS;
+}
+
+static int bfin_crypto_crc_noexport(struct ahash_request *req, void *out)
+{
+ return -ENOSYS;
+}
+
static int bfin_crypto_crc_setkey(struct crypto_ahash *tfm, const u8 *key,
unsigned int keylen)
{
@@ -487,6 +497,8 @@ static struct ahash_alg algs = {
.final = bfin_crypto_crc_final,
.finup = bfin_crypto_crc_finup,
.digest = bfin_crypto_crc_digest,
+ .export = bfin_crypto_crc_noexport,
+ .import = bfin_crypto_crc_noimport,
.setkey = bfin_crypto_crc_setkey,
.halg.digestsize = CHKSUM_DIGEST_SIZE,
.halg.base = {
--
2.15.0
Crypto framework requires export/import in async hash. If driver do not
implement them, wrapper functions in framework will be used, and it will
cause error during ahash alg registration (unless one disables crypto
internal tests). To make change in framework and expose this requirement,
I will remove wrappers from crypto/ahash.c , but this can broke code which
depends on them. Add empty hash export and import, with the same behaviour
as in framework and expose this directly in driver. This can also prevent
OOPS when config option in Cryptographic API 'Disable run-time self tests'
will be enabled.
Signed-off-by: Kamil Konieczny <[email protected]>
Acked-by: Linus Walleij <[email protected]>
---
drivers/crypto/ux500/hash/hash_core.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/crypto/ux500/hash/hash_core.c b/drivers/crypto/ux500/hash/hash_core.c
index 9acccad26928..2d0a677bcc76 100644
--- a/drivers/crypto/ux500/hash/hash_core.c
+++ b/drivers/crypto/ux500/hash/hash_core.c
@@ -1403,6 +1403,16 @@ static int ahash_sha256_digest(struct ahash_request *req)
return ret1 ? ret1 : ret2;
}
+static int ahash_noimport(struct ahash_request *req, const void *in)
+{
+ return -ENOSYS;
+}
+
+static int ahash_noexport(struct ahash_request *req, void *out)
+{
+ return -ENOSYS;
+}
+
static int hmac_sha1_init(struct ahash_request *req)
{
struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
@@ -1507,6 +1517,8 @@ static struct hash_algo_template hash_algs[] = {
.update = ahash_update,
.final = ahash_final,
.digest = ahash_sha1_digest,
+ .export = ahash_noexport,
+ .import = ahash_noimport,
.halg.digestsize = SHA1_DIGEST_SIZE,
.halg.statesize = sizeof(struct hash_ctx),
.halg.base = {
@@ -1529,6 +1541,8 @@ static struct hash_algo_template hash_algs[] = {
.update = ahash_update,
.final = ahash_final,
.digest = ahash_sha256_digest,
+ .export = ahash_noexport,
+ .import = ahash_noimport,
.halg.digestsize = SHA256_DIGEST_SIZE,
.halg.statesize = sizeof(struct hash_ctx),
.halg.base = {
@@ -1553,6 +1567,8 @@ static struct hash_algo_template hash_algs[] = {
.final = ahash_final,
.digest = hmac_sha1_digest,
.setkey = hmac_sha1_setkey,
+ .export = ahash_noexport,
+ .import = ahash_noimport,
.halg.digestsize = SHA1_DIGEST_SIZE,
.halg.statesize = sizeof(struct hash_ctx),
.halg.base = {
@@ -1577,6 +1593,8 @@ static struct hash_algo_template hash_algs[] = {
.final = ahash_final,
.digest = hmac_sha256_digest,
.setkey = hmac_sha256_setkey,
+ .export = ahash_noexport,
+ .import = ahash_noimport,
.halg.digestsize = SHA256_DIGEST_SIZE,
.halg.statesize = sizeof(struct hash_ctx),
.halg.base = {
--
2.15.0
On 01/18/2018 07:34 PM, Kamil Konieczny wrote:
> Export and import are mandatory in async hash. As drivers were
> rewritten, drop empty wrappers and correct init of ahash transformation.
Are you moving checks from the core subsystem to drivers ? This looks
really nonsensical and the commit message doesn't explain the rationale
for that at all.
> Signed-off-by: Kamil Konieczny <[email protected]>
> ---
> crypto/ahash.c | 18 ++----------------
> 1 file changed, 2 insertions(+), 16 deletions(-)
>
> diff --git a/crypto/ahash.c b/crypto/ahash.c
> index 3a35d67de7d9..c3cce508c1d4 100644
> --- a/crypto/ahash.c
> +++ b/crypto/ahash.c
> @@ -434,16 +434,6 @@ static int ahash_def_finup(struct ahash_request *req)
> return ahash_def_finup_finish1(req, err);
> }
>
> -static int ahash_no_export(struct ahash_request *req, void *out)
> -{
> - return -ENOSYS;
> -}
> -
> -static int ahash_no_import(struct ahash_request *req, const void *in)
> -{
> - return -ENOSYS;
> -}
> -
> static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
> {
> struct crypto_ahash *hash = __crypto_ahash_cast(tfm);
> @@ -451,8 +441,6 @@ static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
>
> hash->setkey = ahash_nosetkey;
> hash->has_setkey = false;
> - hash->export = ahash_no_export;
> - hash->import = ahash_no_import;
>
> if (tfm->__crt_alg->cra_type != &crypto_ahash_type)
> return crypto_init_shash_ops_async(tfm);
> @@ -462,15 +450,13 @@ static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
> hash->final = alg->final;
> hash->finup = alg->finup ?: ahash_def_finup;
> hash->digest = alg->digest;
> + hash->export = alg->export;
> + hash->import = alg->import;
>
> if (alg->setkey) {
> hash->setkey = alg->setkey;
> hash->has_setkey = true;
> }
> - if (alg->export)
> - hash->export = alg->export;
> - if (alg->import)
> - hash->import = alg->import;
>
> return 0;
> }
>
--
Best regards,
Marek Vasut
On 18.01.2018 22:31, Marek Vasut wrote:
> On 01/18/2018 07:34 PM, Kamil Konieczny wrote:
>> Export and import are mandatory in async hash. As drivers were
>> rewritten, drop empty wrappers and correct init of ahash transformation.
>
> Are you moving checks from the core subsystem to drivers ? This looks
> really nonsensical and the commit message doesn't explain the rationale
> for that at all.
I am removing checks from core. Export and import were optional in beginnig
of crypto framework, but as time goes on they become mandatory.
>
>> Signed-off-by: Kamil Konieczny <[email protected]>
>> ---
>> crypto/ahash.c | 18 ++----------------
>> 1 file changed, 2 insertions(+), 16 deletions(-)
>>
>> diff --git a/crypto/ahash.c b/crypto/ahash.c
>> index 3a35d67de7d9..c3cce508c1d4 100644
>> --- a/crypto/ahash.c
>> +++ b/crypto/ahash.c
>> @@ -434,16 +434,6 @@ static int ahash_def_finup(struct ahash_request *req)
>> return ahash_def_finup_finish1(req, err);
>> }
>>
>> -static int ahash_no_export(struct ahash_request *req, void *out)
>> -{
>> - return -ENOSYS;
>> -}
>> -
>> -static int ahash_no_import(struct ahash_request *req, const void *in)
>> -{
>> - return -ENOSYS;
>> -}
>> -
>> static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
>> {
>> struct crypto_ahash *hash = __crypto_ahash_cast(tfm);
>> @@ -451,8 +441,6 @@ static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
>>
>> hash->setkey = ahash_nosetkey;
>> hash->has_setkey = false;
>> - hash->export = ahash_no_export;
>> - hash->import = ahash_no_import;
>>
>> if (tfm->__crt_alg->cra_type != &crypto_ahash_type)
>> return crypto_init_shash_ops_async(tfm);
>> @@ -462,15 +450,13 @@ static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
>> hash->final = alg->final;
>> hash->finup = alg->finup ?: ahash_def_finup;
>> hash->digest = alg->digest;
>> + hash->export = alg->export;
>> + hash->import = alg->import;
>>
>> if (alg->setkey) {
>> hash->setkey = alg->setkey;
>> hash->has_setkey = true;
>> }
>> - if (alg->export)
>> - hash->export = alg->export;
>> - if (alg->import)
>> - hash->import = alg->import;
>>
>> return 0;
>> }
>>
>
>
--
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
On 01/19/2018 10:53 AM, Kamil Konieczny wrote:
> On 18.01.2018 22:31, Marek Vasut wrote:
>> On 01/18/2018 07:34 PM, Kamil Konieczny wrote:
>>> Export and import are mandatory in async hash. As drivers were
>>> rewritten, drop empty wrappers and correct init of ahash transformation.
>>
>> Are you moving checks from the core subsystem to drivers ? This looks
>> really nonsensical and the commit message doesn't explain the rationale
>> for that at all.
>
> I am removing checks from core. Export and import were optional in beginnig
> of crypto framework, but as time goes on they become mandatory.
Seems like if the driver doesn't implement those, the core can easily
detect that and perform the necessary action. Moving the checks out of
core seems like the wrong thing to do, rather you should enhance the
checks in core if they're insufficient in my opinion.
>>
>>> Signed-off-by: Kamil Konieczny <[email protected]>
>>> ---
>>> crypto/ahash.c | 18 ++----------------
>>> 1 file changed, 2 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/crypto/ahash.c b/crypto/ahash.c
>>> index 3a35d67de7d9..c3cce508c1d4 100644
>>> --- a/crypto/ahash.c
>>> +++ b/crypto/ahash.c
>>> @@ -434,16 +434,6 @@ static int ahash_def_finup(struct ahash_request *req)
>>> return ahash_def_finup_finish1(req, err);
>>> }
>>>
>>> -static int ahash_no_export(struct ahash_request *req, void *out)
>>> -{
>>> - return -ENOSYS;
>>> -}
>>> -
>>> -static int ahash_no_import(struct ahash_request *req, const void *in)
>>> -{
>>> - return -ENOSYS;
>>> -}
>>> -
>>> static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
>>> {
>>> struct crypto_ahash *hash = __crypto_ahash_cast(tfm);
>>> @@ -451,8 +441,6 @@ static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
>>>
>>> hash->setkey = ahash_nosetkey;
>>> hash->has_setkey = false;
>>> - hash->export = ahash_no_export;
>>> - hash->import = ahash_no_import;
>>>
>>> if (tfm->__crt_alg->cra_type != &crypto_ahash_type)
>>> return crypto_init_shash_ops_async(tfm);
>>> @@ -462,15 +450,13 @@ static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
>>> hash->final = alg->final;
>>> hash->finup = alg->finup ?: ahash_def_finup;
>>> hash->digest = alg->digest;
>>> + hash->export = alg->export;
>>> + hash->import = alg->import;
>>>
>>> if (alg->setkey) {
>>> hash->setkey = alg->setkey;
>>> hash->has_setkey = true;
>>> }
>>> - if (alg->export)
>>> - hash->export = alg->export;
>>> - if (alg->import)
>>> - hash->import = alg->import;
>>>
>>> return 0;
>>> }
>>>
>>
>>
>
--
Best regards,
Marek Vasut
On 19.01.2018 11:08, Marek Vasut wrote:
> On 01/19/2018 10:53 AM, Kamil Konieczny wrote:
>> On 18.01.2018 22:31, Marek Vasut wrote:
>>> On 01/18/2018 07:34 PM, Kamil Konieczny wrote:
>>>> Export and import are mandatory in async hash. As drivers were
>>>> rewritten, drop empty wrappers and correct init of ahash transformation.
>>>
>>> Are you moving checks from the core subsystem to drivers ? This looks
>>> really nonsensical and the commit message doesn't explain the rationale
>>> for that at all.
>>
>> I am removing checks from core. Export and import were optional in beginnig
>> of crypto framework, but as time goes on they become mandatory.
>
> Seems like if the driver doesn't implement those, the core can easily
> detect that and perform the necessary action. Moving the checks out of
> core seems like the wrong thing to do, rather you should enhance the
> checks in core if they're insufficient in my opinion.
I removed all checks. No checks in driver and no checks in crypto framework.
If you would like any check, I think the place to add them is in ahash alg
registraction, in function ahash_prepare_alg add something like:
if ((alg->init == NULL) ||
(alg->digest == NULL) ||
(alg->final == NULL) ||
(alg->update == NULL) ||
(alg->export == NULL) ||
(alg->import == NULL))
return -EINVAL;
The only downsize is this will be usefull in backport (to prevent NULL dereference),
as any new driver will have all those pointers sets.
>>>> Signed-off-by: Kamil Konieczny <[email protected]>
>>>> ---
>>>> crypto/ahash.c | 18 ++----------------
>>>> 1 file changed, 2 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/crypto/ahash.c b/crypto/ahash.c
>>>> index 3a35d67de7d9..c3cce508c1d4 100644
>>>> --- a/crypto/ahash.c
>>>> +++ b/crypto/ahash.c
>>>> @@ -434,16 +434,6 @@ static int ahash_def_finup(struct ahash_request *req)
>>>> return ahash_def_finup_finish1(req, err);
>>>> }
>>>>
>>>> -static int ahash_no_export(struct ahash_request *req, void *out)
>>>> -{
>>>> - return -ENOSYS;
>>>> -}
>>>> -
>>>> -static int ahash_no_import(struct ahash_request *req, const void *in)
>>>> -{
>>>> - return -ENOSYS;
>>>> -}
>>>> -
>>>> static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
>>>> {
>>>> struct crypto_ahash *hash = __crypto_ahash_cast(tfm);
>>>> @@ -451,8 +441,6 @@ static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
>>>>
>>>> hash->setkey = ahash_nosetkey;
>>>> hash->has_setkey = false;
>>>> - hash->export = ahash_no_export;
>>>> - hash->import = ahash_no_import;
>>>>
>>>> if (tfm->__crt_alg->cra_type != &crypto_ahash_type)
>>>> return crypto_init_shash_ops_async(tfm);
>>>> @@ -462,15 +450,13 @@ static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
>>>> hash->final = alg->final;
>>>> hash->finup = alg->finup ?: ahash_def_finup;
>>>> hash->digest = alg->digest;
>>>> + hash->export = alg->export;
>>>> + hash->import = alg->import;
>>>>
>>>> if (alg->setkey) {
>>>> hash->setkey = alg->setkey;
>>>> hash->has_setkey = true;
>>>> }
>>>> - if (alg->export)
>>>> - hash->export = alg->export;
>>>> - if (alg->import)
>>>> - hash->import = alg->import;
>>>>
>>>> return 0;
>>>> }
>>>>
>>>
>>>
>>
>
>
--
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
On Thu, Jan 18, 2018 at 07:33:59PM +0100, Kamil Konieczny wrote:
> First four patches add empty hash export and import functions to each driver,
> with the same behaviour as in crypto framework. The last one drops them from
> crypto framework. Last one for ahash.c depends on all previous.
>
> Changes in v3:
> added change for bfin_crc.c
> make this a patchset, instead of unreleated patches
> make commit message more descriptive
>
> Kamil Konieczny (5):
> crypto: mxs-dcp: Add empty hash export and import
> crypto: n2_core: Add empty hash export and import
> crypto: ux500/hash: Add empty export and import
> crypto: bfin_crc: Add empty hash export and import
> crypto: ahash.c: Require export/import in ahash
>
> crypto/ahash.c | 18 ++----------------
> drivers/crypto/bfin_crc.c | 12 ++++++++++++
> drivers/crypto/mxs-dcp.c | 14 ++++++++++++++
> drivers/crypto/n2_core.c | 12 ++++++++++++
> drivers/crypto/ux500/hash/hash_core.c | 18 ++++++++++++++++++
> 5 files changed, 58 insertions(+), 16 deletions(-)
All applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On 15.02.2018 18:06, Marek Vasut wrote:
> On 02/15/2018 06:00 PM, Kamil Konieczny wrote:
>>
>>
>> On 15.02.2018 17:27, Marek Vasut wrote:
>>> On 02/15/2018 04:41 PM, Herbert Xu wrote:
>>>> On Thu, Jan 18, 2018 at 07:33:59PM +0100, Kamil Konieczny wrote:
>>>>> First four patches add empty hash export and import functions to each driver,
>>>>> with the same behaviour as in crypto framework. The last one drops them from
>>>>> crypto framework. Last one for ahash.c depends on all previous.
>>>>>
>>>>> Changes in v3:
>>>>> added change for bfin_crc.c
>>>>> make this a patchset, instead of unreleated patches
>>>>> make commit message more descriptive
>>>>>
>>>>> Kamil Konieczny (5):
>>>>> crypto: mxs-dcp: Add empty hash export and import
>>>>> crypto: n2_core: Add empty hash export and import
>>>>> crypto: ux500/hash: Add empty export and import
>>>>> crypto: bfin_crc: Add empty hash export and import
>>>>> crypto: ahash.c: Require export/import in ahash
>>>>>
>>>>> crypto/ahash.c | 18 ++----------------
>>>>> drivers/crypto/bfin_crc.c | 12 ++++++++++++
>>>>> drivers/crypto/mxs-dcp.c | 14 ++++++++++++++
>>>>> drivers/crypto/n2_core.c | 12 ++++++++++++
>>>>> drivers/crypto/ux500/hash/hash_core.c | 18 ++++++++++++++++++
>>>>> 5 files changed, 58 insertions(+), 16 deletions(-)
>>>>
>>>> All applied. Thanks.
>>>
>>> This makes no sense, cfr my comment on 5/5
>>>
>>> Seems like if the driver doesn't implement those, the core can easily
>>> detect that and perform the necessary action. Moving the checks out of
>>> core seems like the wrong thing to do, rather you should enhance the
>>> checks in core if they're insufficient in my opinion.
>>
>> The bug can only be in driver which will not implement those two functions,
>> but we already had all drivers with those due to patches 1..4
>> All other drivers do have them.
>
> The core can very well check if these functions are not populated and
> return ENOSYS
>
>> Additionally, with crypto we want minimize code and run as fast as possible.
>
> So you remove all NULL pointer checks ? Esp. in security-sensitive code?
> What is the impact of this non-critical path code on performance?
>
> Come on ...
>
Why you want checks for something that not exist ?
Those without them will not work and will do Oops in crypto testmgr,
so such drivers should not be used nor accepted in drivers/crypto
Ask yourself why crypto do not check for NULL in ahash digest or other
required ahash functions.
>> Moving checks out of core will impose on driver author need for implement
>> those functions, or declare them empty, but in case of empty ones
>> crypto will not work properly with such driver.
>
> You can very well impose that in the core, except you don't duplicate
> the code.
Now size of crypto core is reduced.
--
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
On 02/15/2018 04:41 PM, Herbert Xu wrote:
> On Thu, Jan 18, 2018 at 07:33:59PM +0100, Kamil Konieczny wrote:
>> First four patches add empty hash export and import functions to each driver,
>> with the same behaviour as in crypto framework. The last one drops them from
>> crypto framework. Last one for ahash.c depends on all previous.
>>
>> Changes in v3:
>> added change for bfin_crc.c
>> make this a patchset, instead of unreleated patches
>> make commit message more descriptive
>>
>> Kamil Konieczny (5):
>> crypto: mxs-dcp: Add empty hash export and import
>> crypto: n2_core: Add empty hash export and import
>> crypto: ux500/hash: Add empty export and import
>> crypto: bfin_crc: Add empty hash export and import
>> crypto: ahash.c: Require export/import in ahash
>>
>> crypto/ahash.c | 18 ++----------------
>> drivers/crypto/bfin_crc.c | 12 ++++++++++++
>> drivers/crypto/mxs-dcp.c | 14 ++++++++++++++
>> drivers/crypto/n2_core.c | 12 ++++++++++++
>> drivers/crypto/ux500/hash/hash_core.c | 18 ++++++++++++++++++
>> 5 files changed, 58 insertions(+), 16 deletions(-)
>
> All applied. Thanks.
This makes no sense, cfr my comment on 5/5
Seems like if the driver doesn't implement those, the core can easily
detect that and perform the necessary action. Moving the checks out of
core seems like the wrong thing to do, rather you should enhance the
checks in core if they're insufficient in my opinion.
--
Best regards,
Marek Vasut
On 15.02.2018 17:27, Marek Vasut wrote:
> On 02/15/2018 04:41 PM, Herbert Xu wrote:
>> On Thu, Jan 18, 2018 at 07:33:59PM +0100, Kamil Konieczny wrote:
>>> First four patches add empty hash export and import functions to each driver,
>>> with the same behaviour as in crypto framework. The last one drops them from
>>> crypto framework. Last one for ahash.c depends on all previous.
>>>
>>> Changes in v3:
>>> added change for bfin_crc.c
>>> make this a patchset, instead of unreleated patches
>>> make commit message more descriptive
>>>
>>> Kamil Konieczny (5):
>>> crypto: mxs-dcp: Add empty hash export and import
>>> crypto: n2_core: Add empty hash export and import
>>> crypto: ux500/hash: Add empty export and import
>>> crypto: bfin_crc: Add empty hash export and import
>>> crypto: ahash.c: Require export/import in ahash
>>>
>>> crypto/ahash.c | 18 ++----------------
>>> drivers/crypto/bfin_crc.c | 12 ++++++++++++
>>> drivers/crypto/mxs-dcp.c | 14 ++++++++++++++
>>> drivers/crypto/n2_core.c | 12 ++++++++++++
>>> drivers/crypto/ux500/hash/hash_core.c | 18 ++++++++++++++++++
>>> 5 files changed, 58 insertions(+), 16 deletions(-)
>>
>> All applied. Thanks.
>
> This makes no sense, cfr my comment on 5/5
>
> Seems like if the driver doesn't implement those, the core can easily
> detect that and perform the necessary action. Moving the checks out of
> core seems like the wrong thing to do, rather you should enhance the
> checks in core if they're insufficient in my opinion.
The bug can only be in driver which will not implement those two functions,
but we already had all drivers with those due to patches 1..4
All other drivers do have them.
Additionally, with crypto we want minimize code and run as fast as possible.
Moving checks out of core will impose on driver author need for implement
those functions, or declare them empty, but in case of empty ones
crypto will not work properly with such driver.
--
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
On 02/15/2018 06:00 PM, Kamil Konieczny wrote:
>
>
> On 15.02.2018 17:27, Marek Vasut wrote:
>> On 02/15/2018 04:41 PM, Herbert Xu wrote:
>>> On Thu, Jan 18, 2018 at 07:33:59PM +0100, Kamil Konieczny wrote:
>>>> First four patches add empty hash export and import functions to each driver,
>>>> with the same behaviour as in crypto framework. The last one drops them from
>>>> crypto framework. Last one for ahash.c depends on all previous.
>>>>
>>>> Changes in v3:
>>>> added change for bfin_crc.c
>>>> make this a patchset, instead of unreleated patches
>>>> make commit message more descriptive
>>>>
>>>> Kamil Konieczny (5):
>>>> crypto: mxs-dcp: Add empty hash export and import
>>>> crypto: n2_core: Add empty hash export and import
>>>> crypto: ux500/hash: Add empty export and import
>>>> crypto: bfin_crc: Add empty hash export and import
>>>> crypto: ahash.c: Require export/import in ahash
>>>>
>>>> crypto/ahash.c | 18 ++----------------
>>>> drivers/crypto/bfin_crc.c | 12 ++++++++++++
>>>> drivers/crypto/mxs-dcp.c | 14 ++++++++++++++
>>>> drivers/crypto/n2_core.c | 12 ++++++++++++
>>>> drivers/crypto/ux500/hash/hash_core.c | 18 ++++++++++++++++++
>>>> 5 files changed, 58 insertions(+), 16 deletions(-)
>>>
>>> All applied. Thanks.
>>
>> This makes no sense, cfr my comment on 5/5
>>
>> Seems like if the driver doesn't implement those, the core can easily
>> detect that and perform the necessary action. Moving the checks out of
>> core seems like the wrong thing to do, rather you should enhance the
>> checks in core if they're insufficient in my opinion.
>
> The bug can only be in driver which will not implement those two functions,
> but we already had all drivers with those due to patches 1..4
> All other drivers do have them.
The core can very well check if these functions are not populated and
return ENOSYS
> Additionally, with crypto we want minimize code and run as fast as possible.
So you remove all NULL pointer checks ? Esp. in security-sensitive code?
What is the impact of this non-critical path code on performance?
Come on ...
> Moving checks out of core will impose on driver author need for implement
> those functions, or declare them empty, but in case of empty ones
> crypto will not work properly with such driver.
You can very well impose that in the core, except you don't duplicate
the code.
--
Best regards,
Marek Vasut
On 02/15/2018 07:06 PM, Kamil Konieczny wrote:
>
>
> On 15.02.2018 18:06, Marek Vasut wrote:
>> On 02/15/2018 06:00 PM, Kamil Konieczny wrote:
>>>
>>>
>>> On 15.02.2018 17:27, Marek Vasut wrote:
>>>> On 02/15/2018 04:41 PM, Herbert Xu wrote:
>>>>> On Thu, Jan 18, 2018 at 07:33:59PM +0100, Kamil Konieczny wrote:
>>>>>> First four patches add empty hash export and import functions to each driver,
>>>>>> with the same behaviour as in crypto framework. The last one drops them from
>>>>>> crypto framework. Last one for ahash.c depends on all previous.
>>>>>>
>>>>>> Changes in v3:
>>>>>> added change for bfin_crc.c
>>>>>> make this a patchset, instead of unreleated patches
>>>>>> make commit message more descriptive
>>>>>>
>>>>>> Kamil Konieczny (5):
>>>>>> crypto: mxs-dcp: Add empty hash export and import
>>>>>> crypto: n2_core: Add empty hash export and import
>>>>>> crypto: ux500/hash: Add empty export and import
>>>>>> crypto: bfin_crc: Add empty hash export and import
>>>>>> crypto: ahash.c: Require export/import in ahash
>>>>>>
>>>>>> crypto/ahash.c | 18 ++----------------
>>>>>> drivers/crypto/bfin_crc.c | 12 ++++++++++++
>>>>>> drivers/crypto/mxs-dcp.c | 14 ++++++++++++++
>>>>>> drivers/crypto/n2_core.c | 12 ++++++++++++
>>>>>> drivers/crypto/ux500/hash/hash_core.c | 18 ++++++++++++++++++
>>>>>> 5 files changed, 58 insertions(+), 16 deletions(-)
>>>>>
>>>>> All applied. Thanks.
>>>>
>>>> This makes no sense, cfr my comment on 5/5
>>>>
>>>> Seems like if the driver doesn't implement those, the core can easily
>>>> detect that and perform the necessary action. Moving the checks out of
>>>> core seems like the wrong thing to do, rather you should enhance the
>>>> checks in core if they're insufficient in my opinion.
>>>
>>> The bug can only be in driver which will not implement those two functions,
>>> but we already had all drivers with those due to patches 1..4
>>> All other drivers do have them.
>>
>> The core can very well check if these functions are not populated and
>> return ENOSYS
>>
>>> Additionally, with crypto we want minimize code and run as fast as possible.
>>
>> So you remove all NULL pointer checks ? Esp. in security-sensitive code?
>> What is the impact of this non-critical path code on performance?
>>
>> Come on ...
>>
>
> Why you want checks for something that not exist ?
>
> Those without them will not work and will do Oops in crypto testmgr,
> so such drivers should not be used nor accepted in drivers/crypto
>
> Ask yourself why crypto do not check for NULL in ahash digest or other
> required ahash functions.
Are you suggesting that the kernel code should NOT perform NULL pointer
checks ?
Are you suggesting each driver should implement every single callback
available and if it is not implemented, return -ENOSYS ? This looks like
a MASSIVE code duplication.
>>> Moving checks out of core will impose on driver author need for implement
>>> those functions, or declare them empty, but in case of empty ones
>>> crypto will not work properly with such driver.
>>
>> You can very well impose that in the core, except you don't duplicate
>> the code.
>
> Now size of crypto core is reduced.
You implemented the same code thrice, it surely is not reduced.
--
Best regards,
Marek Vasut
On 15.02.2018 19:32, Marek Vasut wrote:
> On 02/15/2018 07:06 PM, Kamil Konieczny wrote:
>>
>>
>> On 15.02.2018 18:06, Marek Vasut wrote:
>>> On 02/15/2018 06:00 PM, Kamil Konieczny wrote:
>>>>
>>>>
>>>> On 15.02.2018 17:27, Marek Vasut wrote:
>>>>> On 02/15/2018 04:41 PM, Herbert Xu wrote:
>>>>>> On Thu, Jan 18, 2018 at 07:33:59PM +0100, Kamil Konieczny wrote:
>>>>>>> First four patches add empty hash export and import functions to each driver,
>>>>>>> with the same behaviour as in crypto framework. The last one drops them from
>>>>>>> crypto framework. Last one for ahash.c depends on all previous.
>>>>>>>
>>>>>>> Changes in v3:
>>>>>>> added change for bfin_crc.c
>>>>>>> make this a patchset, instead of unreleated patches
>>>>>>> make commit message more descriptive
>>>>>>>
>>>>>>> Kamil Konieczny (5):
>>>>>>> crypto: mxs-dcp: Add empty hash export and import
>>>>>>> crypto: n2_core: Add empty hash export and import
>>>>>>> crypto: ux500/hash: Add empty export and import
>>>>>>> crypto: bfin_crc: Add empty hash export and import
>>>>>>> crypto: ahash.c: Require export/import in ahash
>>>>>>>
>>>>>>> crypto/ahash.c | 18 ++----------------
>>>>>>> drivers/crypto/bfin_crc.c | 12 ++++++++++++
>>>>>>> drivers/crypto/mxs-dcp.c | 14 ++++++++++++++
>>>>>>> drivers/crypto/n2_core.c | 12 ++++++++++++
>>>>>>> drivers/crypto/ux500/hash/hash_core.c | 18 ++++++++++++++++++
>>>>>>> 5 files changed, 58 insertions(+), 16 deletions(-)
>>>>>>
>>>>>> All applied. Thanks.
>>>>>
>>>>> This makes no sense, cfr my comment on 5/5
>>>>>
>>>>> Seems like if the driver doesn't implement those, the core can easily
>>>>> detect that and perform the necessary action. Moving the checks out of
>>>>> core seems like the wrong thing to do, rather you should enhance the
>>>>> checks in core if they're insufficient in my opinion.
>>>>
>>>> The bug can only be in driver which will not implement those two functions,
>>>> but we already had all drivers with those due to patches 1..4
>>>> All other drivers do have them.
>>>
>>> The core can very well check if these functions are not populated and
>>> return ENOSYS
>>>
>>>> Additionally, with crypto we want minimize code and run as fast as possible.
>>>
>>> So you remove all NULL pointer checks ? Esp. in security-sensitive code?
>>> What is the impact of this non-critical path code on performance?
>>>
>>> Come on ...
>>>
>>
>> Why you want checks for something that not exist ?
>>
>> Those without them will not work and will do Oops in crypto testmgr,
>> so such drivers should not be used nor accepted in drivers/crypto
>>
>> Ask yourself why crypto do not check for NULL in ahash digest or other
>> required ahash functions.
>
> Are you suggesting that the kernel code should NOT perform NULL pointer
> checks ?
>
> Are you suggesting each driver should implement every single callback
> available and if it is not implemented, return -ENOSYS ? This looks like
> a MASSIVE code duplication.
It is source code duplication. One do not load all crypto drivers at once,
simple because one board has only one crypto HW (or few closely related),
and if one even try, almost none of them will initialize on given
hardware. E.g. on Exynos board only exynos drivers will load, on board with
omap crypto only omap crypto will load.
>>>> Moving checks out of core will impose on driver author need for implement
>>>> those functions, or declare them empty, but in case of empty ones
>>>> crypto will not work properly with such driver.
>>>
>>> You can very well impose that in the core, except you don't duplicate
>>> the code.
>>
>> Now size of crypto core is reduced.
>
> You implemented the same code thrice, it surely is not reduced.
As I said above, it reduces binary size at cost of more source code in few drivers.
--
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
On 02/16/2018 10:16 AM, Kamil Konieczny wrote:
>
> On 15.02.2018 19:32, Marek Vasut wrote:
>> On 02/15/2018 07:06 PM, Kamil Konieczny wrote:
>>>
>>>
>>> On 15.02.2018 18:06, Marek Vasut wrote:
>>>> On 02/15/2018 06:00 PM, Kamil Konieczny wrote:
>>>>>
>>>>>
>>>>> On 15.02.2018 17:27, Marek Vasut wrote:
>>>>>> On 02/15/2018 04:41 PM, Herbert Xu wrote:
>>>>>>> On Thu, Jan 18, 2018 at 07:33:59PM +0100, Kamil Konieczny wrote:
>>>>>>>> First four patches add empty hash export and import functions to each driver,
>>>>>>>> with the same behaviour as in crypto framework. The last one drops them from
>>>>>>>> crypto framework. Last one for ahash.c depends on all previous.
>>>>>>>>
>>>>>>>> Changes in v3:
>>>>>>>> added change for bfin_crc.c
>>>>>>>> make this a patchset, instead of unreleated patches
>>>>>>>> make commit message more descriptive
>>>>>>>>
>>>>>>>> Kamil Konieczny (5):
>>>>>>>> crypto: mxs-dcp: Add empty hash export and import
>>>>>>>> crypto: n2_core: Add empty hash export and import
>>>>>>>> crypto: ux500/hash: Add empty export and import
>>>>>>>> crypto: bfin_crc: Add empty hash export and import
>>>>>>>> crypto: ahash.c: Require export/import in ahash
>>>>>>>>
>>>>>>>> crypto/ahash.c | 18 ++----------------
>>>>>>>> drivers/crypto/bfin_crc.c | 12 ++++++++++++
>>>>>>>> drivers/crypto/mxs-dcp.c | 14 ++++++++++++++
>>>>>>>> drivers/crypto/n2_core.c | 12 ++++++++++++
>>>>>>>> drivers/crypto/ux500/hash/hash_core.c | 18 ++++++++++++++++++
>>>>>>>> 5 files changed, 58 insertions(+), 16 deletions(-)
>>>>>>>
>>>>>>> All applied. Thanks.
>>>>>>
>>>>>> This makes no sense, cfr my comment on 5/5
>>>>>>
>>>>>> Seems like if the driver doesn't implement those, the core can easily
>>>>>> detect that and perform the necessary action. Moving the checks out of
>>>>>> core seems like the wrong thing to do, rather you should enhance the
>>>>>> checks in core if they're insufficient in my opinion.
>>>>>
>>>>> The bug can only be in driver which will not implement those two functions,
>>>>> but we already had all drivers with those due to patches 1..4
>>>>> All other drivers do have them.
>>>>
>>>> The core can very well check if these functions are not populated and
>>>> return ENOSYS
>>>>
>>>>> Additionally, with crypto we want minimize code and run as fast as possible.
>>>>
>>>> So you remove all NULL pointer checks ? Esp. in security-sensitive code?
>>>> What is the impact of this non-critical path code on performance?
>>>>
>>>> Come on ...
>>>>
>>>
>>> Why you want checks for something that not exist ?
>>>
>>> Those without them will not work and will do Oops in crypto testmgr,
>>> so such drivers should not be used nor accepted in drivers/crypto
>>>
>>> Ask yourself why crypto do not check for NULL in ahash digest or other
>>> required ahash functions.
>>
>> Are you suggesting that the kernel code should NOT perform NULL pointer
>> checks ?
>>
>> Are you suggesting each driver should implement every single callback
>> available and if it is not implemented, return -ENOSYS ? This looks like
>> a MASSIVE code duplication.
>
> It is source code duplication. One do not load all crypto drivers at once,
> simple because one board has only one crypto HW (or few closely related),
You can compile kernel with generic config and at that point you have
all the duplicated code stored on your machine. But this discussion is
moving away from the point I was concerned about -- that this patchset
_increases_ code duplication and I find this wrong.
> and if one even try, almost none of them will initialize on given
> hardware. E.g. on Exynos board only exynos drivers will load, on board with
> omap crypto only omap crypto will load.
>
>>>>> Moving checks out of core will impose on driver author need for implement
>>>>> those functions, or declare them empty, but in case of empty ones
>>>>> crypto will not work properly with such driver.
>>>>
>>>> You can very well impose that in the core, except you don't duplicate
>>>> the code.
>>>
>>> Now size of crypto core is reduced.
>>
>> You implemented the same code thrice, it surely is not reduced.
>
> As I said above, it reduces binary size at cost of more source code in few drivers.
It does NOT reduce the binary size, just try compiling all the drivers
in and it will make the kernel bigger.
--
Best regards,
Marek Vasut