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

[Kamil Konieczny]

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