Btrfs devel - Jun 2012 - [patch] Btrfs: dereferencing free'd memory in panic message

We free "node" and then dereference it in the panic message on the
next
line.  I considered moving the kfree() after the panic given that panic
can return under certain configurations, but in the end I decided it
doesn''t matter if we leak a bit after a panic.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 790f492..c50d80a 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1239,7 +1239,6 @@ static int __must_check __add_reloc_root(struct btrfs_root
*root)
 			      node->bytenr, &node->rb_node);
 	spin_unlock(&rc->reloc_root_tree.lock);
 	if (rb_node) {
-		kfree(node);
 		btrfs_panic(root->fs_info, -EEXIST, "Duplicate root found "
 			    "for start=%llu while inserting into relocation "
 			    "tree\n", node->bytenr);
--
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 06/22/2012 03:14 AM, Dan Carpenter wrote:
> We free "node" and then dereference it in the panic message on
the next
> line.  I considered moving the kfree() after the panic given that panic
> can return under certain configurations, but in the end I decided it
> doesn''t matter if we leak a bit after a panic.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 790f492..c50d80a 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -1239,7 +1239,6 @@ static int __must_check __add_reloc_root(struct
btrfs_root *root)
>   			      node->bytenr, &node->rb_node);
>   	spin_unlock(&rc->reloc_root_tree.lock);
>   	if (rb_node) {
> -		kfree(node);
>   		btrfs_panic(root->fs_info, -EEXIST, "Duplicate root found
"
>   			    "for start=%llu while inserting into relocation "
>   			    "tree\n", node->bytenr);
Except btrfs_panic can not panic the box if it''s mounted to not panic
on
errors, so we still need to do the kfree afterwards.  Thanks,

Josef
--
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, Jun 22, 2012 at 09:09:04AM -0400, Josef Bacik
wrote:
> On 06/22/2012 03:14 AM, Dan Carpenter wrote:
> >We free "node" and then dereference it in the panic message
on the next
> >line.  I considered moving the kfree() after the panic given that panic
> >can return under certain configurations, but in the end I decided it
> >doesn''t matter if we leak a bit after a panic.
> >
> >Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> >diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> >index 790f492..c50d80a 100644
> >--- a/fs/btrfs/relocation.c
> >+++ b/fs/btrfs/relocation.c
> >@@ -1239,7 +1239,6 @@ static int __must_check __add_reloc_root(struct
btrfs_root *root)
> >  			      node->bytenr, &node->rb_node);
> >  	spin_unlock(&rc->reloc_root_tree.lock);
> >  	if (rb_node) {
> >-		kfree(node);
> >  		btrfs_panic(root->fs_info, -EEXIST, "Duplicate root found
"
> >  			    "for start=%llu while inserting into relocation "
> >  			    "tree\n", node->bytenr);
> 
> Except btrfs_panic can not panic the box if it''s mounted to not
> panic on errors, so we still need to do the kfree afterwards.
> Thanks,
Right.  I mentioned that in my change log, but I figured a one time
memory leak was the least of our concerns in that case.  I will
resend.  This should probably return -EEXIST here as well yes?

regards,
dan carpenter

--
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 06/22/2012 09:30 AM, Dan Carpenter wrote:
> On Fri, Jun 22, 2012 at 09:09:04AM -0400, Josef Bacik wrote:
>> On 06/22/2012 03:14 AM, Dan Carpenter wrote:
>>> We free "node" and then dereference it in the panic
message on the next
>>> line.  I considered moving the kfree() after the panic given that
panic
>>> can return under certain configurations, but in the end I decided
it
>>> doesn''t matter if we leak a bit after a panic.
>>>
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
>>> index 790f492..c50d80a 100644
>>> --- a/fs/btrfs/relocation.c
>>> +++ b/fs/btrfs/relocation.c
>>> @@ -1239,7 +1239,6 @@ static int __must_check
__add_reloc_root(struct btrfs_root *root)
>>>   			      node->bytenr, &node->rb_node);
>>>   	spin_unlock(&rc->reloc_root_tree.lock);
>>>   	if (rb_node) {
>>> -		kfree(node);
>>>   		btrfs_panic(root->fs_info, -EEXIST, "Duplicate root
found "
>>>   			    "for start=%llu while inserting into relocation
"
>>>   			    "tree\n", node->bytenr);
>>
>> Except btrfs_panic can not panic the box if it''s mounted to
not
>> panic on errors, so we still need to do the kfree afterwards.
>> Thanks,
>
> Right.  I mentioned that in my change log, but I figured a one time
> memory leak was the least of our concerns in that case.  I will
> resend.  This should probably return -EEXIST here as well yes?
>
Ah sorry I thought you were talking about BUG_ON() not actually stopping 
the box in some cases.  But yes I''d like to not leak anyway and return 
-EEXIST.  Thanks,

Josef
--
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

We dereferenced "node" in the error message after freeing it.  Also
btrfs_panic() can return so we should return an error code instead of
continuing.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: in the first version I just deleted the kfree().

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 790f492..4da0865 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1239,10 +1239,11 @@ static int __must_check __add_reloc_root(struct
btrfs_root *root)
 			      node->bytenr, &node->rb_node);
 	spin_unlock(&rc->reloc_root_tree.lock);
 	if (rb_node) {
-		kfree(node);
 		btrfs_panic(root->fs_info, -EEXIST, "Duplicate root found "
 			    "for start=%llu while inserting into relocation "
 			    "tree\n", node->bytenr);
+		kfree(node);
+		return -EEXIST;
 	}
 
 	list_add_tail(&root->root_list, &rc->reloc_roots);
--
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 Mon, Jun 25, 2012 at 05:15:23AM -0600, Dan Carpenter
wrote:
> We dereferenced "node" in the error message after freeing it. 
Also
> btrfs_panic() can return so we should return an error code instead of
> continuing.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: in the first version I just deleted the kfree().
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 790f492..4da0865 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -1239,10 +1239,11 @@ static int __must_check __add_reloc_root(struct
btrfs_root *root)
>  			      node->bytenr, &node->rb_node);
>  	spin_unlock(&rc->reloc_root_tree.lock);
>  	if (rb_node) {
> -		kfree(node);
>  		btrfs_panic(root->fs_info, -EEXIST, "Duplicate root found "
>  			    "for start=%llu while inserting into relocation "
>  			    "tree\n", node->bytenr);
> +		kfree(node);
> +		return -EEXIST;
>  	}
>  
>  	list_add_tail(&root->root_list, &rc->reloc_roots);
I''m not sure why but it seems like this patch is wrapped.  At first I
thought it
was thunderbird but then I opened it up and saved it from mutt and I got the
same problem.  I will fix it up, but you may want to check your mail client.
Thanks,

Josef
--
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 Mon, Jun 25, 2012 at 09:41:12AM -0400, Josef Bacik
wrote:
> On Mon, Jun 25, 2012 at 05:15:23AM -0600, Dan Carpenter wrote:
> > We dereferenced "node" in the error message after freeing
it.  Also
> > btrfs_panic() can return so we should return an error code instead of
> > continuing.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > v2: in the first version I just deleted the kfree().
> > 
> > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> > index 790f492..4da0865 100644
> > --- a/fs/btrfs/relocation.c
> > +++ b/fs/btrfs/relocation.c
> > @@ -1239,10 +1239,11 @@ static int __must_check
__add_reloc_root(struct btrfs_root *root)
> >  			      node->bytenr, &node->rb_node);
> >  	spin_unlock(&rc->reloc_root_tree.lock);
> >  	if (rb_node) {
> > -		kfree(node);
> >  		btrfs_panic(root->fs_info, -EEXIST, "Duplicate root found
"
> >  			    "for start=%llu while inserting into relocation "
> >  			    "tree\n", node->bytenr);
> > +		kfree(node);
> > +		return -EEXIST;
> >  	}
> >  
> >  	list_add_tail(&root->root_list, &rc->reloc_roots);
> 
> I''m not sure why but it seems like this patch is wrapped.  At
first I thought it
> was thunderbird but then I opened it up and saved it from mutt and I got
the
> same problem.  I will fix it up, but you may want to check your mail
client.
The message that went through to kernel-janitors applies fine for
me.  I''m not sure what to say.

regards,
dan carpenter

--
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

I am also facing similar issue while applying this patch.

[santosh@localhost linux-next]$ sudo git am mail_Dan.txt
Patch format detection failed.
[santosh@localhost linux-next]$




regards
santosh

On Mon, Jun 25, 2012 at 7:23 PM, Dan Carpenter <dan.carpenter@oracle.com>
wrote:
> On Mon, Jun 25, 2012 at 09:41:12AM -0400, Josef Bacik wrote:
>> On Mon, Jun 25, 2012 at 05:15:23AM -0600, Dan Carpenter wrote:
>> > We dereferenced "node" in the error message after
freeing it.  Also
>> > btrfs_panic() can return so we should return an error code instead
of
>> > continuing.
>> >
>> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>> > ---
>> > v2: in the first version I just deleted the kfree().
>> >
>> > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
>> > index 790f492..4da0865 100644
>> > --- a/fs/btrfs/relocation.c
>> > +++ b/fs/btrfs/relocation.c
>> > @@ -1239,10 +1239,11 @@ static int __must_check
__add_reloc_root(struct btrfs_root *root)
>> >                           node->bytenr, &node->rb_node);
>> >     spin_unlock(&rc->reloc_root_tree.lock);
>> >     if (rb_node) {
>> > -           kfree(node);
>> >             btrfs_panic(root->fs_info, -EEXIST, "Duplicate
root found "
>> >                         "for start=%llu while inserting into
relocation "
>> >                         "tree\n", node->bytenr);
>> > +           kfree(node);
>> > +           return -EEXIST;
>> >     }
>> >
>> >     list_add_tail(&root->root_list,
&rc->reloc_roots);
>>
>> I''m not sure why but it seems like this patch is wrapped.  At
first I thought it
>> was thunderbird but then I opened it up and saved it from mutt and I
got the
>> same problem.  I will fix it up, but you may want to check your mail
client.
>
> The message that went through to kernel-janitors applies fine for
> me.  I''m not sure what to say.
>
> regards,
> dan carpenter
>
> --
> To unsubscribe from this list: send the line "unsubscribe
kernel-janitors" 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 Tue, Jun 26, 2012 at 06:51:39AM +0530, santosh prasad nayak
wrote:
> I am also facing similar issue while applying this patch.
> 
> [santosh@localhost linux-next]$ sudo git am mail_Dan.txt
> Patch format detection failed.
> [santosh@localhost linux-next]$
> 
The problem is not on my end.  It made it to marc.info without
getting corrupted.  Marc.info strips out the From: and Subject:
headers so you''d have to add them in manually.

1) Save this file:
http://marc.info/?l=kernel-janitors&m=134062314509635&q=raw

2) Edit the file and add these two lines add the top:

From: Dan Carpenter <dan.carpenter@oracle.com>
Subject: [patch v2] Btrfs: fix error handling in __add_reloc_root()

3) $ git am /home/dcarpenter/tmp/html2/btrfs_raw.txt
Applying: Btrfs: fix error handling in __add_reloc_root()

regards,
dan carpenter


--
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