Re: [PATCH 4/12: eCryptfs] Main module functions

From: Pekka Enberg
Date: Sat Nov 19 2005 - 05:47:05 EST

Next message: Pekka Enberg: "Re: [PATCH 6/12: eCryptfs] Superblock operations"
Previous message: Andrea Arcangeli: "Re: shrinker->nr = LONG_MAX means deadlock for icache"
In reply to: Phillip Hellewell: "[PATCH 4/12: eCryptfs] Main module functions"
Next in thread: Anton Altaparmakov: "Re: [PATCH 4/12: eCryptfs] Main module functions"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 11/19/05, Phillip Hellewell <phillip@xxxxxxxxxxxxxxxxxxxx> wrote:
> +int ecryptfs_verbosity = 1;
> +
> +module_param(ecryptfs_verbosity, int, 0);
> +MODULE_PARM_DESC(ecryptfs_verbosity,
> + "Initial verbosity level (0 or 1; defaults to "
> + "0, which is Quiet)");
> +
> +void __ecryptfs_printk(int verb, const char *fmt, ...)
> +{
> + va_list args;
> + va_start(args, fmt);
> + if ((ecryptfs_verbosity >= verb) && printk_ratelimit())
> + vprintk(fmt, args);
> + va_end(args);
> +}
> +

[snip]

> +int ecryptfs_interpose(struct dentry *lower_dentry, struct dentry *dentry,
> + struct super_block *sb, int flag)
> +{
> + struct inode *lower_inode;
> + int err = 0;
> + struct inode *inode;
> +
> + ecryptfs_printk(1, KERN_NOTICE, "Enter; lower_dentry = [%p], "
> + "lower_dentry->d_name.name = [%s], dentry = "
> + "[%p], dentry->d_name.name = [%s], sb = [%p]; "
> + "flag = [%.4x]; lower_dentry->d_count "
> + "= [%d]; dentry->d_count = [%d]\n", lower_dentry,
> + lower_dentry->d_name.name, dentry, dentry->d_name.name,
> + sb, flag, atomic_read(&lower_dentry->d_count),
> + atomic_read(&dentry->d_count));

Could you use KERN_DEBUG instead and drop ecryptfs_printk()?

> + if (NULL == ECRYPTFS_INODE_TO_LOWER(inode)) {
> + ECRYPTFS_INODE_TO_LOWER(inode) = igrab(lower_inode);
> + /* If we are still NULL at this point, igrab failed.
> + * We are _NOT_ supposed to be failing here */
> + if (NULL == ECRYPTFS_INODE_TO_LOWER(inode)) {

If you're worried about assignment, why not use the normal idiom:

if (!p) {
...
}

> + BUG();
> + err = -EINVAL;
> + goto out;

Why do you want to BUG() and then handle the situation?

> +kmem_cache_t *ecryptfs_sb_info_cache;

We have struct kmem_cache now so please consider using it instead.

> +/* This provides a means of backing out cache creations out of the kernel
> + * so that we can elegantly fail should we run out of memory.
> + */
> +#define ECRYPTFS_AUTH_TOK_LIST_ITEM_CACHE 0x0001
> +#define ECRYPTFS_AUTH_TOK_PKT_SET_CACHE 0x0002
> +#define ECRYPTFS_AUTH_TOK_REQUEST_CACHE 0x0004
> +#define ECRYPTFS_AUTH_TOK_REQUEST_BLOB_CACHE 0x0008
> +#define ECRYPTFS_FILE_INFO_CACHE 0x0010
> +#define ECRYPTFS_DENTRY_INFO_CACHE 0x0020
> +#define ECRYPTFS_INODE_INFO_CACHE 0x0040
> +#define ECRYPTFS_SB_INFO_CACHE 0x0080
> +#define ECRYPTFS_HEADER_CACHE_0 0x0100
> +#define ECRYPTFS_HEADER_CACHE_1 0x0200
> +#define ECRYPTFS_HEADER_CACHE_2 0x0400
> +#define ECRYPTFS_LOWER_PAGE_CACHE 0x0800
> +#define ECRYPTFS_CACHE_CREATION_SUCCESS 0x0FF1

A better way would be to either (1) make ecryptfs_init_kmem_caches()
clean up after itself on error using gotos or (2) keep caches NULL
before initialization and check for that in
ecryptfs_free_kmem_caches().

> +
> +static short ecryptfs_allocated_caches;
> +
> +/**
> + * @return Zero on success; non-zero otherwise
> + *
> + * Sets ecryptfs_allocated_caches with flags so that we can
> + * free created caches should we run out of memory during
> + * creation period.
> + *
> + * The overhead for doing this is offset by the fact that we
> + * only do this once, and that should there be insufficient
> + * memory, then we can elegantly fail, and not leave extra
> + * caches around, or worse, panic the kernel trying to free
> + * something that's not there.
> + */
> +static int ecryptfs_init_kmem_caches(void)
> +{
> + int rc = 0;
> +
> + ecryptfs_auth_tok_list_item_cache =
> + kmem_cache_create("ecryptfs_auth_tok_list_item",
> + sizeof(struct ecryptfs_auth_tok_list_item),
> + 0, SLAB_HWCACHE_ALIGN, NULL, NULL);
> + if (ecryptfs_auth_tok_list_item_cache)
> + rc |= ECRYPTFS_AUTH_TOK_LIST_ITEM_CACHE;
> + else
> + ecryptfs_printk(0, KERN_WARNING, "ecryptfs_auth_tok_list_item "
> + "kmem_cache_create failed\n");
> +
> + ecryptfs_file_info_cache =
> + kmem_cache_create("ecryptfs_file_cache",
> + sizeof(struct ecryptfs_file_info),
> + 0, SLAB_HWCACHE_ALIGN, NULL, NULL);
> + if (ecryptfs_file_info_cache)
> + rc |= ECRYPTFS_FILE_INFO_CACHE;
> + else
> + ecryptfs_printk(0, KERN_WARNING, "ecryptfs_file_cache "
> + "kmem_cache_create failed\n");
> +
> + ecryptfs_dentry_info_cache =
> + kmem_cache_create("ecryptfs_dentry_cache",
> + sizeof(struct ecryptfs_dentry_info),
> + 0, SLAB_HWCACHE_ALIGN, NULL, NULL);
> + if (ecryptfs_dentry_info_cache)
> + rc |= ECRYPTFS_DENTRY_INFO_CACHE;
> + else
> + ecryptfs_printk(0, KERN_WARNING, "ecryptfs_dentry_cache "
> + "kmem_cache_create failed\n");
> +
> + ecryptfs_inode_info_cache =
> + kmem_cache_create("ecryptfs_inode_cache",
> + sizeof(struct ecryptfs_inode_info), 0,
> + SLAB_HWCACHE_ALIGN, inode_info_init_once, NULL);
> + if (ecryptfs_inode_info_cache)
> + rc |= ECRYPTFS_INODE_INFO_CACHE;
> + else
> + ecryptfs_printk(0, KERN_WARNING, "ecryptfs_inode_cache "
> + "kmem_cache_create failed\n");
> +
> + ecryptfs_sb_info_cache =
> + kmem_cache_create("ecryptfs_sb_cache",
> + sizeof(struct ecryptfs_sb_info),
> + 0, SLAB_HWCACHE_ALIGN, NULL, NULL);
> + if (ecryptfs_sb_info_cache)
> + rc |= ECRYPTFS_SB_INFO_CACHE;
> + else
> + ecryptfs_printk(0, KERN_WARNING, "ecryptfs_sb_cache "
> + "kmem_cache_create failed\n");
> +
> + ecryptfs_header_cache_0 =
> + kmem_cache_create("ecryptfs_headers_0", PAGE_CACHE_SIZE,
> + 0, SLAB_HWCACHE_ALIGN, NULL, NULL);
> + if (ecryptfs_header_cache_0)
> + rc |= ECRYPTFS_HEADER_CACHE_0;
> + else
> + ecryptfs_printk(0, KERN_WARNING, "ecryptfs_headers_0 "
> + "kmem_cache_create failed\n");
> +
> + ecryptfs_header_cache_1 =
> + kmem_cache_create("ecryptfs_headers_1", PAGE_CACHE_SIZE,
> + 0, SLAB_HWCACHE_ALIGN, NULL, NULL);
> + if (ecryptfs_header_cache_1)
> + rc |= ECRYPTFS_HEADER_CACHE_1;
> + else
> + ecryptfs_printk(0, KERN_WARNING, "ecryptfs_headers_1 "
> + "kmem_cache_create failed\n");
> +
> + ecryptfs_header_cache_2 =
> + kmem_cache_create("ecryptfs_headers_2", PAGE_CACHE_SIZE,
> + 0, SLAB_HWCACHE_ALIGN, NULL, NULL);
> + if (ecryptfs_header_cache_2)
> + rc |= ECRYPTFS_HEADER_CACHE_2;
> + else
> + ecryptfs_printk(0, KERN_WARNING, "ecryptfs_headers_2 "
> + "kmem_cache_create failed\n");
> +
> + ecryptfs_lower_page_cache =
> + kmem_cache_create("ecryptfs_lower_page_cache", PAGE_CACHE_SIZE,
> + 0, SLAB_HWCACHE_ALIGN, NULL, NULL);
> + if (ecryptfs_lower_page_cache)
> + rc |= ECRYPTFS_LOWER_PAGE_CACHE;
> + else
> + ecryptfs_printk(0, KERN_WARNING, "ecryptfs_lower_page_cache "
> + "kmem_cache_create failed\n");
> +
> + ecryptfs_allocated_caches = rc;
> + rc = ECRYPTFS_CACHE_CREATION_SUCCESS ^ rc;
> + return rc;
> +}
> +
> +/**
> + * @return Zero on success; non-zero otherwise
> + */
> +static int ecryptfs_free_kmem_caches(void)
> +{
> + int rc = 0;
> + int err;
> +
> + if (ecryptfs_allocated_caches & ECRYPTFS_AUTH_TOK_LIST_ITEM_CACHE) {
> + rc = kmem_cache_destroy(ecryptfs_auth_tok_list_item_cache);
> + if (rc)
> + ecryptfs_printk(0, KERN_WARNING,
> + "Not all ecryptfs_auth_tok_"
> + "list_item_cache structures were "
> + "freed\n");
> + }
> + if (ecryptfs_allocated_caches & ECRYPTFS_FILE_INFO_CACHE) {
> + err = kmem_cache_destroy(ecryptfs_file_info_cache);
> + if (err)
> + ecryptfs_printk(0, KERN_WARNING,
> + "Not all ecryptfs_file_info_"
> + "cache regions were freed\n");
> + rc |= err;
> + }
> + if (ecryptfs_allocated_caches & ECRYPTFS_DENTRY_INFO_CACHE) {
> + err = kmem_cache_destroy(ecryptfs_dentry_info_cache);
> + if (err)
> + ecryptfs_printk(0, KERN_WARNING,
> + "Not all ecryptfs_dentry_info_"
> + "cache regions were freed\n");
> + rc |= err;
> + }
> + if (ecryptfs_allocated_caches & ECRYPTFS_INODE_INFO_CACHE) {
> + err = kmem_cache_destroy(ecryptfs_inode_info_cache);
> + if (err)
> + ecryptfs_printk(0, KERN_WARNING,
> + "Not all ecryptfs_inode_info_"
> + "cache regions were freed\n");
> + rc |= err;
> + }
> + if (ecryptfs_allocated_caches & ECRYPTFS_SB_INFO_CACHE) {
> + err = kmem_cache_destroy(ecryptfs_sb_info_cache);
> + if (err)
> + ecryptfs_printk(0, KERN_WARNING,
> + "Not all ecryptfs_sb_info_"
> + "cache regions were freed\n");
> + rc |= err;
> + }
> + if (ecryptfs_allocated_caches & ECRYPTFS_HEADER_CACHE_0) {
> + err = kmem_cache_destroy(ecryptfs_header_cache_0);
> + if (err)
> + ecryptfs_printk(0, KERN_WARNING, "Not all ecryptfs_"
> + "header_cache_0 regions were freed\n");
> + rc |= err;
> + }
> + if (ecryptfs_allocated_caches & ECRYPTFS_HEADER_CACHE_1) {
> + err = kmem_cache_destroy(ecryptfs_header_cache_1);
> + if (err)
> + ecryptfs_printk(0, KERN_WARNING, "Not all ecryptfs_"
> + "header_cache_1 regions were freed\n");
> + rc |= err;
> + }
> + if (ecryptfs_allocated_caches & ECRYPTFS_HEADER_CACHE_2) {
> + err = kmem_cache_destroy(ecryptfs_header_cache_2);
> + if (err)
> + ecryptfs_printk(0, KERN_WARNING, "Not all ecryptfs_"
> + "header_cache_2 regions were freed\n");
> + rc |= err;
> + }
> + if (ecryptfs_allocated_caches & ECRYPTFS_LOWER_PAGE_CACHE) {
> + err = kmem_cache_destroy(ecryptfs_lower_page_cache);
> + if (err)
> + ecryptfs_printk(0, KERN_WARNING, "Not all ecryptfs_"
> + "lower_page_cache regions were "
> + "freed\n");
> + rc |= err;
> + }
> + return rc;
> +}
> +
> +static int __init init_ecryptfs_fs(void)
> +{
> + int rc;
> +
> + rc = ecryptfs_init_kmem_caches();
> + if (rc) {
> + ecryptfs_printk(0, KERN_EMERG, "Failure occured while "
> + "attempting to create caches [CREATED: %x]."
> + "Now freeing caches.\n",
> + ecryptfs_allocated_caches);
> + ecryptfs_free_kmem_caches();
> + return -ENOMEM;
> + }
> + ecryptfs_printk(1, KERN_NOTICE, "Registering eCryptfs\n");
> + return register_filesystem(&ecryptfs_fs_type);

register_filesystem() can fail in which case youre leaking all the
slab caches here.

Pekka
-
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/

Next message: Pekka Enberg: "Re: [PATCH 6/12: eCryptfs] Superblock operations"
Previous message: Andrea Arcangeli: "Re: shrinker->nr = LONG_MAX means deadlock for icache"
In reply to: Phillip Hellewell: "[PATCH 4/12: eCryptfs] Main module functions"
Next in thread: Anton Altaparmakov: "Re: [PATCH 4/12: eCryptfs] Main module functions"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]