btrfs: revert fs_devices state on error of btrfs_init_new_device()
PATCH 1/1 btrfs: revert fs_devices state on error of btrfs_init_new_device()
When btrfs hits error after modifying fs_devices in btrfs_init_new_device() (such as btrfs_add_dev_item() returns error), it leaves everything as is, but frees allocated btrfs_device. As a result, fs_devices->devices and fs_devices->alloc_list contain already freed btrfs_device, leading to later use-after-free bug.
Error path also messes the things like ->num_devices. While they go backs to the original value by unscanning btrfs devices, it is safe to revert them here.
--- fs/btrfs/volumes.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-)
This patch applies on master, but not on kdave/for-next because of 74b9f4e186eb ("btrfs: declare fs_devices in btrfs_init_new_device()")
fs/btrfs/volumes.c +23 -5
@@ -2410,7 +2410,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_pathstruct list_head *devices;struct super_block *sb = fs_info->sb;struct rcu_string *name; -u64 tmp; +u64 orig_super_total_bytes, orig_super_num_devices;int seeding_dev =0 ;int ret =0 ;bool unlocked = false;
@@ -2509,12 +2509,14 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_pathif (!blk_queue_nonrot (q)) fs_info->fs_devices->rotating =1 ; - tmp =btrfs_super_total_bytes (fs_info->super_copy); + orig_super_total_bytes =btrfs_super_total_bytes (fs_info->super_copy);btrfs_set_super_total_bytes (fs_info->super_copy, -round_down (tmp + device->total_bytes, fs_info->sectorsize)); +round_down (orig_super_total_bytes + device->total_bytes, + fs_info->sectorsize)); - tmp =btrfs_super_num_devices (fs_info->super_copy); -btrfs_set_super_num_devices (fs_info->super_copy, tmp +1 ); + orig_super_num_devices =btrfs_super_num_devices (fs_info->super_copy); +btrfs_set_super_num_devices (fs_info->super_copy, + orig_super_num_devices +1 );/* add sysfs device entry */ btrfs_sysfs_add_device_link (fs_info->fs_devices, device);
@@ -2594,6 +2596,22 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path error_sysfs:btrfs_sysfs_rm_device_link (fs_info->fs_devices, device); +mutex_lock (&fs_info->fs_devices->device_list_mutex); +mutex_lock (&fs_info->chunk_mutex); +list_del_rcu (&device->dev_list); +list_del (&device->dev_alloc_list); + fs_info->fs_devices->num_devices--; + fs_info->fs_devices->open_devices--; + fs_info->fs_devices->rw_devices--; + fs_info->fs_devices->total_devices--; + fs_info->fs_devices->total_rw_bytes -= device->total_bytes; +atomic64_sub (device->total_bytes, &fs_info->free_chunk_space); +btrfs_set_super_total_bytes (fs_info->super_copy, + orig_super_total_bytes); +btrfs_set_super_num_devices (fs_info->super_copy, + orig_super_num_devices); +mutex_unlock (&fs_info->chunk_mutex); +mutex_unlock (&fs_info->fs_devices->device_list_mutex); error_trans:if (seeding_dev) sb->s_flags |=SB_RDONLY ;
Signature · 1 line
2.18.0
Quoted 6 lines · Naohiro Aota
When btrfs hits error after modifying
fs_devicesinbtrfs_init_new_device()(such asbtrfs_add_dev_item()returns error), it leaves everything as is, but frees allocatedbtrfs_device. As a result,fs_devices->devicesandfs_devices->alloc_listcontain already freedbtrfs_device, leading to later use-after-free bug.
Quoted 6 lines
Error path also messes the things like ->num_devices. While they go backs to the original value by unscanning btrfs devices, it is safe to revert them here.
Fixes 79787eaab461 ("btrfs: replace many BUG_ONs with proper error handling")Signed-off-by Naohiro Aota <[email protected]>
Looks good, only fs_info->fs_devices->rotating isn't restored but currently that causes no problems.
Quoted 69 lines
--- fs/btrfs/volumes.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-)This patch applies on master, but not on kdave/for-next because of
74b9f4e186eb("btrfs: declarefs_devicesinbtrfs_init_new_device()")fs/btrfs/volumes.c +23
-6@@ -2410,7 +2410,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_pathstruct list_head *devices;struct super_block *sb = fs_info->sb;struct rcu_string *name; -u64 tmp; +u64 orig_super_total_bytes, orig_super_num_devices;int seeding_dev =0 ;int ret =0 ;bool unlocked = false;@@ -2509,12 +2509,14 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_pathif (!blk_queue_nonrot (q)) fs_info->fs_devices->rotating =1 ; - tmp =btrfs_super_total_bytes (fs_info->super_copy); + orig_super_total_bytes =btrfs_super_total_bytes (fs_info->super_copy);btrfs_set_super_total_bytes (fs_info->super_copy, -round_down (tmp + device->total_bytes, fs_info->sectorsize)); +round_down (orig_super_total_bytes + device->total_bytes, + fs_info->sectorsize)); - tmp =btrfs_super_num_devices (fs_info->super_copy); -btrfs_set_super_num_devices (fs_info->super_copy, tmp +1 ); + orig_super_num_devices =btrfs_super_num_devices (fs_info->super_copy); +btrfs_set_super_num_devices (fs_info->super_copy, + orig_super_num_devices +1 );/* add sysfs device entry */ btrfs_sysfs_add_device_link (fs_info->fs_devices, device);@@ -2594,6 +2596,22 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path error_sysfs:btrfs_sysfs_rm_device_link (fs_info->fs_devices, device); +mutex_lock (&fs_info->fs_devices->device_list_mutex); +mutex_lock (&fs_info->chunk_mutex); +list_del_rcu (&device->dev_list); +list_del (&device->dev_alloc_list); + fs_info->fs_devices->num_devices--; + fs_info->fs_devices->open_devices--; + fs_info->fs_devices->rw_devices--; + fs_info->fs_devices->total_devices--; + fs_info->fs_devices->total_rw_bytes -= device->total_bytes; +atomic64_sub (device->total_bytes, &fs_info->free_chunk_space); +btrfs_set_super_total_bytes (fs_info->super_copy, + orig_super_total_bytes); +btrfs_set_super_num_devices (fs_info->super_copy, + orig_super_num_devices); +mutex_unlock (&fs_info->chunk_mutex); +mutex_unlock (&fs_info->fs_devices->device_list_mutex); error_trans:if (seeding_dev) sb->s_flags |=SB_RDONLY ; --2.18.0
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 3 lines
Filipe David Manana, “Whether you think you can, or you think you can't — you're right.”
Quoted 5 lines · Naohiro Aota
When btrfs hits error after modifying
fs_devicesinbtrfs_init_new_device()(such asbtrfs_add_dev_item()returns error), it leaves everything as is, but frees allocatedbtrfs_device. As a result,fs_devices->devicesandfs_devices->alloc_listcontain already freedbtrfs_device, leading to later use-after-free bug.
the undo part of the btrfs_init_new_device() is broken for a while now. Thanks for the fix, but..
- this patch does not fix the seed device context, its ok to fix that in a separate patch though.
- and does not undo the effect of
-----
if (!blk_queue_nonrot(q))
fs_info->fs_devices->rotating = 1
::
btrfs_clear_space_info_full(fs_info);----
which I think should be handled as part of this patch.
Thanks, Anand
Quoted 69 lines
Error path also messes the things like ->num_devices. While they go backs to the original value by unscanning btrfs devices, it is safe to revert them here.
Fixes 79787eaab461 ("btrfs: replace many BUG_ONs with proper error handling")Signed-off-by Naohiro Aota <[email protected]>--- fs/btrfs/volumes.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-)This patch applies on master, but not on kdave/for-next because of
74b9f4e186eb("btrfs: declarefs_devicesinbtrfs_init_new_device()")fs/btrfs/volumes.c +23
-5@@ -2410,7 +2410,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_pathstruct list_head *devices;struct super_block *sb = fs_info->sb;struct rcu_string *name; -u64 tmp; +u64 orig_super_total_bytes, orig_super_num_devices;int seeding_dev =0 ;int ret =0 ;bool unlocked = false;@@ -2509,12 +2509,14 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_pathif (!blk_queue_nonrot (q)) fs_info->fs_devices->rotating =1 ; - tmp =btrfs_super_total_bytes (fs_info->super_copy); + orig_super_total_bytes =btrfs_super_total_bytes (fs_info->super_copy);btrfs_set_super_total_bytes (fs_info->super_copy, -round_down (tmp + device->total_bytes, fs_info->sectorsize)); +round_down (orig_super_total_bytes + device->total_bytes, + fs_info->sectorsize)); - tmp =btrfs_super_num_devices (fs_info->super_copy); -btrfs_set_super_num_devices (fs_info->super_copy, tmp +1 ); + orig_super_num_devices =btrfs_super_num_devices (fs_info->super_copy); +btrfs_set_super_num_devices (fs_info->super_copy, + orig_super_num_devices +1 );/* add sysfs device entry */ btrfs_sysfs_add_device_link (fs_info->fs_devices, device);@@ -2594,6 +2596,22 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path error_sysfs:btrfs_sysfs_rm_device_link (fs_info->fs_devices, device); +mutex_lock (&fs_info->fs_devices->device_list_mutex); +mutex_lock (&fs_info->chunk_mutex); +list_del_rcu (&device->dev_list); +list_del (&device->dev_alloc_list); + fs_info->fs_devices->num_devices--; + fs_info->fs_devices->open_devices--; + fs_info->fs_devices->rw_devices--; + fs_info->fs_devices->total_devices--; + fs_info->fs_devices->total_rw_bytes -= device->total_bytes; +atomic64_sub (device->total_bytes, &fs_info->free_chunk_space); +btrfs_set_super_total_bytes (fs_info->super_copy, + orig_super_total_bytes); +btrfs_set_super_num_devices (fs_info->super_copy, + orig_super_num_devices); +mutex_unlock (&fs_info->chunk_mutex); +mutex_unlock (&fs_info->fs_devices->device_list_mutex); error_trans:if (seeding_dev) sb->s_flags |=SB_RDONLY ;
Quoted 18 lines · Anand Jain
Quoted 6 lines · Naohiro Aota
When btrfs hits error after modifying
fs_devicesinbtrfs_init_new_device()(such asbtrfs_add_dev_item()returns error), it leaves everything as is, but frees allocatedbtrfs_device. As a result,fs_devices->devicesandfs_devices->alloc_listcontain already freedbtrfs_device, leading to later use-after-free bug.the undo part of the
btrfs_init_new_device()is broken for a while now. Thanks for the fix, but..- this patch does not fix the seed device context, its ok to fix that in a separate patch though.
- and does not undo the effect of
Quoted 7 lines
-----
if (!blk_queue_nonrot(q)) fs_info->fs_devices->rotating = 1 :: btrfs_clear_space_info_full(fs_info);----
which I think should be handled as part of this patch.
Doesn't matter, the filesystem was turned to RO mode (transaction aborted).
Quoted 81 lines
Thanks, Anand
Quoted 62 lines
Error path also messes the things like ->num_devices. While they go backs to the original value by unscanning btrfs devices, it is safe to revert them here.
Fixes 79787eaab461 ("btrfs: replace many BUG_ONs with proper errorhandling")
Signed-off-by Naohiro Aota <[email protected]>--- fs/btrfs/volumes.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-)This patch applies on master, but not on kdave/for-next because of
74b9f4e186eb("btrfs: declarefs_devicesinbtrfs_init_new_device()")fs/btrfs/volumes.c +1
-1@@ -2410,7 +2410,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_pathstruct list_head *devices;struct super_block *sb = fs_info->sb;struct rcu_string *name; -u64 tmp; +u64 orig_super_total_bytes, orig_super_num_devices;int seeding_dev =0 ;int ret =0 ;bool unlocked = false;fs/btrfs/volumes.c +15
-2@@ -2509,12 +2509,14 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_pathif (!blk_queue_nonrot (q)) fs_info->fs_devices->rotating =1 ; - tmp =btrfs_super_total_bytes (fs_info->super_copy); + orig_super_total_bytes =btrfs_super_total_bytes (fs_info->super_copy);btrfs_set_super_total_bytes (fs_info->super_copy, -round_down (tmp + device->total_bytes, fs_info->sectorsize)); +round_down (orig_super_total_bytes + device->total_bytes, + fs_info->sectorsize)); - tmp =btrfs_super_num_devices (fs_info->super_copy); -btrfs_set_super_num_devices (fs_info->super_copy, tmp +1 ); + orig_super_num_devices =btrfs_super_num_devices (fs_info->super_copy); +btrfs_set_super_num_devices (fs_info->super_copy, + orig_super_num_devices +1 );/* add sysfs device entry */ btrfs_sysfs_add_device_link (fs_info->fs_devices, device);@@ -2594,6 +2596,22 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path error_sysfs:btrfs_sysfs_rm_device_link (fs_info->fs_devices, device); +mutex_lock (&fs_info->fs_devices->device_list_mutex); +mutex_lock (&fs_info->chunk_mutex); +list_del_rcu (&device->dev_list); +list_del (&device->dev_alloc_list); + fs_info->fs_devices->num_devices--; + fs_info->fs_devices->open_devices--; + fs_info->fs_devices->rw_devices--; + fs_info->fs_devices->total_devices--; + fs_info->fs_devices->total_rw_bytes -= device->total_bytes;fs/btrfs/volumes.c int btrfs_init_new_device(struct btrfs_fs_info · line 2617
Quoted 10 lines
fs/btrfs/volumes.c +7
-0+atomic64_sub (device->total_bytes, &fs_info->free_chunk_space); +btrfs_set_super_total_bytes (fs_info->super_copy, + orig_super_total_bytes); +btrfs_set_super_num_devices (fs_info->super_copy, + orig_super_num_devices); +mutex_unlock (&fs_info->chunk_mutex); +mutex_unlock (&fs_info->fs_devices->device_list_mutex); error_trans:if (seeding_dev) sb->s_flags |=SB_RDONLY ;--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 3 lines
Filipe David Manana, “Whether you think you can, or you think you can't — you're right.”
Quoted 27 lines · Filipe Manana
Quoted 18 lines · Anand Jain
Quoted 6 lines · Naohiro Aota
When btrfs hits error after modifying
fs_devicesinbtrfs_init_new_device()(such asbtrfs_add_dev_item()returns error), it leaves everything as is, but frees allocatedbtrfs_device. As a result,fs_devices->devicesandfs_devices->alloc_listcontain already freedbtrfs_device, leading to later use-after-free bug.the undo part of the
btrfs_init_new_device()is broken for a while now. Thanks for the fix, but..- this patch does not fix the seed device context, its ok to fix that in a separate patch though.
- and does not undo the effect ofQuoted 7 lines
-----
if (!blk_queue_nonrot(q)) fs_info->fs_devices->rotating = 1 :: btrfs_clear_space_info_full(fs_info);----
which I think should be handled as part of this patch.
Quoted 1 line
Doesn't matter, the filesystem was turned to RO mode (transaction aborted).
. That's not true in all cases. Filesystem can still be in the RW
mode after the transaction aborted. Tested with the following simulation.
--------------
fs/btrfs/volumes.c +4 -0
@@ -2458,6 +2458,10 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path } } + ret = -ENOMEM ; +btrfs_abort_transaction (trans, ret); + goto error_sysfs; + ret =btrfs_add_dev_item (trans, device);if (ret) {btrfs_abort_transaction (trans, ret);
-------------------
# mount /dev/sdb /btrfs
# btrfs dev add /dev/sdc /btrfs ERROR: error adding device '/dev/sdc': Cannot allocate memory
# cat /proc/self/mounts | grep btrfs /dev/sdb /btrfs btrfs rw,relatime,space_cache,subvolid=5,subvol=/ 0 0
# echo "test" > /btrfs/tf; echo $? 0
. In any case, I would rather put the things right even if it just
theoretical. A core dump taken after this would indicate a wrong state of the space and fs_devices::rotating.
Thanks, Anand
Quoted 84 lines
Quoted 81 lines
Thanks, Anand
Quoted 62 lines
Error path also messes the things like ->num_devices. While they go backs to the original value by unscanning btrfs devices, it is safe to revert them here.
Fixes 79787eaab461 ("btrfs: replace many BUG_ONs with proper errorhandling")
Signed-off-by Naohiro Aota <[email protected]>--- fs/btrfs/volumes.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-)This patch applies on master, but not on kdave/for-next because of
74b9f4e186eb("btrfs: declarefs_devicesinbtrfs_init_new_device()")fs/btrfs/volumes.c +1
-1@@ -2410,7 +2410,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_pathstruct list_head *devices;struct super_block *sb = fs_info->sb;struct rcu_string *name; -u64 tmp; +u64 orig_super_total_bytes, orig_super_num_devices;int seeding_dev =0 ;int ret =0 ;bool unlocked = false;fs/btrfs/volumes.c +15
-2@@ -2509,12 +2509,14 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_pathif (!blk_queue_nonrot (q)) fs_info->fs_devices->rotating =1 ; - tmp =btrfs_super_total_bytes (fs_info->super_copy); + orig_super_total_bytes =btrfs_super_total_bytes (fs_info->super_copy);btrfs_set_super_total_bytes (fs_info->super_copy, -round_down (tmp + device->total_bytes, fs_info->sectorsize)); +round_down (orig_super_total_bytes + device->total_bytes, + fs_info->sectorsize)); - tmp =btrfs_super_num_devices (fs_info->super_copy); -btrfs_set_super_num_devices (fs_info->super_copy, tmp +1 ); + orig_super_num_devices =btrfs_super_num_devices (fs_info->super_copy); +btrfs_set_super_num_devices (fs_info->super_copy, + orig_super_num_devices +1 );/* add sysfs device entry */ btrfs_sysfs_add_device_link (fs_info->fs_devices, device);@@ -2594,6 +2596,22 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path error_sysfs:btrfs_sysfs_rm_device_link (fs_info->fs_devices, device); +mutex_lock (&fs_info->fs_devices->device_list_mutex); +mutex_lock (&fs_info->chunk_mutex); +list_del_rcu (&device->dev_list); +list_del (&device->dev_alloc_list); + fs_info->fs_devices->num_devices--; + fs_info->fs_devices->open_devices--; + fs_info->fs_devices->rw_devices--; + fs_info->fs_devices->total_devices--; + fs_info->fs_devices->total_rw_bytes -= device->total_bytes;fs/btrfs/volumes.c int btrfs_init_new_device(struct btrfs_fs_info · line 2617
Quoted 10 lines
fs/btrfs/volumes.c +7
-0+atomic64_sub (device->total_bytes, &fs_info->free_chunk_space); +btrfs_set_super_total_bytes (fs_info->super_copy, + orig_super_total_bytes); +btrfs_set_super_num_devices (fs_info->super_copy, + orig_super_num_devices); +mutex_unlock (&fs_info->chunk_mutex); +mutex_unlock (&fs_info->fs_devices->device_list_mutex); error_trans:if (seeding_dev) sb->s_flags |=SB_RDONLY ;--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 31 lines · Anand Jain
Quoted 27 lines · Filipe Manana
Quoted 25 lines · Anand Jain
Quoted 6 lines · Naohiro Aota
When btrfs hits error after modifying
fs_devicesinbtrfs_init_new_device()(such asbtrfs_add_dev_item()returns error), it leaves everything as is, but frees allocatedbtrfs_device. As a result,fs_devices->devicesandfs_devices->alloc_listcontain already freedbtrfs_device, leading to later use-after-free bug.the undo part of the
btrfs_init_new_device()is broken for a while now. Thanks for the fix, but..- this patch does not fix the seed device context, its ok to fix that in a separate patch though.
- and does not undo the effect of-----
if (!blk_queue_nonrot(q))fs_info->fs_devices->rotating= 1 ::
btrfs_clear_space_info_full(fs_info);
----
which I think should be handled as part of this patch.
Quoted 3 lines
Quoted 1 line
Doesn't matter, the filesystem was turned to RO mode (transaction aborted).
. That's not true in all cases. Filesystem can still be in the RW
typo I mean not true in some cases and FS can still be RW able after the transaction abort, below is a test case and results.
Thanks. Aannd
Quoted 130 lines
mode after the transaction aborted. Tested with the following simulation.
--------------
fs/btrfs/volumes.c +4
-0@@ -2458,6 +2458,10 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path } } + ret = -ENOMEM; + btrfs_abort_transaction(trans, ret); + goto error_sysfs; + ret = btrfs_add_dev_item(trans, device); if (ret) {
btrfs_abort_transaction(trans, ret);
-------------------# mount /dev/sdb /btrfs# btrfs dev add /dev/sdc /btrfs ERROR: error adding device '/dev/sdc': Cannot allocate memory# cat /proc/self/mounts | grep btrfs /dev/sdb /btrfs btrfs rw,relatime,space_cache,subvolid=5,subvol=/ 0 0# echo "test" > /btrfs/tf; echo $? 0. In any case, I would rather put the things right even if it just
theoretical. A core dump taken after this would indicate a wrong state of the space andfs_devices::rotating.Thanks, Anand
Quoted 85 lines
Quoted 82 lines
Thanks, Anand
Quoted 73 lines
Error path also messes the things like ->num_devices. While they go backs to the original value by unscanning btrfs devices, it is safe to revert them here.
Fixes 79787eaab461 ("btrfs: replace many BUG_ONs with proper errorhandling")
Signed-off-by Naohiro Aota <[email protected]>---
fs/btrfs/volumes.c| 28 +++++++++++++++++++++++-----1 file changed, 23 insertions(+), 5 deletions(-)This patch applies on master, but not on kdave/for-next because of
74b9f4e186eb("btrfs: declarefs_devicesinbtrfs_init_new_device()")fs/btrfs/volumes.c +1
-1@@ -2410,7 +2410,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path struct list_head *devices; struct super_block *sb = fs_info->sb; struct rcu_string *name; - u64 tmp; + u64 orig_super_total_bytes, orig_super_num_devices; int seeding_dev = 0; int ret = 0;bool unlocked = false;
fs/btrfs/volumes.c +6
-2@@ -2509,12 +2509,14 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path if (!blk_queue_nonrot(q)) fs_info->fs_devices->rotating = 1; - tmp = btrfs_super_total_bytes(fs_info->super_copy); + orig_super_total_bytes =btrfs_super_total_bytes (fs_info->super_copy); btrfs_set_super_total_bytes(fs_info->super_copy, - round_down(tmp + device->total_bytes, fs_info->sectorsize)); + round_down(orig_super_total_bytes + device->total_bytes, + fs_info->sectorsize)); - tmp = btrfs_super_num_devices(fs_info->super_copy); - btrfs_set_super_num_devices(fs_info->super_copy, tmp + 1); + orig_super_num_devices =btrfs_super_num_devices (fs_info->super_copy); + btrfs_set_super_num_devices(fs_info->super_copy, + orig_super_num_devices + 1);/* add sysfs device entry */
btrfs_sysfs_add_device_link(fs_info->fs_devices, device);fs/btrfs/volumes.c +0
-0@@ -2594,6 +2596,22 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
error_sysfs:
btrfs_sysfs_rm_device_link(fs_info->fs_devices, device); +mutex_lock(&fs_info->fs_devices->device_list_mutex); +mutex_lock(&fs_info->chunk_mutex);
+list_del_rcu(&device->dev_list);
+list_del(&device->dev_alloc_list);
+ fs_info->fs_devices->num_devices--;
+ fs_info->fs_devices->open_devices--;
+ fs_info->fs_devices->rw_devices--;
+ fs_info->fs_devices->total_devices--;
+fs_info->fs_devices->total_rw_bytes-=device->total_bytes; +atomic64_sub(device->total_bytes, &fs_info->free_chunk_space); + btrfs_set_super_total_bytes(fs_info->super_copy,
+orig_super_total_bytes); + btrfs_set_super_num_devices(fs_info->super_copy,
+orig_super_num_devices); +mutex_unlock(&fs_info->chunk_mutex);
+mutex_unlock(&fs_info->fs_devices->device_list_mutex);error_trans:
if (seeding_dev)
sb->s_flags|=SB_RDONLY;Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.htmlSignature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 43 lines · Anand Jain
Quoted 40 lines · Anand Jain
Quoted 35 lines · Filipe Manana
Quoted 30 lines · Anand Jain
Quoted 8 lines · Naohiro Aota
When btrfs hits error after modifying
fs_devicesinbtrfs_init_new_device()(such asbtrfs_add_dev_item()returns error), it
leaves everything as is, but frees allocatedbtrfs_device. As a result,fs_devices->devicesandfs_devices->alloc_listcontain already freedbtrfs_device, leading to later use-after-free bug.the undo part of the
btrfs_init_new_device()is broken for a while
now.
Thanks for the fix, but..- this patch does not fix the seed device context, its ok to fix that in a separate patch though.
- and does not undo the effect of-----
if (!blk_queue_nonrot(q)) fs_info->fs_devices->rotating = 1 :: btrfs_clear_space_info_full(fs_info);----
which I think should be handled as part of this patch.
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
Doesn't matter, the filesystem was turned to RO mode (transaction aborted).
. That's not true in all cases. Filesystem can still be in the RW
Yes, if nothing was done yet in the transaction (exactly what happens in your test), in which case there's no risk of leaving inconsistent metadata on disk.
Space info being full is rather rare, further setting it to full only makes the next allocation attempt to do some work looking for space instead of returning enospc immediately.
Settting the rotating flag has currently no effect for a mounted filesystem.
That is, there are no problems.
Quoted 142 lines
typo I mean not true in some cases and FS can still be RW able after the transaction abort, below is a test case and results.
Thanks. Aannd
Quoted 134 lines
mode after the transaction aborted. Tested with the following simulation.
--------------
fs/btrfs/volumes.c +4
-0@@ -2458,6 +2458,10 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path } } + ret = -ENOMEM ; +btrfs_abort_transaction (trans, ret); + goto error_sysfs; + ret =btrfs_add_dev_item (trans, device);if (ret) {btrfs_abort_transaction(trans, ret);-------------------
# mount /dev/sdb /btrfs# btrfs dev add /dev/sdc /btrfs ERROR: error adding device '/dev/sdc': Cannot allocate memory# cat /proc/self/mounts | grep btrfs /dev/sdb /btrfs btrfs rw,relatime,space_cache,subvolid=5,subvol=/ 0 0# echo "test" > /btrfs/tf; echo $? 0. In any case, I would rather put the things right even if it just
theoretical. A core dump taken after this would indicate a wrong state of the space andfs_devices::rotating.Thanks, Anand
Quoted 77 lines
Quoted 77 lines
Thanks, Anand
Quoted 72 lines
Error path also messes the things like ->num_devices. While they go backs
to the original value by unscanning btrfs devices, it is safe to revert them here.Fixes 79787eaab461 ("btrfs: replace many BUG_ONs with proper errorhandling")
Signed-off-by Naohiro Aota <[email protected]>--- fs/btrfs/volumes.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-)This patch applies on master, but not on kdave/for-next because of
74b9f4e186eb("btrfs: declarefs_devicesinbtrfs_init_new_device()")fs/btrfs/volumes.c +1
-1@@ -2410,7 +2410,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_pathstruct list_head *devices;struct super_block *sb = fs_info->sb;struct rcu_string *name; -u64 tmp; +u64 orig_super_total_bytes, orig_super_num_devices;int seeding_dev =0 ;int ret =0 ;bool unlocked = false;fs/btrfs/volumes.c +22
-2@@ -2509,12 +2509,14 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_pathif (!blk_queue_nonrot (q)) fs_info->fs_devices->rotating =1 ; - tmp =btrfs_super_total_bytes (fs_info->super_copy); + orig_super_total_bytes =btrfs_super_total_bytes (fs_info->super_copy);btrfs_set_super_total_bytes (fs_info->super_copy, -round_down (tmp + device->total_bytes, fs_info->sectorsize)); +round_down (orig_super_total_bytes + device->total_bytes, + fs_info->sectorsize)); - tmp =btrfs_super_num_devices (fs_info->super_copy); -btrfs_set_super_num_devices (fs_info->super_copy, tmp +1 ); + orig_super_num_devices =btrfs_super_num_devices (fs_info->super_copy); +btrfs_set_super_num_devices (fs_info->super_copy, + orig_super_num_devices +1 );/* add sysfs device entry */ btrfs_sysfs_add_device_link (fs_info->fs_devices, device);@@ -2594,6 +2596,22 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path error_sysfs:btrfs_sysfs_rm_device_link (fs_info->fs_devices, device); +mutex_lock (&fs_info->fs_devices->device_list_mutex); +mutex_lock (&fs_info->chunk_mutex); +list_del_rcu (&device->dev_list); +list_del (&device->dev_alloc_list); + fs_info->fs_devices->num_devices--; + fs_info->fs_devices->open_devices--; + fs_info->fs_devices->rw_devices--; + fs_info->fs_devices->total_devices--; + fs_info->fs_devices->total_rw_bytes -= device->total_bytes; +atomic64_sub (device->total_bytes, &fs_info->free_chunk_space); +btrfs_set_super_total_bytes (fs_info->super_copy, + orig_super_total_bytes); +btrfs_set_super_num_devices (fs_info->super_copy, + orig_super_num_devices); +mutex_unlock (&fs_info->chunk_mutex); +mutex_unlock (&fs_info->fs_devices->device_list_mutex); error_trans:Quoted 12 lines
Quoted 8 lines
fs/btrfs/volumes.c int btrfs_init_new_device(struct btrfs_fs_info · line 2617
Quoted 3 lines
fs/btrfs/volumes.c +0
-0if (seeding_dev) sb->s_flags |=SB_RDONLY ;--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 3 lines
Filipe David Manana, “Whether you think you can, or you think you can't — you're right.”
Quoted 6 lines · Naohiro Aota
When btrfs hits error after modifying
fs_devicesinbtrfs_init_new_device()(such asbtrfs_add_dev_item()returns error), it leaves everything as is, but frees allocatedbtrfs_device. As a result,fs_devices->devicesandfs_devices->alloc_listcontain already freedbtrfs_device, leading to later use-after-free bug.
Quoted 6 lines
Error path also messes the things like ->num_devices. While they go backs to the original value by unscanning btrfs devices, it is safe to revert them here.
Fixes 79787eaab461 ("btrfs: replace many BUG_ONs with proper error handling")Signed-off-by Naohiro Aota <[email protected]>
Added to misc-next, thanks.