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

[Mimi Zohar]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/