| Message ID | 201104190527.AA00014@T-ITOH1.jp.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
(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
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
(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
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 --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);
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