Btrfs: leak in btrfs_find_item and cleanups

GIT PULL New David Sterba 1 participant 6 messages Jan 14, 2015 Btrfs file system

Share & tools

David Sterba

author

2015-01-14 18:36 UTC #0 lore raw

Coverity for btrfs-progs spotted a variable leak in btrfs_find_item though the same error is not reported in the kernel project and the code is the same. The leak is very rare though and does not need to go to stable immediatelly. The rest of patches are cleanups in the related codepaths.

You can also pull the changes from
git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git fix/find-item-path-leak

Top commit: 1d4c08e0a60be3561
Base: 3.19-rc4

David Sterba (5):
btrfs: fix leak of path in btrfs_find_item
btrfs: cleanup, remove inode_item_info helper
btrfs: cleanup, remove inode_ref_info helper
btrfs: simplify insert_orphan_item
btrfs: expand btrfs_find_item if found_key is NULL

 fs/btrfs/backref.c  | 28 ++++++----------------------
 fs/btrfs/backref.h  |  3 ---
 fs/btrfs/ctree.c    | 18 +++++-------------
 fs/btrfs/disk-io.c  | 15 +++++++++++++--
 fs/btrfs/inode.c    | 10 +++++++---
 fs/btrfs/scrub.c    | 10 +++++++++-
 fs/btrfs/tree-log.c | 11 ++++++-----
 7 files changed, 46 insertions(+), 49 deletions(-)

Signature · 1 line

1.8.4.5

David Sterba

author

2015-01-14 18:36 UTC #1 lore raw

PATCH 1/5 btrfs: fix leak of path in btrfs_find_item

If btrfs_find_item is called with NULL path it allocates one locally but does not free it. Affected paths are inserting an orphan item for a file and for a subvol root.

Move the path allocation to the callers.

CC <[email protected]> # 3.14+

Fixes 3f870c289900 ("btrfs: expand btrfs_find_item() to include find_orphan_item functionality")

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/ctree.c    | 17 ++++-------------
 fs/btrfs/disk-io.c  |  9 ++++++++-
 fs/btrfs/tree-log.c | 11 ++++++++++-
 3 files changed, 22 insertions(+), 15 deletions(-)

fs/btrfs/ctree.c +4 ~~-13~~

index 14a72ed14ef7..f54511dd287e 100644
@@ -2609,32 +2609,23 @@ static int key_search(struct extent_buffer *b, struct btrfs_key *key,
 	return 0;
 }
 
-int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path *found_path,
+int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path *path,
 		u64 iobjectid, u64 ioff, u8 key_type,
 		struct btrfs_key *found_key)
 {
 	int ret;
 	struct btrfs_key key;
 	struct extent_buffer *eb;
-	struct btrfs_path *path;
+
+	ASSERT(path);
 
 	key.type = key_type;
 	key.objectid = iobjectid;
 	key.offset = ioff;
 
-	if (found_path == NULL) {
-		path = btrfs_alloc_path();
-		if (!path)
-			return -ENOMEM;
-	} else
-		path = found_path;
-
 	ret = btrfs_search_slot(NULL, fs_root, &key, path, 0, 0);
-	if ((ret < 0) || (found_key == NULL)) {
-		if (path != found_path)
-			btrfs_free_path(path);
+	if ((ret < 0) || (found_key == NULL))
 		return ret;
-	}
 
 	eb = path->nodes[0];
 	if (ret && path->slots[0] >= btrfs_header_nritems(eb)) {

fs/btrfs/disk-io.c +8 -1

index 8c63419a7f70..6182e5493d0f 100644
@@ -1630,6 +1630,7 @@ struct btrfs_root *btrfs_get_fs_root(struct btrfs_fs_info *fs_info,
 				     bool check_ref)
 {
 	struct btrfs_root *root;
+	struct btrfs_path *path;
 	int ret;
 
 	if (location->objectid == BTRFS_ROOT_TREE_OBJECTID)

@@ -1669,8 +1670,14 @@ again:
 	if (ret)
 		goto fail;
 
-	ret = btrfs_find_item(fs_info->tree_root, NULL, BTRFS_ORPHAN_OBJECTID,
+	path = btrfs_alloc_path();
+	if (!path) {
+		ret = -ENOMEM;
+		goto fail;
+	}
+	ret = btrfs_find_item(fs_info->tree_root, path, BTRFS_ORPHAN_OBJECTID,
 			location->objectid, BTRFS_ORPHAN_ITEM_KEY, NULL);
+	btrfs_free_path(path);
 	if (ret < 0)
 		goto fail;
 	if (ret == 0)

fs/btrfs/tree-log.c +10 -1

index 9a02da16f2be..5be45c12dd71 100644
@@ -1257,10 +1257,19 @@ static int insert_orphan_item(struct btrfs_trans_handle *trans,
 			      struct btrfs_root *root, u64 offset)
 {
 	int ret;
-	ret = btrfs_find_item(root, NULL, BTRFS_ORPHAN_OBJECTID,
+	struct btrfs_path *path;
+
+	path = btrfs_alloc_path();
+	if (!path)
+		return -ENOMEM;
+
+	ret = btrfs_find_item(root, path, BTRFS_ORPHAN_OBJECTID,
 			offset, BTRFS_ORPHAN_ITEM_KEY, NULL);
 	if (ret > 0)
 		ret = btrfs_insert_orphan_item(trans, root, offset);
+
+	btrfs_free_path(path);
+
 	return ret;
 }

Signature · 1 line

1.8.4.5

David Sterba

author

2015-01-14 18:36 UTC #2 lore raw

PATCH 2/5 btrfs: cleanup, remove inode_item_info helper

In reply to David Sterba

It's only a simple wrapper around btrfs_find_item, the locally defined key is not used.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/backref.c | 11 -----------
 fs/btrfs/backref.h |  3 ---
 fs/btrfs/scrub.c   |  6 +++++-
 3 files changed, 5 insertions(+), 15 deletions(-)

fs/btrfs/backref.c +0 ~~-11~~

index 2d3e32ebfd15..66554b3f7748 100644
@@ -1246,17 +1246,6 @@ int btrfs_check_shared(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-/*
- * this makes the path point to (inum INODE_ITEM ioff)
- */
-int inode_item_info(u64 inum, u64 ioff, struct btrfs_root *fs_root,
-			struct btrfs_path *path)
-{
-	struct btrfs_key key;
-	return btrfs_find_item(fs_root, path, inum, ioff,
-			BTRFS_INODE_ITEM_KEY, &key);
-}
-
 static int inode_ref_info(u64 inum, u64 ioff, struct btrfs_root *fs_root,
 				struct btrfs_path *path,
 				struct btrfs_key *found_key)

fs/btrfs/backref.h +0 -3

index 2a1ac6bfc724..9c41fbac3009 100644
@@ -32,9 +32,6 @@ struct inode_fs_paths {
 typedef int (iterate_extent_inodes_t)(u64 inum, u64 offset, u64 root,
 		void *ctx);
 
-int inode_item_info(u64 inum, u64 ioff, struct btrfs_root *fs_root,
-			struct btrfs_path *path);
-
 int extent_from_logical(struct btrfs_fs_info *fs_info, u64 logical,
 			struct btrfs_path *path, struct btrfs_key *found_key,
 			u64 *flags);

fs/btrfs/scrub.c +5 -1

index f2bb13a23f86..8bfe3b6fd471 100644
@@ -530,7 +530,11 @@ static int scrub_print_warning_inode(u64 inum, u64 offset, u64 root,
 		goto err;
 	}
 
-	ret = inode_item_info(inum, 0, local_root, swarn->path);
+	/*
+	 * this makes the path point to (inum INODE_ITEM ioff)
+	 */
+	ret = btrfs_find_item(local_root, swarn->path, inum, 0,
+			BTRFS_INODE_ITEM_KEY, NULL);
 	if (ret) {
 		btrfs_release_path(swarn->path);
 		goto err;

Signature · 1 line

1.8.4.5

David Sterba

author

2015-01-14 18:36 UTC #3 lore raw

PATCH 3/5 btrfs: cleanup, remove inode_ref_info helper

In reply to David Sterba

A simple wrapper around btrfs_find_item.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/backref.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

fs/btrfs/backref.c +6 ~~-11~~

index 66554b3f7748..e715c16e9d07 100644
@@ -1246,14 +1246,6 @@ int btrfs_check_shared(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-static int inode_ref_info(u64 inum, u64 ioff, struct btrfs_root *fs_root,
-				struct btrfs_path *path,
-				struct btrfs_key *found_key)
-{
-	return btrfs_find_item(fs_root, path, inum, ioff,
-			BTRFS_INODE_REF_KEY, found_key);
-}
-
 int btrfs_find_one_extref(struct btrfs_root *root, u64 inode_objectid,
 			  u64 start_off, struct btrfs_path *path,
 			  struct btrfs_inode_extref **ret_extref,

@@ -1363,7 +1355,8 @@ char *btrfs_ref_to_path(struct btrfs_root *fs_root, struct btrfs_path *path,
 			btrfs_tree_read_unlock_blocking(eb);
 			free_extent_buffer(eb);
 		}
-		ret = inode_ref_info(parent, 0, fs_root, path, &found_key);
+		ret = btrfs_find_item(fs_root, path, parent, 0,
+				BTRFS_INODE_REF_KEY, &found_key);
 		if (ret > 0)
 			ret = -ENOENT;
 		if (ret)

@@ -1709,8 +1702,10 @@ static int iterate_inode_refs(u64 inum, struct btrfs_root *fs_root,
 	struct btrfs_key found_key;
 
 	while (!ret) {
-		ret = inode_ref_info(inum, parent ? parent+1 : 0, fs_root, path,
-				     &found_key);
+		ret = btrfs_find_item(fs_root, path, inum,
+				parent ? parent + 1 : 0, BTRFS_INODE_REF_KEY,
+				&found_key);
+
 		if (ret < 0)
 			break;
 		if (ret) {

Signature · 1 line

1.8.4.5

David Sterba

author

2015-01-14 18:36 UTC #4 lore raw

PATCH 4/5 btrfs: simplify insert_orphan_item

In reply to David Sterba

We can search and add the orphan item in one go, btrfs_insert_orphan_item will find out if the item already exists.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/tree-log.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

fs/btrfs/tree-log.c +4 ~~-12~~

index 5be45c12dd71..25a1c363a5f4 100644
@@ -1254,21 +1254,13 @@ out:
 }
 
 static int insert_orphan_item(struct btrfs_trans_handle *trans,
-			      struct btrfs_root *root, u64 offset)
+			      struct btrfs_root *root, u64 ino)
 {
 	int ret;
-	struct btrfs_path *path;
-
-	path = btrfs_alloc_path();
-	if (!path)
-		return -ENOMEM;
 
-	ret = btrfs_find_item(root, path, BTRFS_ORPHAN_OBJECTID,
-			offset, BTRFS_ORPHAN_ITEM_KEY, NULL);
-	if (ret > 0)
-		ret = btrfs_insert_orphan_item(trans, root, offset);
-
-	btrfs_free_path(path);
+	ret = btrfs_insert_orphan_item(trans, root, ino);
+	if (ret == -EEXIST)
+		ret = 0;
 
 	return ret;
 }

Signature · 1 line

1.8.4.5

David Sterba

author

2015-01-14 18:36 UTC #5 lore raw

PATCH 5/5 btrfs: expand btrfs_find_item if found_key is NULL

In reply to David Sterba

If the found_key is NULL, then btrfs_find_item becomes a verbose wrapper for simple btrfs_search_slot.

After we've removed all such callers, passing a NULL key is not valid anymore.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/ctree.c   |  3 ++-
 fs/btrfs/disk-io.c |  8 ++++++--
 fs/btrfs/inode.c   | 10 +++++++---
 fs/btrfs/scrub.c   |  8 ++++++--
 4 files changed, 21 insertions(+), 8 deletions(-)

fs/btrfs/ctree.c +2 -1

index f54511dd287e..20d1f2b0403d 100644
@@ -2618,13 +2618,14 @@ int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path *path,
 	struct extent_buffer *eb;
 
 	ASSERT(path);
+	ASSERT(found_key);
 
 	key.type = key_type;
 	key.objectid = iobjectid;
 	key.offset = ioff;
 
 	ret = btrfs_search_slot(NULL, fs_root, &key, path, 0, 0);
-	if ((ret < 0) || (found_key == NULL))
+	if (ret < 0)
 		return ret;
 
 	eb = path->nodes[0];

fs/btrfs/disk-io.c +6 -2

index 6182e5493d0f..8d486603e8a3 100644
@@ -1631,6 +1631,7 @@ struct btrfs_root *btrfs_get_fs_root(struct btrfs_fs_info *fs_info,
 {
 	struct btrfs_root *root;
 	struct btrfs_path *path;
+	struct btrfs_key key;
 	int ret;
 
 	if (location->objectid == BTRFS_ROOT_TREE_OBJECTID)

@@ -1675,8 +1676,11 @@ again:
 		ret = -ENOMEM;
 		goto fail;
 	}
-	ret = btrfs_find_item(fs_info->tree_root, path, BTRFS_ORPHAN_OBJECTID,
-			location->objectid, BTRFS_ORPHAN_ITEM_KEY, NULL);
+	key.objectid = BTRFS_ORPHAN_OBJECTID;
+	key.type = BTRFS_ORPHAN_ITEM_KEY;
+	key.offset = location->objectid;
+
+	ret = btrfs_search_slot(NULL, fs_info->tree_root, &key, path, 0, 0);
 	btrfs_free_path(path);
 	if (ret < 0)
 		goto fail;

fs/btrfs/inode.c +7 -3

index e687bb0dc73a..cb69db7c273c 100644
@@ -5009,6 +5009,7 @@ static int fixup_tree_root_location(struct btrfs_root *root,
 	struct btrfs_root *new_root;
 	struct btrfs_root_ref *ref;
 	struct extent_buffer *leaf;
+	struct btrfs_key key;
 	int ret;
 	int err = 0;

@@ -5019,9 +5020,12 @@ static int fixup_tree_root_location(struct btrfs_root *root,
 	}
 
 	err = -ENOENT;
-	ret = btrfs_find_item(root->fs_info->tree_root, path,
-				BTRFS_I(dir)->root->root_key.objectid,
-				location->objectid, BTRFS_ROOT_REF_KEY, NULL);
+	key.objectid = BTRFS_I(dir)->root->root_key.objectid;
+	key.type = BTRFS_ROOT_REF_KEY;
+	key.offset = location->objectid;
+
+	ret = btrfs_search_slot(NULL, root->fs_info->tree_root, &key, path,
+				0, 0);
 	if (ret) {
 		if (ret < 0)
 			err = ret;

fs/btrfs/scrub.c +6 -2

index 8bfe3b6fd471..0baf3b50c61f 100644
@@ -520,6 +520,7 @@ static int scrub_print_warning_inode(u64 inum, u64 offset, u64 root,
 	struct inode_fs_paths *ipath = NULL;
 	struct btrfs_root *local_root;
 	struct btrfs_key root_key;
+	struct btrfs_key key;
 
 	root_key.objectid = root;
 	root_key.type = BTRFS_ROOT_ITEM_KEY;

@@ -533,8 +534,11 @@ static int scrub_print_warning_inode(u64 inum, u64 offset, u64 root,
 	/*
 	 * this makes the path point to (inum INODE_ITEM ioff)
 	 */
-	ret = btrfs_find_item(local_root, swarn->path, inum, 0,
-			BTRFS_INODE_ITEM_KEY, NULL);
+	key.objectid = inum;
+	key.type = BTRFS_INODE_ITEM_KEY;
+	key.offset = 0;
+
+	ret = btrfs_search_slot(NULL, local_root, &key, swarn->path, 0, 0);
 	if (ret) {
 		btrfs_release_path(swarn->path);
 		goto err;

Signature · 1 line

1.8.4.5