diff mbox

Btrfs: check return value of kmalloc()

Message ID 201104190527.AA00014@T-ITOH1.jp.fujitsu.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tsutomu Itoh April 19, 2011, 5:27 a.m. UTC 
The check on the return value of kmalloc() in inode.c is added.

Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
---
 fs/btrfs/inode.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

David Sterba April 19, 2011, 11:12 a.m. UTC | #1 
Hi,

there are more unchecked kmallocs, in inode.c:btrfs_add_delayed_iput()

2030        delayed = kmalloc(sizeof(*delayed), GFP_NOFS | __GFP_NOFAIL);

and in extent-tree.c:relocate_one_extent()

7992        new_extents = kmalloc(sizeof(*new_extents),
7993                                                 GFP_NOFS);

the value is checked later, new_extents is passed to get_new_locations
and there it's checked, but no other callers pass potential NULL and the
check fits here and can be dropped from get_new_locations; there's a
little chance that get_new_locations will be able to succesfully
allocate the same data a jiffy later.

IMO the check can be safely added here and get_new_locations cleaned up
later.

Feel free to fold the changes into your patch.

I did not find any more unchecked allocatinos.


dave


On Tue, Apr 19, 2011 at 02:27:08PM +0900, Tsutomu Itoh wrote:
> The check on the return value of kmalloc() in inode.c is added.
> 
> Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
> ---
>  fs/btrfs/inode.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index a4157cf..c718d27 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -953,6 +953,7 @@ 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);
> +		BUG_ON(!async_cow);
>  		async_cow->inode = inode;
>  		async_cow->root = root;
>  		async_cow->locked_page = locked_page;
> @@ -5001,6 +5002,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);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tsutomu Itoh April 20, 2011, 12:51 a.m. UTC | #2 
(2011/04/19 20:12), David Sterba wrote:

Thanks for your review.

> Hi,
> 
> there are more unchecked kmallocs, in inode.c:btrfs_add_delayed_iput()
> 
> 2030        delayed = kmalloc(sizeof(*delayed), GFP_NOFS | __GFP_NOFAIL);

I think that it doesn't fail ordinary when __GFP_NOFAIL is specified...

> 
> and in extent-tree.c:relocate_one_extent()
> 
> 7992        new_extents = kmalloc(sizeof(*new_extents),
> 7993                                                 GFP_NOFS);
> 
> the value is checked later, new_extents is passed to get_new_locations
> and there it's checked, but no other callers pass potential NULL and the
> check fits here and can be dropped from get_new_locations; there's a
> little chance that get_new_locations will be able to succesfully
> allocate the same data a jiffy later.

Yes, therefore I did not check 'new_extents'.

Thanks,
Tsutomu


> 
> IMO the check can be safely added here and get_new_locations cleaned up
> later.
> 
> Feel free to fold the changes into your patch.
> 
> I did not find any more unchecked allocatinos.
> 
> 
> dave
> 
> 
> On Tue, Apr 19, 2011 at 02:27:08PM +0900, Tsutomu Itoh wrote:
>> The check on the return value of kmalloc() in inode.c is added.
>>
>> Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
>> ---
>>  fs/btrfs/inode.c |    3 +++
>>  1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index a4157cf..c718d27 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -953,6 +953,7 @@ 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);
>> +		BUG_ON(!async_cow);
>>  		async_cow->inode = inode;
>>  		async_cow->root = root;
>>  		async_cow->locked_page = locked_page;
>> @@ -5001,6 +5002,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);
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba April 21, 2011, 12:18 p.m. UTC | #3 
On Wed, Apr 20, 2011 at 09:51:30AM +0900, Tsutomu Itoh wrote:
> > 2030        delayed = kmalloc(sizeof(*delayed), GFP_NOFS | __GFP_NOFAIL);
> 
> I think that it doesn't fail ordinary when __GFP_NOFAIL is specified...

yes I agree with you, my oversight. However, from linux/gfp.h

 * __GFP_NOFAIL: The VM implementation _must_ retry infinitely: the
 * caller cannot handle allocation failures.  This modifier is
 * deprecated and no new users should be added.

in long-term, redoing without NOFAIL would be probably wise. Currently
the btrfs_add_delayed_iput is called at places which do not seem to
like failure, I'm not sure whether possibly blocking indefinetely is
better than occasional failure with chance to do recovery ...

> 
> > 
> > and in extent-tree.c:relocate_one_extent()
> > 
> > 7992        new_extents = kmalloc(sizeof(*new_extents),
> > 7993                                                 GFP_NOFS);
> > 
> > the value is checked later, new_extents is passed to get_new_locations
> > and there it's checked, but no other callers pass potential NULL and the
> > check fits here and can be dropped from get_new_locations;

> > there's a
> > little chance that get_new_locations will be able to succesfully
> > allocate the same data a jiffy later.
> 
> Yes, therefore I did not check 'new_extents'.

heh reading it again after myself, it sounds quite the opposite than I
wanted: I'd rather see the kmalloc checked right at the callsite, easier
to read and understand, than diving into get_new_locations and there
seeing checking extents for NULL and doing own alloc/free.


david
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tsutomu Itoh April 22, 2011, 1:23 a.m. UTC | #4 
(2011/04/21 21:18), David Sterba wrote:
> On Wed, Apr 20, 2011 at 09:51:30AM +0900, Tsutomu Itoh wrote:
>>> 2030        delayed = kmalloc(sizeof(*delayed), GFP_NOFS | __GFP_NOFAIL);
>>
>> I think that it doesn't fail ordinary when __GFP_NOFAIL is specified...
> 
> yes I agree with you, my oversight. However, from linux/gfp.h
> 
>  * __GFP_NOFAIL: The VM implementation _must_ retry infinitely: the
>  * caller cannot handle allocation failures.  This modifier is
>  * deprecated and no new users should be added.
> 
> in long-term, redoing without NOFAIL would be probably wise. Currently
> the btrfs_add_delayed_iput is called at places which do not seem to
> like failure, I'm not sure whether possibly blocking indefinetely is
> better than occasional failure with chance to do recovery ...
> 
>>
>>>
>>> and in extent-tree.c:relocate_one_extent()
>>>
>>> 7992        new_extents = kmalloc(sizeof(*new_extents),
>>> 7993                                                 GFP_NOFS);
>>>
>>> the value is checked later, new_extents is passed to get_new_locations
>>> and there it's checked, but no other callers pass potential NULL and the
>>> check fits here and can be dropped from get_new_locations;
> 
>>> there's a
>>> little chance that get_new_locations will be able to succesfully
>>> allocate the same data a jiffy later.
>>
>> Yes, therefore I did not check 'new_extents'.
> 

> heh reading it again after myself, it sounds quite the opposite than I
> wanted: I'd rather see the kmalloc checked right at the callsite, easier
> to read and understand, than diving into get_new_locations and there
> seeing checking extents for NULL and doing own alloc/free.

OK, I understand.
But, reading code again, I noticed nobody use relocate_one_extent() now. ;-)

However, because there is a possibility to be going to be used in the future,
I'll add the check of 'new_extents' for the readability.

Thanks,
Tsutomu
 

> 
> 
> david
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba April 22, 2011, 12:02 p.m. UTC | #5 
On Fri, Apr 22, 2011 at 10:23:14AM +0900, Tsutomu Itoh wrote:
> But, reading code again, I noticed nobody use relocate_one_extent() now. ;-)
> However, because there is a possibility to be going to be used in the future,
> I'll add the check of 'new_extents' for the readability.

vim hilight confused me, but the function is inside a large #if 0 block
already, I think we do not need to fix this kmalloc anymore. sorry for not
pointing it out.

the #if0 comes from commit 5d4f98a28c7d334091c1b7744f48a1acdd2a4ae0
dated back to  Jun 10 10:45:14 2009 -0400
Btrfs: Mixed back reference  (FORWARD ROLLING FORMAT CHANGE)

big changeset, disk format change, it's quite understandable that the
autors wanted to keep the previous code in place for some time, but it's
not needed anymore.


david
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index a4157cf..c718d27 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -953,6 +953,7 @@  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);
+		BUG_ON(!async_cow);
 		async_cow->inode = inode;
 		async_cow->root = root;
 		async_cow->locked_page = locked_page;
@@ -5001,6 +5002,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);