Re: [PATCH 1/1]: Add countersize to CTR

[Michael Halcrow <mike@halcrow.us>]

[-- Attachment #1: Type: text/plain, Size: 2503 bytes --]

On Wed, Oct 24, 2007 at 08:42:30AM +0800, Herbert Xu wrote:
> On Tue, Oct 23, 2007 at 03:40:08PM -0500, Michael Halcrow wrote:
> > On Tue, Oct 23, 2007 at 03:26:29PM -0500, Joy Latten wrote:
> > > +	unsigned int countersize;
> > 
> > It's somewhat nicer to just use size_t in the kernel for these sorts
> > of data types. If you care about the exact number of bytes used by the
> > variable, types like u32 make the code more parsable.
> 
> I don't see how this makes a difference here at all.

Functionally, it makes no difference. It's just a common kernel
convention to use size_t rather than unsigned int. I don't think this
is a big deal though.

> > > +	err = crypto_attr_u32(tb[4], &countersize);
> > > +	if (err)
> > > +		goto out_put_alg;
> > 
> > It's also nice to have printk's along error paths. Syslogs down the
> > road tend to be less cryptic.
> 
> Please don't.  That's what error return values are for.  Adding
> printk's means that we'd have to think about limiting them later
> when this is opened up for user-space access.

It is usually appropriate to print something to the system log when
there is an error condition in the kernel code. That can help triage
down the road when people have troubles.

The only reason I can think of as to why we would *not* want
explanations in the syslog for failures is if frequent failures are
expected in a significant fraction of deployments.

> > > -		test_cipher("ctr(aes,4,8)", ENCRYPT, aes_ctr_enc_tv_template,
> > > +		test_cipher("ctr(aes,4,8,4)", ENCRYPT, aes_ctr_enc_tv_template,
> > >  			    AES_CTR_ENC_TEST_VECTORS);
> > > -		test_cipher("ctr(aes,4,8)", DECRYPT, aes_ctr_dec_tv_template,
> > > +		test_cipher("ctr(aes,4,8,4)", DECRYPT, aes_ctr_dec_tv_template,
> > >  			    AES_CTR_DEC_TEST_VECTORS);
> > 
> > I have never been particularly thrilled about the the string-based
> > method of parameterizing block ciphers for in-kernel API calls.
> 
> If you have a better suggestion I'd love to hear it!

Well, for calls made internally from kernel functions to kernel
functions, pretty much anything other than writing sequences of
comma-delimited parameters into to a character string.

For instance, define regular old-fashioned C data structs for the
various ciphers and modes. Fill in those structures with their
requisite parameters, and pass pointers to those structs into the
crypto API. Just as we would do with objects for any other subsystem
of the kernel.

Mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 481 bytes --]