Re: [PATCH 5/5] crypto: ahash.c: Require export/import in ahash

[Kamil Konieczny]

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 
 ---
  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 != _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 Institute Poland

Re: [PATCH 5/5] crypto: ahash.c: Require export/import in ahash

[Marek Vasut]

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 
>>> ---
>>>  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 != _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

Re: [PATCH 5/5] crypto: ahash.c: Require export/import in ahash

[Kamil Konieczny]

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 
>> ---
>>  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 != _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 Institute Poland

Re: [PATCH 5/5] crypto: ahash.c: Require export/import in ahash

[Marek Vasut]

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 
> ---
>  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 != _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

[PATCH 5/5] crypto: ahash.c: Require export/import in ahash

[Kamil Konieczny]

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 
---
 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 != _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