Check all kmalloc(), etc, functions for success
PATCH 1/1 Check all kmalloc(), etc, functions for success
Hi Chris, et. al,
I was bored on the long flight from Melbourne to LA (and kept awake by crying babies) so I thought I'd dip my toe into kernel programming and thought I'd see if any results from kmalloc() were being used without being checked for success first.
Turns out there were quite a few that I found by hand with a simple "git grep -A2 kmalloc fs/btrfs" and so I've gone through and either BUG_ON()'d them or made them return -ENOMEM in those cases where the return value is checked.
I then installed Coccinelle (packaged in Ubuntu 10.04) and used the kmtest.cocci file to pick up other cases of memory allocations that need to be tested:
http://coccinelle.lip6.fr/impact/kmtest.html
There was one odd case in fs/btrfs/inode.c where the kzalloc() was preceded by a WARN_ON(pages); which would always be true as the only prior reference was its declaration and initialisation to NULL, so I took the liberty of moving that after the allocation and changing it to a BUG_ON().
As I'm new to this I'm only using BUG_ON() as that seems to be used elsewhere for this case in btrfs but the kernel itself (include/asm-generic/bug.h) seems to indicate that you should only use BUG_ON() as a last resort.
Please review the patch and let me know whether I'm on the right track or not! Just be gentle with me, I'm jetlagged. :-)
Patch is included inline and also attached as a MIME attachment to give a better alternative in case of wordwrap breakage in the inline version.
All the best,
Chris
fs/btrfs/compression.c +3 -0
@@ -351,6 +351,7 @@ int btrfs_submit_compressed_write(struct inode *inode, u64 start,WARN_ON (start & ((u64 )PAGE_CACHE_SIZE -1 )); cb =kmalloc (compressed_bio_size (root, compressed_len),GFP_NOFS ); +BUG_ON (!cb);atomic_set (&cb->pending_bios,0 ); cb->errors =0 ; cb->inode = inode;
@@ -588,6 +589,7 @@ int btrfs_submit_compressed_read(struct inode *inode, struct bio *bio, compressed_len = em->block_len; cb =kmalloc (compressed_bio_size (root, compressed_len),GFP_NOFS ); +BUG_ON (!cb);atomic_set (&cb->pending_bios,0 ); cb->errors =0 ; cb->inode = inode;
@@ -609,6 +611,7 @@ int btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,PAGE_CACHE_SIZE ; cb->compressed_pages =kmalloc (sizeof (struct page *) * nr_pages,GFP_NOFS ); +BUG_ON (!cb->compressed_pages); bdev =BTRFS_I (inode)->root->fs_info->fs_devices->latest_bdev;for (page_index =0 ; page_index < nr_pages; page_index++) {
fs/btrfs/disk-io.c +1 -0
@@ -1967,6 +1967,7 @@ struct btrfs_root *open_ctree(struct super_block *sb, log_tree_root =kzalloc (sizeof (struct btrfs_root ),GFP_NOFS ); +BUG_ON (!log_tree_root); __setup_root(nodesize, leafsize, sectorsize, stripesize, log_tree_root, fs_info,
BTRFS_TREE_LOG_OBJECTID);
fs/btrfs/extent-tree.c +2 -0
@@ -7161,6 +7161,8 @@ static noinline int relocate_one_extent(struct btrfs_root *extent_root,u64 group_start = group->key.objectid; new_extents =kmalloc (sizeof (*new_extents),GFP_NOFS ); +if (!new_extents) + goto out; nr_extents =1 ; ret = get_new_locations(reloc_inode, extent_key,
fs/btrfs/file.c +1 -0
@@ -877,6 +877,7 @@ static ssize_t btrfs_file_write(struct file *file, const char __user *buf,file_update_time (file); pages =kmalloc (nrptrs *sizeof (struct page *),GFP_KERNEL ); +BUG_ON (!pages);/* generic_write_checks can change our pos */ start_pos = pos;
fs/btrfs/inode.c +1 -1
@@ -284,6 +284,7 @@ static noinline int add_async_extent(struct async_cow *cow,struct async_extent *async_extent; async_extent =kmalloc (sizeof (*async_extent),GFP_NOFS ); +BUG_ON (!async_extent); async_extent->start = start; async_extent->ram_size = ram_size; async_extent->compressed_size = compressed_size;
@@ -382,8 +383,8 @@ again:if (!(BTRFS_I (inode)->flags &BTRFS_INODE_NOCOMPRESS ) && (btrfs_test_opt (root,COMPRESS ) || (BTRFS_I (inode)->force_compress))) { -WARN_ON (pages); pages = kzalloc(sizeof(struct page *) * nr_pages,
GFP_NOFS);
+ BUG_ON(pages);
ret = btrfs_zlib_compress_pages(inode->i_mapping, start,
total_compressed, pages,
@@ -930,6 +931,8 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
1, 0, NULL, GFP_NOFS);
while (start < end) {
async_cow = kmalloc(sizeof(*async_cow), GFP_NOFS);
+ if (!async_cow)
+ return -ENOMEM;
async_cow->inode = inode;
async_cow->root = root;
async_cow->locked_page = locked_page;
@@ -1958,6 +1961,7 @@ void btrfs_add_delayed_iput(struct inode *inode)
return;
delayed = kmalloc(sizeof(*delayed), GFP_NOFS | __GFP_NOFAIL);
+ BUG_ON(!delayed);
delayed->inode = inode;
spin_lock(&fs_info->delayed_iput_lock);
@@ -4568,6 +4572,8 @@ static noinline int uncompress_inline(struct btrfs_path *path,
inline_size = btrfs_file_extent_inline_item_len(leaf,
btrfs_item_nr(leaf,
path->slots[0]));
tmp = kmalloc(inline_size, GFP_NOFS);
+ if(!tmp)
+ return -ENOMEM;
ptr = btrfs_file_extent_inline_start(item);
read_extent_buffer(leaf, tmp, ptr, inline_size);fs/btrfs/tree-log.c +8 -0
@@ -334,7 +334,11 @@ static noinline int overwrite_item(struct btrfs_trans_handle *trans,return 0 ; } dst_copy =kmalloc (item_size,GFP_NOFS ); +if (!dst_copy) +return -ENOMEM ; src_copy =kmalloc (item_size,GFP_NOFS ); +if (!src_copy) +return -ENOMEM ;read_extent_buffer (eb, src_copy, src_ptr, item_size);
@@ -662,6 +666,8 @@ static noinline int drop_one_dir_item(struct btrfs_trans_handle *trans,btrfs_dir_item_key_to_cpu (leaf, di, &location); name_len =btrfs_dir_name_len (leaf, di); name =kmalloc (name_len,GFP_NOFS ); +if (!name) +return -ENOMEM ;read_extent_buffer (leaf, name, (unsigned long )(di +1 ), name_len);btrfs_release_path (root, path);
@@ -1180,6 +1186,8 @@ static noinline int replay_one_name(struct btrfs_trans_handle *trans, name_len =btrfs_dir_name_len (eb, di); name =kmalloc (name_len,GFP_NOFS ); +if (!name) +return -ENOMEM ; log_type =btrfs_dir_type (eb, di);read_extent_buffer (eb, name, (unsigned long )(di +1 ), name_len);
Signature · 1 line
Chris Samuel : http://www.csamuel.org/ : Melbourne, VIC
fs/btrfs/compression.c +3 -0
@@ -351,6 +351,7 @@ int btrfs_submit_compressed_write(struct inode *inode, u64 start,WARN_ON (start & ((u64 )PAGE_CACHE_SIZE -1 )); cb =kmalloc (compressed_bio_size (root, compressed_len),GFP_NOFS ); +BUG_ON (!cb);atomic_set (&cb->pending_bios,0 ); cb->errors =0 ; cb->inode = inode;
@@ -588,6 +589,7 @@ int btrfs_submit_compressed_read(struct inode *inode, struct bio *bio, compressed_len = em->block_len; cb =kmalloc (compressed_bio_size (root, compressed_len),GFP_NOFS ); +BUG_ON (!cb);atomic_set (&cb->pending_bios,0 ); cb->errors =0 ; cb->inode = inode;
@@ -609,6 +611,7 @@ int btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,PAGE_CACHE_SIZE ; cb->compressed_pages =kmalloc (sizeof (struct page *) * nr_pages,GFP_NOFS ); +BUG_ON (!cb->compressed_pages); bdev =BTRFS_I (inode)->root->fs_info->fs_devices->latest_bdev;for (page_index =0 ; page_index < nr_pages; page_index++) {
fs/btrfs/disk-io.c +1 -0
@@ -1967,6 +1967,7 @@ struct btrfs_root *open_ctree(struct super_block *sb, log_tree_root =kzalloc (sizeof (struct btrfs_root ),GFP_NOFS ); +BUG_ON (!log_tree_root);__setup_root (nodesize, leafsize, sectorsize, stripesize, log_tree_root, fs_info,BTRFS_TREE_LOG_OBJECTID );
fs/btrfs/extent-tree.c +2 -0
@@ -7161,6 +7161,8 @@ static noinline int relocate_one_extent(struct btrfs_root *extent_root,u64 group_start = group->key.objectid; new_extents =kmalloc (sizeof (*new_extents),GFP_NOFS ); +if (!new_extents) + goto out; nr_extents =1 ; ret = get_new_locations(reloc_inode, extent_key,
fs/btrfs/file.c +1 -0
@@ -877,6 +877,7 @@ static ssize_t btrfs_file_write(struct file *file, const char __user *buf,file_update_time (file); pages =kmalloc (nrptrs *sizeof (struct page *),GFP_KERNEL ); +BUG_ON (!pages);/* generic_write_checks can change our pos */ start_pos = pos;
fs/btrfs/inode.c +7 -1
@@ -284,6 +284,7 @@ static noinline int add_async_extent(struct async_cow *cow,struct async_extent *async_extent; async_extent =kmalloc (sizeof (*async_extent),GFP_NOFS ); +BUG_ON (!async_extent); async_extent->start = start; async_extent->ram_size = ram_size; async_extent->compressed_size = compressed_size;
@@ -382,8 +383,8 @@ again:if (!(BTRFS_I (inode)->flags &BTRFS_INODE_NOCOMPRESS ) && (btrfs_test_opt (root,COMPRESS ) || (BTRFS_I (inode)->force_compress))) { -WARN_ON (pages); pages =kzalloc (sizeof (struct page *) * nr_pages,GFP_NOFS ); +BUG_ON (pages); ret = btrfs_zlib_compress_pages(inode->i_mapping, start, total_compressed, pages,
@@ -930,6 +931,8 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page, 1, 0, NULL, GFP_NOFS);while (start < end) { async_cow =kmalloc (sizeof (*async_cow),GFP_NOFS ); +if (!async_cow) +return -ENOMEM ; async_cow->inode = inode; async_cow->root = root; async_cow->locked_page = locked_page;
@@ -1958,6 +1961,7 @@ void btrfs_add_delayed_iput(struct inode *inode)return ; delayed =kmalloc (sizeof (*delayed),GFP_NOFS | __GFP_NOFAIL); +BUG_ON (!delayed); delayed->inode = inode;spin_lock (&fs_info->delayed_iput_lock);
@@ -4568,6 +4572,8 @@ static noinline int uncompress_inline(struct btrfs_path *path, inline_size =btrfs_file_extent_inline_item_len (leaf,btrfs_item_nr (leaf, path->slots[0 ])); tmp =kmalloc (inline_size,GFP_NOFS ); +if (!tmp) +return -ENOMEM ; ptr =btrfs_file_extent_inline_start (item);read_extent_buffer (leaf, tmp, ptr, inline_size);
fs/btrfs/tree-log.c +8 -0
@@ -334,7 +334,11 @@ static noinline int overwrite_item(struct btrfs_trans_handle *trans,return 0 ; } dst_copy =kmalloc (item_size,GFP_NOFS ); +if (!dst_copy) +return -ENOMEM ; src_copy =kmalloc (item_size,GFP_NOFS ); +if (!src_copy) +return -ENOMEM ;read_extent_buffer (eb, src_copy, src_ptr, item_size);
@@ -662,6 +666,8 @@ static noinline int drop_one_dir_item(struct btrfs_trans_handle *trans,btrfs_dir_item_key_to_cpu (leaf, di, &location); name_len =btrfs_dir_name_len (leaf, di); name =kmalloc (name_len,GFP_NOFS ); +if (!name) +return -ENOMEM ;read_extent_buffer (leaf, name, (unsigned long )(di +1 ), name_len);btrfs_release_path (root, path);
@@ -1180,6 +1186,8 @@ static noinline int replay_one_name(struct btrfs_trans_handle *trans, name_len =btrfs_dir_name_len (eb, di); name =kmalloc (name_len,GFP_NOFS ); +if (!name) +return -ENOMEM ; log_type =btrfs_dir_type (eb, di);read_extent_buffer (eb, name, (unsigned long )(di +1 ), name_len);
Quoted 2 lines · Chris Samuel
Please review the patch and let me know whether I'm on the right track or not! Just be gentle with me, I'm jetlagged. :-)
Did this patch make it out OK ?
If so, any thoughts if my thinking is OK on how to address this ?
cheers,
Chris
Signature · 1 line
Chris Samuel : http://www.csamuel.org/ : Melbourne, VIC