Re: [PATCH v3 1/3] ima: use ahash API for file hash calculation

[Dmitry Kasatkin <d.kasatkin@samsung.com>]

On 07/07/14 19:34, Mimi Zohar wrote:
> On Mon, 2014-07-07 at 19:11 +0300, Dmitry Kasatkin wrote: 
>> On 07/07/14 18:44, Mimi Zohar wrote:
>>> On Mon, 2014-07-07 at 16:37 +0300, Dmitry Kasatkin wrote: 
>>>> On 07/07/14 14:56, Mimi Zohar wrote:
>>>>> On Fri, 2014-07-04 at 15:05 +0300, Dmitry Kasatkin wrote: 
>>>>>> +/**
>>>>> This is the kernel-doc delimiter.
>>>>>
>>>>>> + * ima_calc_file_hash - calculae file hash
>>>>>> + *
>>>>> Missing kernel-doc argument descriptions.  Refer to
>>>>> Documentation/kernel-doc-nano-HOWTO.txt.
>>> Not defining the arguments results in a kernel-doc warning.  Providing
>>> kernel-doc is nice, but is unnecessary in this case, as it isn't an
>>> exported loadable module, nor an externally visible function to other
>>> kernel files.  Either remove the extra asterisk, making it a regular
>>> comment, or add the arguments.
>>>
>>>> There is no need to explain arguments as they self-evident.
>>>>
>>>>>> + * if ima_ahash_minsize parameter is non-zero, this function uses
>>>>>> + * ahash for hash caclulation. ahash performance varies for different
>>>>>> + * data sizes on different crypto accelerators. shash performance might
>>>>>> + * be better for small file. 'ima.ahash_minsize' module parameter allows
>>>>>> + * to specify the best value for the system.
>>>>>> + *
>>>>>> + * If ahash fails, it fallbacks to shash.
>>>>>> + */
>>>>>> +int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>>>>>> +{
>>>>>> +	loff_t i_size;
>>>>>> +	int rc;
>>>>>> +
>>>>>> +	i_size = i_size_read(file_inode(file));
>>>>>> +
>>>>>> +	if (ima_ahash_minsize && i_size >= ima_ahash_minsize) {
>>>>>> +		rc = ima_calc_file_ahash(file, hash);
>>>>>> +		if (!rc)
>>>>>> +			return 0;
>>>>>> +	}
>>>>>> +
>>>>>> +	return ima_calc_file_shash(file, hash);
>>>>>> +}
>>>>> If the crypto accelerator fails, it falls back to using shash.  Is their
>>>>> any indication that the HW error is intermittent or persistent?  Should
>>>>> ima_ahash_minsize be reset?
>>>> If hw constantly does not work then it is simply broken.
>>> True
>>>
>>>> You want to be protected from "random" failures.
>>>> For me it is not the case either... If it works then it works...
>>> This discussion isn't about your particular HW environment, but a
>>> general question.  For example, suppose we were discussing a laptop with
>>> a HW crypto accelerator.  If the HW crypto broke, I would at least want
>>> to be able to quiesce the system properly.  I'd most likely want to be
>>> able to continue using my laptop with software crypto.
>> Driver probing code will detect that HW is not responding and driver
>> will not be enabled...
>>
>> IMA will not be able to use it...
>>
>> It is the same story as with any other HW and driver in the system.
> Right, but my concern is not about unloading the kernel module, but
> about the IMA module parameters left initialized.  The existing code
> will continue using ahash (software version), even though the kernel
> module was unloaded, not shash.  My question is about the software
> implementations of ahash vs. shash performance.
>
> Mimi

If HW driver will not be available, ahash loads generic driver which is
using shash.
Performance of that will be the same as for using shash directly.

- Dmitry

>