* "btrfs rescue super-recover" memory corruption
@ 2014-09-17 17:00 Eric Sandeen
2014-09-17 17:01 ` Eric Sandeen
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Eric Sandeen @ 2014-09-17 17:00 UTC (permalink / raw)
To: linux-btrfs, Shilong Wang, Chris Murphy
This:
# truncate --size=8g
# dd if=/dev/zero of=file conv=notrunc bs=4 seek=16384 count=1
# valgrind ./btrfs rescue super-recover file -v
yields:
==4604== Memcheck, a memory error detector
==4604== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==4604== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==4604== Command: ./btrfs rescue super-recover file -v
==4604==
All Devices:
Device: id = 1, name = file
Before Recovering:
[All good supers]:
device name = file
superblock bytenr = 67108864
[All bad supers]:
device name = file
superblock bytenr = 65536
Make sure this is a btrfs disk otherwise the tool will destroy other fs, Are you sure? [y/N]: y
Recovered bad superblocks successful
==4604== Invalid read of size 8
==4604== at 0x426B55: btrfs_recover_superblocks (list.h:204)
==4604== by 0x421C79: cmd_super_recover (cmds-rescue.c:148)
==4604== by 0x40420A: handle_command_group (btrfs.c:145)
==4604== by 0x421B54: cmd_rescue (cmds-rescue.c:162)
==4604== by 0x404199: main (btrfs.c:247)
==4604== Address 0x4c250b0 is 48 bytes inside a block of size 96 free'd
==4604== at 0x4A063F0: free (vg_replace_malloc.c:446)
==4604== by 0x43C77E: btrfs_close_devices (volumes.c:196)
==4604== by 0x42F5D1: close_ctree (disk-io.c:1404)
==4604== by 0x426A85: btrfs_recover_superblocks (super-recover.c:340)
==4604== by 0x421C79: cmd_super_recover (cmds-rescue.c:148)
==4604== by 0x40420A: handle_command_group (btrfs.c:145)
==4604== by 0x421B54: cmd_rescue (cmds-rescue.c:162)
==4604== by 0x404199: main (btrfs.c:247)
==4604==
==4604== Invalid free() / delete / delete[] / realloc()
==4604== at 0x4A063F0: free (vg_replace_malloc.c:446)
==4604== by 0x426B9E: btrfs_recover_superblocks (super-recover.c:85)
==4604== by 0x421C79: cmd_super_recover (cmds-rescue.c:148)
==4604== by 0x40420A: handle_command_group (btrfs.c:145)
==4604== by 0x421B54: cmd_rescue (cmds-rescue.c:162)
==4604== by 0x404199: main (btrfs.c:247)
==4604== Address 0x4c25080 is 0 bytes inside a block of size 96 free'd
==4604== at 0x4A063F0: free (vg_replace_malloc.c:446)
==4604== by 0x43C77E: btrfs_close_devices (volumes.c:196)
==4604== by 0x42F5D1: close_ctree (disk-io.c:1404)
==4604== by 0x426A85: btrfs_recover_superblocks (super-recover.c:340)
==4604== by 0x421C79: cmd_super_recover (cmds-rescue.c:148)
==4604== by 0x40420A: handle_command_group (btrfs.c:145)
==4604== by 0x421B54: cmd_rescue (cmds-rescue.c:162)
==4604== by 0x404199: main (btrfs.c:247)
==4604==
==4604==
==4604== HEAP SUMMARY:
==4604== in use at exit: 0 bytes in 0 blocks
==4604== total heap usage: 72 allocs, 73 frees, 140,384 bytes allocated
==4604==
==4604== All heap blocks were freed -- no leaks are possible
==4604==
==4604== For counts of detected and suppressed errors, rerun with: -v
==4604== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 6 from 6)
i.e. I think we are double freeing memory:
close_ctree(root); // <-- here
no_recover:
recover_err_str(ret);
free_recover_superblock(&recover); // <-- and here
I can't really work out what all this is all doing, but maybe the fix is obvious
to Wang Shilong (who wrote the original code)?
Thanks,
-Eric
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: "btrfs rescue super-recover" memory corruption
2014-09-17 17:00 "btrfs rescue super-recover" memory corruption Eric Sandeen
@ 2014-09-17 17:01 ` Eric Sandeen
2014-09-18 7:28 ` Wang Shilong
2014-09-18 9:01 ` [PATCH] Btrfs-progs: super-recover: fix double free fs_devices memory Wang Shilong
2 siblings, 0 replies; 6+ messages in thread
From: Eric Sandeen @ 2014-09-17 17:01 UTC (permalink / raw)
To: linux-btrfs, Shilong Wang, Chris Murphy
On 9/17/14 12:00 PM, Eric Sandeen wrote:
> This:
>
> # truncate --size=8g
oops, s/b:
# truncate --size=8g file
> # dd if=/dev/zero of=file conv=notrunc bs=4 seek=16384 count=1
> # valgrind ./btrfs rescue super-recover file -v
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: "btrfs rescue super-recover" memory corruption
2014-09-17 17:00 "btrfs rescue super-recover" memory corruption Eric Sandeen
2014-09-17 17:01 ` Eric Sandeen
@ 2014-09-18 7:28 ` Wang Shilong
2014-09-18 9:01 ` [PATCH] Btrfs-progs: super-recover: fix double free fs_devices memory Wang Shilong
2 siblings, 0 replies; 6+ messages in thread
From: Wang Shilong @ 2014-09-18 7:28 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-btrfs, Chris Murphy
Hi Eric,
> This:
>
> # truncate --size=8g
> # dd if=/dev/zero of=file conv=notrunc bs=4 seek=16384 count=1
> # valgrind ./btrfs rescue super-recover file -v
>
> yields:
>
> ==4604== Memcheck, a memory error detector
> ==4604== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
> ==4604== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
> ==4604== Command: ./btrfs rescue super-recover file -v
> ==4604==
> All Devices:
> Device: id = 1, name = file
>
> Before Recovering:
> [All good supers]:
> device name = file
> superblock bytenr = 67108864
>
> [All bad supers]:
> device name = file
> superblock bytenr = 65536
>
>
> Make sure this is a btrfs disk otherwise the tool will destroy other fs, Are you sure? [y/N]: y
> Recovered bad superblocks successful
> ==4604== Invalid read of size 8
> ==4604== at 0x426B55: btrfs_recover_superblocks (list.h:204)
> ==4604== by 0x421C79: cmd_super_recover (cmds-rescue.c:148)
> ==4604== by 0x40420A: handle_command_group (btrfs.c:145)
> ==4604== by 0x421B54: cmd_rescue (cmds-rescue.c:162)
> ==4604== by 0x404199: main (btrfs.c:247)
> ==4604== Address 0x4c250b0 is 48 bytes inside a block of size 96 free'd
> ==4604== at 0x4A063F0: free (vg_replace_malloc.c:446)
> ==4604== by 0x43C77E: btrfs_close_devices (volumes.c:196)
> ==4604== by 0x42F5D1: close_ctree (disk-io.c:1404)
> ==4604== by 0x426A85: btrfs_recover_superblocks (super-recover.c:340)
> ==4604== by 0x421C79: cmd_super_recover (cmds-rescue.c:148)
> ==4604== by 0x40420A: handle_command_group (btrfs.c:145)
> ==4604== by 0x421B54: cmd_rescue (cmds-rescue.c:162)
> ==4604== by 0x404199: main (btrfs.c:247)
> ==4604==
> ==4604== Invalid free() / delete / delete[] / realloc()
> ==4604== at 0x4A063F0: free (vg_replace_malloc.c:446)
> ==4604== by 0x426B9E: btrfs_recover_superblocks (super-recover.c:85)
> ==4604== by 0x421C79: cmd_super_recover (cmds-rescue.c:148)
> ==4604== by 0x40420A: handle_command_group (btrfs.c:145)
> ==4604== by 0x421B54: cmd_rescue (cmds-rescue.c:162)
> ==4604== by 0x404199: main (btrfs.c:247)
> ==4604== Address 0x4c25080 is 0 bytes inside a block of size 96 free'd
> ==4604== at 0x4A063F0: free (vg_replace_malloc.c:446)
> ==4604== by 0x43C77E: btrfs_close_devices (volumes.c:196)
> ==4604== by 0x42F5D1: close_ctree (disk-io.c:1404)
> ==4604== by 0x426A85: btrfs_recover_superblocks (super-recover.c:340)
> ==4604== by 0x421C79: cmd_super_recover (cmds-rescue.c:148)
> ==4604== by 0x40420A: handle_command_group (btrfs.c:145)
> ==4604== by 0x421B54: cmd_rescue (cmds-rescue.c:162)
> ==4604== by 0x404199: main (btrfs.c:247)
> ==4604==
> ==4604==
> ==4604== HEAP SUMMARY:
> ==4604== in use at exit: 0 bytes in 0 blocks
> ==4604== total heap usage: 72 allocs, 73 frees, 140,384 bytes allocated
> ==4604==
> ==4604== All heap blocks were freed -- no leaks are possible
> ==4604==
> ==4604== For counts of detected and suppressed errors, rerun with: -v
> ==4604== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 6 from 6)
>
> i.e. I think we are double freeing memory:
>
> close_ctree(root); // <-- here
> no_recover:
> recover_err_str(ret);
> free_recover_superblock(&recover); // <-- and here
>
> I can't really work out what all this is all doing, but maybe the fix is obvious
> to Wang Shilong (who wrote the original code)?
Though i no longer spend much time on btrfs, i will take a look at this. ^_^
Thanks Eric
>
> Thanks,
> -Eric
Best Regards,
Wang Shilong
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Btrfs-progs: super-recover: fix double free fs_devices memory
2014-09-17 17:00 "btrfs rescue super-recover" memory corruption Eric Sandeen
2014-09-17 17:01 ` Eric Sandeen
2014-09-18 7:28 ` Wang Shilong
@ 2014-09-18 9:01 ` Wang Shilong
2014-09-18 9:23 ` Gui Hecheng
2014-09-22 20:39 ` Eric Sandeen
2 siblings, 2 replies; 6+ messages in thread
From: Wang Shilong @ 2014-09-18 9:01 UTC (permalink / raw)
To: linux-btrfs; +Cc: Wang Shilong, Eric Sandeen, Chris Murphy
super-recover collects btrfs devices infomation using existed
functions scan_one_devices().
Problem is fs_devices is freed twice in close_ctree() and
free_recover_superblock() for super correction path.
Fix this problem by checking whether fs_devices memory
have been freed before we free it.
Cc: Eric Sandeen <sandeen@redhat.com>
Cc: Chris Murphy <lists@colorremedies.com>
Signed-off-by: Wang Shilong <wangshilong1991@gmail.com>
---
super-recover.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/super-recover.c b/super-recover.c
index 767de4b..419b86a 100644
--- a/super-recover.c
+++ b/super-recover.c
@@ -69,21 +69,11 @@ void init_recover_superblock(struct btrfs_recover_superblock *recover)
static
void free_recover_superblock(struct btrfs_recover_superblock *recover)
{
- struct btrfs_device *device;
struct super_block_record *record;
if (!recover->fs_devices)
return;
- while (!list_empty(&recover->fs_devices->devices)) {
- device = list_entry(recover->fs_devices->devices.next,
- struct btrfs_device, dev_list);
- list_del_init(&device->dev_list);
- free(device->name);
- free(device);
- }
- free(recover->fs_devices);
-
while (!list_empty(&recover->good_supers)) {
record = list_entry(recover->good_supers.next,
struct super_block_record, list);
@@ -341,6 +331,9 @@ int btrfs_recover_superblocks(const char *dname,
no_recover:
recover_err_str(ret);
free_recover_superblock(&recover);
+ /* check if we have freed fs_deivces in close_ctree() */
+ if (!root)
+ btrfs_close_devices(recover.fs_devices);
return ret;
}
--
1.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] Btrfs-progs: super-recover: fix double free fs_devices memory
2014-09-18 9:01 ` [PATCH] Btrfs-progs: super-recover: fix double free fs_devices memory Wang Shilong
@ 2014-09-18 9:23 ` Gui Hecheng
2014-09-22 20:39 ` Eric Sandeen
1 sibling, 0 replies; 6+ messages in thread
From: Gui Hecheng @ 2014-09-18 9:23 UTC (permalink / raw)
To: Wang Shilong; +Cc: linux-btrfs, Eric Sandeen, Chris Murphy
On Thu, 2014-09-18 at 05:01 -0400, Wang Shilong wrote:
> super-recover collects btrfs devices infomation using existed
> functions scan_one_devices().
>
> Problem is fs_devices is freed twice in close_ctree() and
> free_recover_superblock() for super correction path.
>
> Fix this problem by checking whether fs_devices memory
> have been freed before we free it.
>
> Cc: Eric Sandeen <sandeen@redhat.com>
> Cc: Chris Murphy <lists@colorremedies.com>
> Signed-off-by: Wang Shilong <wangshilong1991@gmail.com>
> ---
> super-recover.c | 13 +++----------
> 1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/super-recover.c b/super-recover.c
> index 767de4b..419b86a 100644
> --- a/super-recover.c
> +++ b/super-recover.c
> @@ -69,21 +69,11 @@ void init_recover_superblock(struct btrfs_recover_superblock *recover)
> static
> void free_recover_superblock(struct btrfs_recover_superblock *recover)
> {
> - struct btrfs_device *device;
> struct super_block_record *record;
>
> if (!recover->fs_devices)
> return;
>
> - while (!list_empty(&recover->fs_devices->devices)) {
> - device = list_entry(recover->fs_devices->devices.next,
> - struct btrfs_device, dev_list);
> - list_del_init(&device->dev_list);
> - free(device->name);
> - free(device);
> - }
> - free(recover->fs_devices);
> -
> while (!list_empty(&recover->good_supers)) {
> record = list_entry(recover->good_supers.next,
> struct super_block_record, list);
> @@ -341,6 +331,9 @@ int btrfs_recover_superblocks(const char *dname,
> no_recover:
> recover_err_str(ret);
> free_recover_superblock(&recover);
> + /* check if we have freed fs_deivces in close_ctree() */
> + if (!root)
> + btrfs_close_devices(recover.fs_devices);
> return ret;
> }
>
nice catch! "+20, recorded". ^_^
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Btrfs-progs: super-recover: fix double free fs_devices memory
2014-09-18 9:01 ` [PATCH] Btrfs-progs: super-recover: fix double free fs_devices memory Wang Shilong
2014-09-18 9:23 ` Gui Hecheng
@ 2014-09-22 20:39 ` Eric Sandeen
1 sibling, 0 replies; 6+ messages in thread
From: Eric Sandeen @ 2014-09-22 20:39 UTC (permalink / raw)
To: Wang Shilong, linux-btrfs; +Cc: Chris Murphy
On 9/18/14 4:01 AM, Wang Shilong wrote:
> super-recover collects btrfs devices infomation using existed
> functions scan_one_devices().
>
> Problem is fs_devices is freed twice in close_ctree() and
> free_recover_superblock() for super correction path.
>
> Fix this problem by checking whether fs_devices memory
> have been freed before we free it.
>
> Cc: Eric Sandeen <sandeen@redhat.com>
> Cc: Chris Murphy <lists@colorremedies.com>
> Signed-off-by: Wang Shilong <wangshilong1991@gmail.com>
That does seem to fix the testcase. Thanks!
Acked-by: Eric Sandeen <sandeen@redhat.com>
> ---
> super-recover.c | 13 +++----------
> 1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/super-recover.c b/super-recover.c
> index 767de4b..419b86a 100644
> --- a/super-recover.c
> +++ b/super-recover.c
> @@ -69,21 +69,11 @@ void init_recover_superblock(struct btrfs_recover_superblock *recover)
> static
> void free_recover_superblock(struct btrfs_recover_superblock *recover)
> {
> - struct btrfs_device *device;
> struct super_block_record *record;
>
> if (!recover->fs_devices)
> return;
>
> - while (!list_empty(&recover->fs_devices->devices)) {
> - device = list_entry(recover->fs_devices->devices.next,
> - struct btrfs_device, dev_list);
> - list_del_init(&device->dev_list);
> - free(device->name);
> - free(device);
> - }
> - free(recover->fs_devices);
> -
> while (!list_empty(&recover->good_supers)) {
> record = list_entry(recover->good_supers.next,
> struct super_block_record, list);
> @@ -341,6 +331,9 @@ int btrfs_recover_superblocks(const char *dname,
> no_recover:
> recover_err_str(ret);
> free_recover_superblock(&recover);
> + /* check if we have freed fs_deivces in close_ctree() */
> + if (!root)
> + btrfs_close_devices(recover.fs_devices);
> return ret;
> }
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-09-22 20:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-17 17:00 "btrfs rescue super-recover" memory corruption Eric Sandeen
2014-09-17 17:01 ` Eric Sandeen
2014-09-18 7:28 ` Wang Shilong
2014-09-18 9:01 ` [PATCH] Btrfs-progs: super-recover: fix double free fs_devices memory Wang Shilong
2014-09-18 9:23 ` Gui Hecheng
2014-09-22 20:39 ` Eric Sandeen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).