[PATCH] btrfs: qgroup: Fix possible leak in btrfs_add_qgroup_relation()

[PATCH] btrfs: qgroup: Fix possible leak in btrfs_add_qgroup_relation()

[Christian Engelmayer]

Commit 9c8b35b1ba21 ("btrfs: quota: Automatically update related qgroups or
mark INCONSISTENT flags when assigning/deleting a qgroup relations.")
introduced the allocation of a temporary ulist in function
btrfs_add_qgroup_relation() and added the corresponding cleanup to the out
path. However, the allocation was introduced before the src/dst level check
that directly returns. Fix the possible leakage of the ulist by moving the
allocation after the input validation. Detected by Coverity CID 1295988.

Signed-off-by: Christian Engelmayer <cengelma@gmx.at>
---
Compile tested only. Applies against linux-next.
---
 fs/btrfs/qgroup.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 3d6546581bb9..842ff86d4ae8 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1115,14 +1115,14 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans,
 	struct ulist *tmp;
 	int ret = 0;
 
-	tmp = ulist_alloc(GFP_NOFS);
-	if (!tmp)
-		return -ENOMEM;
-
 	/* Check the level of src and dst first */
 	if (btrfs_qgroup_level(src) >= btrfs_qgroup_level(dst))
 		return -EINVAL;
 
+	tmp = ulist_alloc(GFP_NOFS);
+	if (!tmp)
+		return -ENOMEM;
+
 	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	quota_root = fs_info->quota_root;
 	if (!quota_root) {
-- 
1.9.1

Re: [PATCH] btrfs: qgroup: Fix possible leak in btrfs_add_qgroup_relation()

[Qu Wenruo]

-------- Original Message  --------
Subject: [PATCH] btrfs: qgroup: Fix possible leak in 
btrfs_add_qgroup_relation()
From: Christian Engelmayer <cengelma@gmx.at>
To: <linux-btrfs@vger.kernel.org>
Date: 2015年05月02日 23:19

> Commit 9c8b35b1ba21 ("btrfs: quota: Automatically update related qgroups or
> mark INCONSISTENT flags when assigning/deleting a qgroup relations.")
> introduced the allocation of a temporary ulist in function
> btrfs_add_qgroup_relation() and added the corresponding cleanup to the out
> path. However, the allocation was introduced before the src/dst level check
> that directly returns. Fix the possible leakage of the ulist by moving the
> allocation after the input validation. Detected by Coverity CID 1295988.
>
> Signed-off-by: Christian Engelmayer <cengelma@gmx.at>
> ---
> Compile tested only. Applies against linux-next.
> ---
>   fs/btrfs/qgroup.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 3d6546581bb9..842ff86d4ae8 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1115,14 +1115,14 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans,
>   	struct ulist *tmp;
>   	int ret = 0;
>
> -	tmp = ulist_alloc(GFP_NOFS);
> -	if (!tmp)
> -		return -ENOMEM;
> -
>   	/* Check the level of src and dst first */
>   	if (btrfs_qgroup_level(src) >= btrfs_qgroup_level(dst))
>   		return -EINVAL;
>
> +	tmp = ulist_alloc(GFP_NOFS);
> +	if (!tmp)
> +		return -ENOMEM;
> +
>   	mutex_lock(&fs_info->qgroup_ioctl_lock);
>   	quota_root = fs_info->quota_root;
>   	if (!quota_root) {
>
Oh, my fault.
Thanks for catching it.

Qu

Re: [PATCH] btrfs: qgroup: Fix possible leak in btrfs_add_qgroup_relation()

[David Sterba]

On Sat, May 02, 2015 at 05:19:55PM +0200, Christian Engelmayer wrote:
> Commit 9c8b35b1ba21 ("btrfs: quota: Automatically update related qgroups or
> mark INCONSISTENT flags when assigning/deleting a qgroup relations.")
> introduced the allocation of a temporary ulist in function
> btrfs_add_qgroup_relation() and added the corresponding cleanup to the out
> path. However, the allocation was introduced before the src/dst level check
> that directly returns. Fix the possible leakage of the ulist by moving the
> allocation after the input validation. Detected by Coverity CID 1295988.
> 
> Signed-off-by: Christian Engelmayer <cengelma@gmx.at>
Reviewed-by: David Sterba <dsterba@suse.cz>