I think "btrfs: fix leak of path in btrfs_find_item" broke stable trees ...

I think "btrfs: fix leak of path in btrfs_find_item" broke stable trees ...

[Eric Sandeen]

Looks like "btrfs: fix leak of path in btrfs_find_item" got sent
to stable trees, but in my testing, it causes deadlocks on mount:

[23379.359246] mount           D 0000000000000000     0 22541  22274 0x00000080
[23379.366326]  ffff8803ebadf6c8 0000000000000086 ffff88027ff10230 0000000000013680
[23379.373770]  0000000000013680 ffff8803ebadffd8 ffff8803ebadc010 0000000000013680
[23379.381208]  ffff8803ebadffd8 0000000000013680 ffff880261c78b60 ffff8802140a0b60
[23379.388648] Call Trace:
[23379.391106]  [<ffffffff816182b9>] schedule+0x29/0x70
[23379.396091]  [<ffffffffa06b82f5>] btrfs_tree_lock+0xb5/0x290 [btrfs]
[23379.402444]  [<ffffffff8109b470>] ? wake_up_bit+0x40/0x40
[23379.407855]  [<ffffffffa064f7a5>] ? generic_bin_search+0xf5/0x180 [btrfs]
[23379.414643]  [<ffffffffa065041b>] btrfs_lock_root_node+0x3b/0x50 [btrfs]
[23379.421345]  [<ffffffffa065936b>] btrfs_search_slot+0x63b/0x800 [btrfs]
[23379.427956]  [<ffffffffa064fa49>] ? btrfs_set_path_blocking+0x39/0x80 [btrfs]
[23379.435088]  [<ffffffffa0659ede>] btrfs_insert_empty_items+0x7e/0xe0 [btrfs]
[23379.442125]  [<ffffffffa065051a>] ? btrfs_alloc_path+0x1a/0x20 [btrfs]
[23379.448655]  [<ffffffffa06b8989>] btrfs_insert_orphan_item+0x69/0x90 [btrfs]
[23379.455696]  [<ffffffffa06ba938>] insert_orphan_item+0x68/0x90 [btrfs]
[23379.462251]  [<ffffffffa06bf772>] replay_one_buffer+0x372/0x380 [btrfs]
[23379.468878]  [<ffffffffa069a4c1>] ? mark_extent_buffer_accessed+0x51/0x70 [btrfs]
[23379.476372]  [<ffffffffa06ba54b>] walk_up_log_tree+0x1cb/0x250 [btrfs]
[23379.482910]  [<ffffffffa06ba68f>] walk_log_tree+0xbf/0x1b0 [btrfs]
[23379.489098]  [<ffffffffa06bd51c>] btrfs_recover_log_trees+0x1ec/0x4c0 [btrfs]
...

I could hit this by running ./check generic/015  generic/039 in fstests,
with a SCRATCH_DEV_POOL defined (not sure it matters, it's just what I
have...)

This fixes it, though I'm not totally sure why.  Refcounts?

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 906934e..d37e6d1 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -1265,11 +1265,11 @@ static int insert_orphan_item(struct btrfs_trans_handle *trans,
 
 	ret = btrfs_find_item(root, path, BTRFS_ORPHAN_OBJECTID,
 			offset, BTRFS_ORPHAN_ITEM_KEY, NULL);
+	btrfs_free_path(path);
+
 	if (ret > 0)
 		ret = btrfs_insert_orphan_item(trans, root, offset);
 
-	btrfs_free_path(path);
-
 	return ret;
 }
 

but it never likely showed up upstream, because 

9c4f61f btrfs: simplify insert_orphan_item

made the whole path alloc/free go away.

-Eric

Re: I think "btrfs: fix leak of path in btrfs_find_item" broke stable trees ...

[Filipe David Manana]

On Thu, Mar 26, 2015 at 3:24 AM, Eric Sandeen <sandeen@redhat.com> wrote:
> Looks like "btrfs: fix leak of path in btrfs_find_item" got sent
> to stable trees, but in my testing, it causes deadlocks on mount:
>
> [23379.359246] mount           D 0000000000000000     0 22541  22274 0x00000080
> [23379.366326]  ffff8803ebadf6c8 0000000000000086 ffff88027ff10230 0000000000013680
> [23379.373770]  0000000000013680 ffff8803ebadffd8 ffff8803ebadc010 0000000000013680
> [23379.381208]  ffff8803ebadffd8 0000000000013680 ffff880261c78b60 ffff8802140a0b60
> [23379.388648] Call Trace:
> [23379.391106]  [<ffffffff816182b9>] schedule+0x29/0x70
> [23379.396091]  [<ffffffffa06b82f5>] btrfs_tree_lock+0xb5/0x290 [btrfs]
> [23379.402444]  [<ffffffff8109b470>] ? wake_up_bit+0x40/0x40
> [23379.407855]  [<ffffffffa064f7a5>] ? generic_bin_search+0xf5/0x180 [btrfs]
> [23379.414643]  [<ffffffffa065041b>] btrfs_lock_root_node+0x3b/0x50 [btrfs]
> [23379.421345]  [<ffffffffa065936b>] btrfs_search_slot+0x63b/0x800 [btrfs]
> [23379.427956]  [<ffffffffa064fa49>] ? btrfs_set_path_blocking+0x39/0x80 [btrfs]
> [23379.435088]  [<ffffffffa0659ede>] btrfs_insert_empty_items+0x7e/0xe0 [btrfs]
> [23379.442125]  [<ffffffffa065051a>] ? btrfs_alloc_path+0x1a/0x20 [btrfs]
> [23379.448655]  [<ffffffffa06b8989>] btrfs_insert_orphan_item+0x69/0x90 [btrfs]
> [23379.455696]  [<ffffffffa06ba938>] insert_orphan_item+0x68/0x90 [btrfs]
> [23379.462251]  [<ffffffffa06bf772>] replay_one_buffer+0x372/0x380 [btrfs]
> [23379.468878]  [<ffffffffa069a4c1>] ? mark_extent_buffer_accessed+0x51/0x70 [btrfs]
> [23379.476372]  [<ffffffffa06ba54b>] walk_up_log_tree+0x1cb/0x250 [btrfs]
> [23379.482910]  [<ffffffffa06ba68f>] walk_log_tree+0xbf/0x1b0 [btrfs]
> [23379.489098]  [<ffffffffa06bd51c>] btrfs_recover_log_trees+0x1ec/0x4c0 [btrfs]
> ...
>
> I could hit this by running ./check generic/015  generic/039 in fstests,
> with a SCRATCH_DEV_POOL defined (not sure it matters, it's just what I
> have...)
>
> This fixes it, though I'm not totally sure why.  Refcounts?

Hi Eric,

Your patch seems correct to me.
The problem is that btrfs_insert_orphan_item tries to get a write lock
on the same node/leaf for which its caller (insert_orphan_item) is
already holding a read lock.

If you plan to submit a proper patch, feel free to add my Reviewed-by:
Filipe Manana <fdmanana@suse.com>

thanks

>
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 906934e..d37e6d1 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -1265,11 +1265,11 @@ static int insert_orphan_item(struct btrfs_trans_handle *trans,
>
>         ret = btrfs_find_item(root, path, BTRFS_ORPHAN_OBJECTID,
>                         offset, BTRFS_ORPHAN_ITEM_KEY, NULL);
> +       btrfs_free_path(path);
> +
>         if (ret > 0)
>                 ret = btrfs_insert_orphan_item(trans, root, offset);
>
> -       btrfs_free_path(path);
> -
>         return ret;
>  }
>
>
> but it never likely showed up upstream, because
>
> 9c4f61f btrfs: simplify insert_orphan_item
>
> made the whole path alloc/free go away.
>
> -Eric
> --
> 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



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: I think "btrfs: fix leak of path in btrfs_find_item" broke stable trees ...

[Eric Sandeen]

On 3/26/15 5:23 AM, Filipe David Manana wrote:
> On Thu, Mar 26, 2015 at 3:24 AM, Eric Sandeen <sandeen@redhat.com> wrote:
>> Looks like "btrfs: fix leak of path in btrfs_find_item" got sent
>> to stable trees, but in my testing, it causes deadlocks on mount:
>>
>> [23379.359246] mount           D 0000000000000000     0 22541  22274 0x00000080
>> [23379.366326]  ffff8803ebadf6c8 0000000000000086 ffff88027ff10230 0000000000013680
>> [23379.373770]  0000000000013680 ffff8803ebadffd8 ffff8803ebadc010 0000000000013680
>> [23379.381208]  ffff8803ebadffd8 0000000000013680 ffff880261c78b60 ffff8802140a0b60
>> [23379.388648] Call Trace:
>> [23379.391106]  [<ffffffff816182b9>] schedule+0x29/0x70
>> [23379.396091]  [<ffffffffa06b82f5>] btrfs_tree_lock+0xb5/0x290 [btrfs]
>> [23379.402444]  [<ffffffff8109b470>] ? wake_up_bit+0x40/0x40
>> [23379.407855]  [<ffffffffa064f7a5>] ? generic_bin_search+0xf5/0x180 [btrfs]
>> [23379.414643]  [<ffffffffa065041b>] btrfs_lock_root_node+0x3b/0x50 [btrfs]
>> [23379.421345]  [<ffffffffa065936b>] btrfs_search_slot+0x63b/0x800 [btrfs]
>> [23379.427956]  [<ffffffffa064fa49>] ? btrfs_set_path_blocking+0x39/0x80 [btrfs]
>> [23379.435088]  [<ffffffffa0659ede>] btrfs_insert_empty_items+0x7e/0xe0 [btrfs]
>> [23379.442125]  [<ffffffffa065051a>] ? btrfs_alloc_path+0x1a/0x20 [btrfs]
>> [23379.448655]  [<ffffffffa06b8989>] btrfs_insert_orphan_item+0x69/0x90 [btrfs]
>> [23379.455696]  [<ffffffffa06ba938>] insert_orphan_item+0x68/0x90 [btrfs]
>> [23379.462251]  [<ffffffffa06bf772>] replay_one_buffer+0x372/0x380 [btrfs]
>> [23379.468878]  [<ffffffffa069a4c1>] ? mark_extent_buffer_accessed+0x51/0x70 [btrfs]
>> [23379.476372]  [<ffffffffa06ba54b>] walk_up_log_tree+0x1cb/0x250 [btrfs]
>> [23379.482910]  [<ffffffffa06ba68f>] walk_log_tree+0xbf/0x1b0 [btrfs]
>> [23379.489098]  [<ffffffffa06bd51c>] btrfs_recover_log_trees+0x1ec/0x4c0 [btrfs]
>> ...
>>
>> I could hit this by running ./check generic/015  generic/039 in fstests,
>> with a SCRATCH_DEV_POOL defined (not sure it matters, it's just what I
>> have...)
>>
>> This fixes it, though I'm not totally sure why.  Refcounts?
> 
> Hi Eric,
> 
> Your patch seems correct to me.
> The problem is that btrfs_insert_orphan_item tries to get a write lock
> on the same node/leaf for which its caller (insert_orphan_item) is
> already holding a read lock.
> 
> If you plan to submit a proper patch, feel free to add my Reviewed-by:
> Filipe Manana <fdmanana@suse.com>

Thanks; well -

>> but it never likely showed up upstream, because
>>
>> 9c4f61f btrfs: simplify insert_orphan_item
>>
>> made the whole path alloc/free go away.

so I think there's no need for my patch; may as well just send the above to stable
and fix it that way, as long as 9c4f61f is deemed safe & correct, I think.

-Eric

Re: I think "btrfs: fix leak of path in btrfs_find_item" broke stable trees ...

[Chris Mason]

On Thu, Mar 26, 2015 at 10:11 AM, Eric Sandeen <sandeen@redhat.com> 
wrote:
> On 3/26/15 5:23 AM, Filipe David Manana wrote:
>>  On Thu, Mar 26, 2015 at 3:24 AM, Eric Sandeen <sandeen@redhat.com> 
>> wrote:
>>>  Looks like "btrfs: fix leak of path in btrfs_find_item" got sent
>>>  to stable trees, but in my testing, it causes deadlocks on mount:
>>> 
>>>  [23379.359246] mount           D 0000000000000000     0 22541  
>>> 22274 0x00000080
>>>  [23379.366326]  ffff8803ebadf6c8 0000000000000086 ffff88027ff10230 
>>> 0000000000013680
>>>  [23379.373770]  0000000000013680 ffff8803ebadffd8 ffff8803ebadc010 
>>> 0000000000013680
>>>  [23379.381208]  ffff8803ebadffd8 0000000000013680 ffff880261c78b60 
>>> ffff8802140a0b60
>>>  [23379.388648] Call Trace:
>>>  [23379.391106]  [<ffffffff816182b9>] schedule+0x29/0x70
>>>  [23379.396091]  [<ffffffffa06b82f5>] btrfs_tree_lock+0xb5/0x290 
>>> [btrfs]
>>>  [23379.402444]  [<ffffffff8109b470>] ? wake_up_bit+0x40/0x40
>>>  [23379.407855]  [<ffffffffa064f7a5>] ? 
>>> generic_bin_search+0xf5/0x180 [btrfs]
>>>  [23379.414643]  [<ffffffffa065041b>] 
>>> btrfs_lock_root_node+0x3b/0x50 [btrfs]
>>>  [23379.421345]  [<ffffffffa065936b>] btrfs_search_slot+0x63b/0x800 
>>> [btrfs]
>>>  [23379.427956]  [<ffffffffa064fa49>] ? 
>>> btrfs_set_path_blocking+0x39/0x80 [btrfs]
>>>  [23379.435088]  [<ffffffffa0659ede>] 
>>> btrfs_insert_empty_items+0x7e/0xe0 [btrfs]
>>>  [23379.442125]  [<ffffffffa065051a>] ? btrfs_alloc_path+0x1a/0x20 
>>> [btrfs]
>>>  [23379.448655]  [<ffffffffa06b8989>] 
>>> btrfs_insert_orphan_item+0x69/0x90 [btrfs]
>>>  [23379.455696]  [<ffffffffa06ba938>] insert_orphan_item+0x68/0x90 
>>> [btrfs]
>>>  [23379.462251]  [<ffffffffa06bf772>] replay_one_buffer+0x372/0x380 
>>> [btrfs]
>>>  [23379.468878]  [<ffffffffa069a4c1>] ? 
>>> mark_extent_buffer_accessed+0x51/0x70 [btrfs]
>>>  [23379.476372]  [<ffffffffa06ba54b>] walk_up_log_tree+0x1cb/0x250 
>>> [btrfs]
>>>  [23379.482910]  [<ffffffffa06ba68f>] walk_log_tree+0xbf/0x1b0 
>>> [btrfs]
>>>  [23379.489098]  [<ffffffffa06bd51c>] 
>>> btrfs_recover_log_trees+0x1ec/0x4c0 [btrfs]
>>>  ...
>>> 
>>>  I could hit this by running ./check generic/015  generic/039 in 
>>> fstests,
>>>  with a SCRATCH_DEV_POOL defined (not sure it matters, it's just 
>>> what I
>>>  have...)
>>> 
>>>  This fixes it, though I'm not totally sure why.  Refcounts?
>> 
>>  Hi Eric,
>> 
>>  Your patch seems correct to me.
>>  The problem is that btrfs_insert_orphan_item tries to get a write 
>> lock
>>  on the same node/leaf for which its caller (insert_orphan_item) is
>>  already holding a read lock.
>> 
>>  If you plan to submit a proper patch, feel free to add my 
>> Reviewed-by:
>>  Filipe Manana <fdmanana@suse.com>
> 
> Thanks; well -
> 
>>>  but it never likely showed up upstream, because
>>> 
>>>  9c4f61f btrfs: simplify insert_orphan_item
>>> 
>>>  made the whole path alloc/free go away.
> 
> so I think there's no need for my patch; may as well just send the 
> above to stable
> and fix it that way, as long as 9c4f61f is deemed safe & correct, I 
> think.

Nice catch, thanks Eric.  9c4f61f looks fine for stable to me, but 
since he's already testing on stable, I talked Eric into giving it a 
pass through xfstests before I send it up.

-chris

Re: I think "btrfs: fix leak of path in btrfs_find_item" broke stable trees ...

[Eric Sandeen]

On 3/26/15 9:48 AM, Chris Mason wrote:
> On Thu, Mar 26, 2015 at 10:11 AM, Eric Sandeen <sandeen@redhat.com> wrote:

...

>>>>  9c4f61f btrfs: simplify insert_orphan_item
>>>>
>>>>  made the whole path alloc/free go away.
>>
>> so I think there's no need for my patch; may as well just send the above to stable
>> and fix it that way, as long as 9c4f61f is deemed safe & correct, I think.
> 
> Nice catch, thanks Eric. 9c4f61f looks fine for stable to me, but
> since he's already testing on stable, I talked Eric into giving it a
> pass through xfstests before I send it up.
> 
> -chris

./check -g auto on 3.19-stable-ish seems fine-ish.  Certainly no worse w/ the patch added :)

Failures: btrfs/010 btrfs/017 btrfs/078 generic/015 generic/039 generic/040 generic/041 generic/065 generic/066 generic/071 generic/204
Failed 11 of 202 tests

I'd say ship it!

-Eric

Re: I think "btrfs: fix leak of path in btrfs_find_item" broke stable trees ...

[Filipe David Manana]

On Thu, Mar 26, 2015 at 5:11 PM, Eric Sandeen <sandeen@redhat.com> wrote:
> On 3/26/15 9:48 AM, Chris Mason wrote:
>> On Thu, Mar 26, 2015 at 10:11 AM, Eric Sandeen <sandeen@redhat.com> wrote:
>
> ...
>
>>>>>  9c4f61f btrfs: simplify insert_orphan_item
>>>>>
>>>>>  made the whole path alloc/free go away.
>>>
>>> so I think there's no need for my patch; may as well just send the above to stable
>>> and fix it that way, as long as 9c4f61f is deemed safe & correct, I think.
>>
>> Nice catch, thanks Eric. 9c4f61f looks fine for stable to me, but
>> since he's already testing on stable, I talked Eric into giving it a
>> pass through xfstests before I send it up.
>>
>> -chris
>
> ./check -g auto on 3.19-stable-ish seems fine-ish.  Certainly no worse w/ the patch added :)
>
> Failures: btrfs/010 btrfs/017 btrfs/078 generic/015 generic/039 generic/040 generic/041 generic/065 generic/066 generic/071 generic/204
> Failed 11 of 202 tests

Just curious, how did btrfs/078 fail? It isn't supposed to fail on
3.19.x nor 3.18.x.

>
> I'd say ship it!
>
> -Eric
>



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: I think "btrfs: fix leak of path in btrfs_find_item" broke stable trees ...

[Eric Sandeen]

On 3/26/15 12:25 PM, Filipe David Manana wrote:
> On Thu, Mar 26, 2015 at 5:11 PM, Eric Sandeen <sandeen@redhat.com> wrote:
>> On 3/26/15 9:48 AM, Chris Mason wrote:
>>> On Thu, Mar 26, 2015 at 10:11 AM, Eric Sandeen <sandeen@redhat.com> wrote:
>>
>> ...
>>
>>>>>>  9c4f61f btrfs: simplify insert_orphan_item
>>>>>>
>>>>>>  made the whole path alloc/free go away.
>>>>
>>>> so I think there's no need for my patch; may as well just send the above to stable
>>>> and fix it that way, as long as 9c4f61f is deemed safe & correct, I think.
>>>
>>> Nice catch, thanks Eric. 9c4f61f looks fine for stable to me, but
>>> since he's already testing on stable, I talked Eric into giving it a
>>> pass through xfstests before I send it up.
>>>
>>> -chris
>>
>> ./check -g auto on 3.19-stable-ish seems fine-ish.  Certainly no worse w/ the patch added :)
>>
>> Failures: btrfs/010 btrfs/017 btrfs/078 generic/015 generic/039 generic/040 generic/041 generic/065 generic/066 generic/071 generic/204
>> Failed 11 of 202 tests
> 
> Just curious, how did btrfs/078 fail? It isn't supposed to fail on
> 3.19.x nor 3.18.x.

This was the use after free / segfault / valgrind splat from userspace that I reported yesterday.

btrfs/078 104s ... 106s
*** glibc detected *** btrfs check: double free or corruption (fasttop): 0x0000000000c12a40 ***
...
./common/rc: line 1932: 43334 Aborted                 btrfsck $device > $tmp.fsck 2>&1
_check_btrfs_filesystem: filesystem on /dev/sdc5 is inconsistent (see /mnt/test2/git/xfstests/results//btrfs/078.full)
Ran: btrfs/078
Failures: btrfs/078
Failed 1 of 1 tests

-Eric

Re: I think "btrfs: fix leak of path in btrfs_find_item" broke stable trees ...

[Eryu Guan]

On Thu, Mar 26, 2015 at 12:11:51PM -0500, Eric Sandeen wrote:
> On 3/26/15 9:48 AM, Chris Mason wrote:
> > On Thu, Mar 26, 2015 at 10:11 AM, Eric Sandeen <sandeen@redhat.com> wrote:
> 
> ...
> 
> >>>>  9c4f61f btrfs: simplify insert_orphan_item
> >>>>
> >>>>  made the whole path alloc/free go away.
> >>
> >> so I think there's no need for my patch; may as well just send the above to stable
> >> and fix it that way, as long as 9c4f61f is deemed safe & correct, I think.
> > 
> > Nice catch, thanks Eric. 9c4f61f looks fine for stable to me, but
> > since he's already testing on stable, I talked Eric into giving it a
> > pass through xfstests before I send it up.
> > 
> > -chris
> 
> ./check -g auto on 3.19-stable-ish seems fine-ish.  Certainly no worse w/ the patch added :)
> 
> Failures: btrfs/010 btrfs/017 btrfs/078 generic/015 generic/039 generic/040 generic/041 generic/065 generic/066 generic/071 generic/204
> Failed 11 of 202 tests

Just FYI.

I think generic/204 is a test case issue, _filter_mkfs failed to print
isize and dbsize for btrfs and test failed because of divide by zero
error.

--- /dev/fd/63	2015-03-25 12:17:05.987107715 -0400
+++ results/generic/204.out.bad		      2015-03-25 12:17:05.423101244 -0400
@@ -1,2 +1,3 @@
 QA output created by 204
 +./tests/generic/204: line 76: space / (isize + dbsize): division by 0 (error token is ")")
  *** done

I'm working a patch for fstests.

Eryu

Re: I think "btrfs: fix leak of path in btrfs_find_item" broke stable trees ...

[Eric Sandeen]

On 3/26/15 10:34 PM, Eryu Guan wrote:

> Just FYI.
> 
> I think generic/204 is a test case issue, _filter_mkfs failed to print
> isize and dbsize for btrfs and test failed because of divide by zero
> error.
> 
> --- /dev/fd/63	2015-03-25 12:17:05.987107715 -0400
> +++ results/generic/204.out.bad		      2015-03-25 12:17:05.423101244 -0400
> @@ -1,2 +1,3 @@
>  QA output created by 204
>  +./tests/generic/204: line 76: space / (isize + dbsize): division by 0 (error token is ")")
>   *** done
> 
> I'm working a patch for fstests.

Sorry, I beat you to it :)

-Eric