Re: [PATCH 3/5] eCryptfs: Filename Encryption: Encoding and encryption functions

[Michael Halcrow <mhalcrow@us.ibm.com>]

On Wed, Nov 05, 2008 at 10:17:36AM -0800, Dave Hansen wrote:
> If you truly need to track the actual allocated name size, I'd
> suggest just having a:
> 
> struct e...c_filename {
> 	char *name;
> 	char *len;
> };
> 
> Stack allocate that sucker just like you stack allocate
> 'copied_name_size', and then pass *it* around.
> 
> 	filename->name = foo;
> 	filename->bar = 1234;
> 
> is way more readable than:
> 
> +                       (*encoded_name) = NULL;
> +                       (*encoded_name_size) = 0;
> 
> and
> 
> +                       (*encoded_name)[(*encoded_name_size)] = '\0';
> +                       (*encoded_name_size)++;

Agreed; something akin to qstr would do well for this sort of thing
throughout eCryptfs. In addition to making it more readable, it would
also help a bit with stack usage in some places.

> I'm certainly guilty of overly-verbose names for things, but I
> think:
> 
> 	ECRYPTFS_FNEK_ENCRYPTED_FILENAME_PREFIX_SIZE
> 
> may be a few characters too long.  :)

I generally try to strike a balance between verbosity and readability
with my symbol names. I frequently find myself torn between something
like:

ECRYPTFS_FILENAME_ENCRYPTION_KEY_ENCRYPTED_FILENAME_PREFIX_SIZE

And something like:

FNEKEFNPFXSZ

> This construct:
> 
> +               decoded_name = kmalloc(decoded_name_size, GFP_KERNEL);
> +               if (!decoded_name) {
> +                       printk(KERN_ERR "%s: Out of memory whilst attempting "
> +                              "to kmalloc [%Zd] bytes\n", __func__,
> +                              decoded_name_size);
> +                       rc = -ENOMEM;
> +                       goto out;
> +               }
> 
> also appears all over.  Personally, I think this is way too verbose,
> but I can also see how it would be useful.  But, the crappy part is
> that there is nothing unique in each of this printk()s to help the
> dmesg reader to figure out what is going on.

There are so many colorful ways for eCryptfs to blow up that I have
found it very useful to have printk's all over the place along error
paths. On the other hand, I have yet to ever receive a bug report
indicating that a kmalloc failed, so all of these particular warnings
may be more trouble than they are worth.

> I think you'd save yourself a lot of code if you had an
> ecryptfs_kmalloc(), did the NULL check and printk() in there, and
> also added a stack dump.

I still need to set the rc value appropriate for the context and then
jump to the appropriate exit label for the location in the function at
which the kmalloc occurs. Wrapping kmalloc() can only really serve, in
general, to provide the printk and stack dump. In that case, why not
just implement a kernel-wide symbol for this (e.g.,
kmalloc_verbose())?

> >         rc = write_tag_66_packet(auth_tok->token.private_key.signature,
> > -                                ecryptfs_code_for_cipher_string(crypt_stat),
> > +                                ecryptfs_code_for_cipher_string(
> > +                                        crypt_stat->cipher,
> > +                                        crypt_stat->key_size),
> >                                  crypt_stat, &payload, &payload_len);
> >         if (rc) {
> 
> Why not just have ecryptfs_code_for_cipher_string() do the ->cipher
> and ->key_size lookup?

I changed this function in this patchset specifically to support
cipher strings and key sizes from either ecryptfs_crypt_stat or
ecryptfs_mount_crypt_stat structs.