Hi Adam,
(2014/06/14 8:43), Adam Buchbinder wrote:
> When a struct btrfs_fs_devices was being torn down by
> btrfs_close_devices(), there was an invalidated pointer in the global
> list fs_uuids which still pointed to it; if a device was closed and
> then reopened (which btrfs-convert does), freed memory would be
> accessed.
It looks good to me.
Reviewed-by: Satoru Takeuchi <takeuchi_satoru@xxxxxxxxxxxxxx>
I confirmed ...
- btrfs_fs_devices is linked to fs_uuids in device_list_add().
- However, there is no list_del(btrfs_fs_devices) from
fs_uuids at all.
- There is at least one use-after-free of btrfs_fs_devices
as you said (btrfs-convert).
P.S.
I suggest you to add "btrfs-progs: " or "btrfs:" tag to mail subject
from the next time.
Thanks,
Satoru
>
> This was found using ThreadSanitizer (pretty much doing what
> AddressSanitizer would, but not exiting after the first failure).
> To reproduce, build with -fsanitize=thread and run 'make test'.
> Representative output is below.
>
> This change makes the current tests TSan-clean.
>
> WARNING: ThreadSanitizer: heap-use-after-free (pid=29161)
> Read of size 8 at 0x7d180000eee0 by main thread:
> #0 memcmp ??:0
> #1 find_fsid .../volumes.c:81
> #2 device_list_add .../volumes.c:95
> #3 btrfs_scan_one_device .../volumes.c:259
> #4 btrfs_scan_fs_devices .../disk-io.c:1002
> #5 __open_ctree_fd .../disk-io.c:1090
> #6 open_ctree_fd .../disk-io.c:1191
> #7 do_convert .../btrfs-convert.c:2317
> #8 main .../btrfs-convert.c:2745
>
> Previous write of size 8 at 0x7d180000eee0 by main thread:
> #0 free ??:0
> #1 btrfs_close_devices .../volumes.c:191
> #2 close_ctree .../disk-io.c:1401
> #3 do_convert .../btrfs-convert.c:2300
> #4 main .../btrfs-convert.c:2745
>
> Location is heap block of size 96 at 0x7d180000eee0 allocated by main thread:
> #0 calloc ??:0 (exe+0x00000002acc6)
> #1 device_list_add .../volumes.c:97
> #2 btrfs_scan_one_device .../volumes.c:259
> #3 btrfs_scan_fs_devices .../disk-io.c:1002
> #4 __open_ctree_fd .../disk-io.c:1090
> #5 open_ctree_fd .../disk-io.c:1191
> #6 do_convert .../btrfs-convert.c:2256
> #7 main .../btrfs-convert.c:2745
>
> Signed-off-by: Adam Buchbinder <abuchbinder@xxxxxxxxxx>
> ---
> volumes.c | 1 +
> volumes.h | 2 +-
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/volumes.c b/volumes.c
> index a61928c..36f6050 100644
> --- a/volumes.c
> +++ b/volumes.c
> @@ -188,6 +188,7 @@ again:
> goto again;
> }
>
> + list_del(&fs_devices->list);
> free(fs_devices);
> return 0;
> }
> diff --git a/volumes.h b/volumes.h
> index b1ff3d0..2e960b5 100644
> --- a/volumes.h
> +++ b/volumes.h
> @@ -67,7 +67,7 @@ struct btrfs_device {
> struct btrfs_fs_devices {
> u8 fsid[BTRFS_FSID_SIZE]; /* FS specific uuid */
>
> - /* the device with this id has the most recent coyp of the super */
> + /* the device with this id has the most recent copy of the super */
> u64 latest_devid;
> u64 latest_trans;
> u64 lowest_devid;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
