btrfs: delayed-ref: simplify btrfs_add_delayed_tree_ref()
diff mbox

Message ID 20180523080650.28286-1-suy.fnst@cn.fujitsu.com
State New
Headers show

Commit Message

Su Yue May 23, 2018, 8:06 a.m. UTC
Commit 5a5003df98d5 ("btrfs: delayed-ref: double free in
btrfs_add_delayed_tree_ref()") fixed double free problem by creating
an unnessesary label to jump.
The elegant way is just to change "ref" to "head_ref" and keep
btrfs_add_delayed_tree_ref() and btrfs_add_delayed_data_ref() in
similar structure.

This patch reverts commit 5a5003df98d5 ("btrfs: delayed-ref: double
free in btrfs_add_delayed_tree_ref()") and frees the right head_ref.
No functional change.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
This patch is based on for-next to avoid conflicts with patches
already in for-next.

 fs/btrfs/delayed-ref.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

Comments

Nikolay Borisov May 23, 2018, 8:22 a.m. UTC | #1
On 23.05.2018 11:06, Su Yue wrote:
> Commit 5a5003df98d5 ("btrfs: delayed-ref: double free in
> btrfs_add_delayed_tree_ref()") fixed double free problem by creating
> an unnessesary label to jump.
> The elegant way is just to change "ref" to "head_ref" and keep
> btrfs_add_delayed_tree_ref() and btrfs_add_delayed_data_ref() in
> similar structure.

I agree, personally I'm a fan of multiple returns rather than jump
labels, because at this point you know the function terminates and
that's it.

> 
> This patch reverts commit 5a5003df98d5 ("btrfs: delayed-ref: double
> free in btrfs_add_delayed_tree_ref()") and frees the right head_ref.
> No functional change.
> 
> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
> This patch is based on for-next to avoid conflicts with patches
> already in for-next.
> 
>  fs/btrfs/delayed-ref.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
> index 03dec673d12a..38f8d5d549ed 100644
> --- a/fs/btrfs/delayed-ref.c
> +++ b/fs/btrfs/delayed-ref.c
> @@ -741,14 +741,20 @@ int btrfs_add_delayed_tree_ref(struct btrfs_fs_info *fs_info,
>  	ref->level = level;
>  
>  	head_ref = kmem_cache_alloc(btrfs_delayed_ref_head_cachep, GFP_NOFS);
> -	if (!head_ref)
> -		goto free_ref;
> +	if (!head_ref) {
> +		kmem_cache_free(btrfs_delayed_tree_ref_cachep, ref);
> +		return -ENOMEM;
> +	}
>  
>  	if (test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags) &&
>  	    is_fstree(ref_root)) {
>  		record = kmalloc(sizeof(*record), GFP_NOFS);
> -		if (!record)
> -			goto free_head_ref;
> +		if (!record) {
> +			kmem_cache_free(btrfs_delayed_tree_ref_cachep, ref);
> +			kmem_cache_free(btrfs_delayed_ref_head_cachep,
> +					head_ref);
> +			return -ENOMEM;
> +		}
>  	}
>  
>  	init_delayed_ref_head(head_ref, record, bytenr, num_bytes,
> @@ -779,13 +785,6 @@ int btrfs_add_delayed_tree_ref(struct btrfs_fs_info *fs_info,
>  		btrfs_qgroup_trace_extent_post(fs_info, record);
>  
>  	return 0;
> -
> -free_head_ref:
> -	kmem_cache_free(btrfs_delayed_ref_head_cachep, head_ref);
> -free_ref:
> -	kmem_cache_free(btrfs_delayed_tree_ref_cachep, ref);
> -
> -	return -ENOMEM;
>  }
>  
>  /*
> 
--
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 June 20, 2018, 2:28 p.m. UTC | #2
On Wed, May 23, 2018 at 11:22:20AM +0300, Nikolay Borisov wrote:
> 
> 
> On 23.05.2018 11:06, Su Yue wrote:
> > Commit 5a5003df98d5 ("btrfs: delayed-ref: double free in
> > btrfs_add_delayed_tree_ref()") fixed double free problem by creating
> > an unnessesary label to jump.
> > The elegant way is just to change "ref" to "head_ref" and keep
> > btrfs_add_delayed_tree_ref() and btrfs_add_delayed_data_ref() in
> > similar structure.
> 
> I agree, personally I'm a fan of multiple returns rather than jump
> labels, because at this point you know the function terminates and
> that's it.

Ok, let's do the freeing in-place, but it would be better to put them
before any other code, which is init_delayed_ref_common() in this case.
--
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
Nikolay Borisov June 21, 2018, 8:33 a.m. UTC | #3
On 21.06.2018 11:38, Su Yue wrote:
> 
> 
> On 06/20/2018 11:43 PM, Nikolay Borisov wrote:
>> Currently the function uses 2 goto labels to properly handle allocation
>> failures. This could be simplified by simply re-arranging the code so
>> that allocations are the in the beginning of the function. This allows
>> to use simple return statements. No function changes.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> 
> Hi, Nikolay
> 
> I just saw the patch wasn't CCed to mail list.
> It seems that you forgot to CC?

Indeed I seem to have missed that. I will resend

> 
> Thanks,
> Su
> 
>> ---
>>
>> So how about something like that
>>
>>   fs/btrfs/delayed-ref.c | 35 +++++++++++++++++------------------
>>   1 file changed, 17 insertions(+), 18 deletions(-)
>>
>> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
>> index 03dec673d12a..c2c2634693e1 100644
>> --- a/fs/btrfs/delayed-ref.c
>> +++ b/fs/btrfs/delayed-ref.c
>> @@ -730,27 +730,33 @@ int btrfs_add_delayed_tree_ref(struct
>> btrfs_fs_info *fs_info,
>>       if (!ref)
>>           return -ENOMEM;
>>   +    head_ref = kmem_cache_alloc(btrfs_delayed_ref_head_cachep,
>> GFP_NOFS);
>> +    if (!head_ref) {
>> +        kmem_cache_free(btrfs_delayed_tree_ref_cachep, ref);
>> +        return -ENOMEM;
>> +    }
>> +
>> +    if (test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags) &&
>> +        is_fstree(ref_root)) {
>> +        record = kmalloc(sizeof(*record), GFP_NOFS);
>> +        if (!record) {
>> +            kmem_cache_free(btrfs_delayed_tree_ref_cachep, ref);
>> +            kmem_cache_free(btrfs_delayed_ref_head_cachep, head_ref);
>> +            return -ENOMEM;
>> +        }
>> +    }
>> +
>>       if (parent)
>>           ref_type = BTRFS_SHARED_BLOCK_REF_KEY;
>>       else
>>           ref_type = BTRFS_TREE_BLOCK_REF_KEY;
>> +
>>       init_delayed_ref_common(fs_info, &ref->node, bytenr, num_bytes,
>>                   ref_root, action, ref_type);
>>       ref->root = ref_root;
>>       ref->parent = parent;
>>       ref->level = level;
>>   -    head_ref = kmem_cache_alloc(btrfs_delayed_ref_head_cachep,
>> GFP_NOFS);
>> -    if (!head_ref)
>> -        goto free_ref;
>> -
>> -    if (test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags) &&
>> -        is_fstree(ref_root)) {
>> -        record = kmalloc(sizeof(*record), GFP_NOFS);
>> -        if (!record)
>> -            goto free_head_ref;
>> -    }
>> -
>>       init_delayed_ref_head(head_ref, record, bytenr, num_bytes,
>>                     ref_root, 0, action, false, is_system);
>>       head_ref->extent_op = extent_op;
>> @@ -779,13 +785,6 @@ int btrfs_add_delayed_tree_ref(struct
>> btrfs_fs_info *fs_info,
>>           btrfs_qgroup_trace_extent_post(fs_info, record);
>>         return 0;
>> -
>> -free_head_ref:
>> -    kmem_cache_free(btrfs_delayed_ref_head_cachep, head_ref);
>> -free_ref:
>> -    kmem_cache_free(btrfs_delayed_tree_ref_cachep, ref);
>> -
>> -    return -ENOMEM;
>>   }
>>     /*
>>
> 
> 
> 
--
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

Patch
diff mbox

diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index 03dec673d12a..38f8d5d549ed 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -741,14 +741,20 @@  int btrfs_add_delayed_tree_ref(struct btrfs_fs_info *fs_info,
 	ref->level = level;
 
 	head_ref = kmem_cache_alloc(btrfs_delayed_ref_head_cachep, GFP_NOFS);
-	if (!head_ref)
-		goto free_ref;
+	if (!head_ref) {
+		kmem_cache_free(btrfs_delayed_tree_ref_cachep, ref);
+		return -ENOMEM;
+	}
 
 	if (test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags) &&
 	    is_fstree(ref_root)) {
 		record = kmalloc(sizeof(*record), GFP_NOFS);
-		if (!record)
-			goto free_head_ref;
+		if (!record) {
+			kmem_cache_free(btrfs_delayed_tree_ref_cachep, ref);
+			kmem_cache_free(btrfs_delayed_ref_head_cachep,
+					head_ref);
+			return -ENOMEM;
+		}
 	}
 
 	init_delayed_ref_head(head_ref, record, bytenr, num_bytes,
@@ -779,13 +785,6 @@  int btrfs_add_delayed_tree_ref(struct btrfs_fs_info *fs_info,
 		btrfs_qgroup_trace_extent_post(fs_info, record);
 
 	return 0;
-
-free_head_ref:
-	kmem_cache_free(btrfs_delayed_ref_head_cachep, head_ref);
-free_ref:
-	kmem_cache_free(btrfs_delayed_tree_ref_cachep, ref);
-
-	return -ENOMEM;
 }
 
 /*