btrfs: allocate raid type kobjects dynamically

PATCH Superseded Jeff Mahoney 1 participant 3 messages May 23, 2014 Btrfs file system

Share & tools

Jeff Mahoney

author

2014-05-23 19:04 UTC #0 lore raw

PATCH 1/1 btrfs: allocate raid type kobjects dynamically

We are currently allocating space_info objects in an array when we allocate space_info. When a user does something like:

# btrfs balance start -mconvert=raid1 -dconvert=raid1 /mnt
# btrfs balance start -mconvert=single -dconvert=single /mnt -f
# btrfs balance start -mconvert=raid1 -dconvert=raid1 /

We can end up with memory corruption since the kobject hasn't been reinitialized properly and the name pointer was left set.

The rationale behind allocating them statically was to avoid creating a separate kobject container that just contained the raid type. It used the index in the array to determine the index.

Ultimately, though, this wastes more memory than it saves in all but the most complex scenarios and introduces kobject lifetime questions.

This patch allocates the kobjects dynamically instead. Note that we also remove the kobject_get/put of the parent kobject since kobject_add and kobject_del do that internally.

Signed-off-by Jeff Mahoney <[email protected]>

---
 fs/btrfs/ctree.h       |    8 +++++++-
 fs/btrfs/extent-tree.c |   33 +++++++++++++++++++++------------
 fs/btrfs/sysfs.c       |    5 +++--
 3 files changed, 31 insertions(+), 15 deletions(-)

fs/btrfs/ctree.h +7 -1

@@ -1113,6 +1113,12 @@ struct btrfs_qgroup_limit_item {
 	__le64 rsv_excl;
 } __attribute__ ((__packed__));
 
+/* For raid type sysfs entries */
+struct raid_kobject {
+	int raid_type;
+	struct kobject kobj;
+};
+
 struct btrfs_space_info {
 	spinlock_t lock;

@@ -1163,7 +1169,7 @@ struct btrfs_space_info {
 	wait_queue_head_t wait;
 
 	struct kobject kobj;
-	struct kobject block_group_kobjs[BTRFS_NR_RAID_TYPES];
+	struct kobject *block_group_kobjs[BTRFS_NR_RAID_TYPES];
 };
 
 #define	BTRFS_BLOCK_RSV_GLOBAL		1

fs/btrfs/extent-tree.c +21 ~~-12~~

@@ -3401,10 +3401,8 @@ static int update_space_info(struct btrf
 		return ret;
 	}
 
-	for (i = 0; i < BTRFS_NR_RAID_TYPES; i++) {
+	for (i = 0; i < BTRFS_NR_RAID_TYPES; i++)
 		INIT_LIST_HEAD(&found->block_groups[i]);
-		kobject_init(&found->block_group_kobjs[i], &btrfs_raid_ktype);
-	}
 	init_rwsem(&found->groups_sem);
 	spin_lock_init(&found->lock);
 	found->flags = flags & BTRFS_BLOCK_GROUP_TYPE_MASK;

@@ -8327,7 +8325,8 @@ int btrfs_free_block_groups(struct btrfs
 		list_del(&space_info->list);
 		for (i = 0; i < BTRFS_NR_RAID_TYPES; i++) {
 			struct kobject *kobj;
-			kobj = &space_info->block_group_kobjs[i];
+			kobj = space_info->block_group_kobjs[i];
+			space_info->block_group_kobjs[i] = NULL;
 			if (kobj->parent) {
 				kobject_del(kobj);
 				kobject_put(kobj);

@@ -8352,17 +8351,26 @@ static void __link_block_group(struct bt
 	up_write(&space_info->groups_sem);
 
 	if (first) {
-		struct kobject *kobj = &space_info->block_group_kobjs[index];
+		struct raid_kobject *rkobj;
 		int ret;
 
-		kobject_get(&space_info->kobj); /* put in release */
-		ret = kobject_add(kobj, &space_info->kobj, "%s",
-				  get_raid_name(index));
+		rkobj = kzalloc(sizeof(*rkobj), GFP_KERNEL);
+		if (!rkobj)
+			goto out_err;
+		rkobj->raid_type = index;
+		kobject_init(&rkobj->kobj, &btrfs_raid_ktype);
+		ret = kobject_add(&rkobj->kobj, &space_info->kobj,
+				  "%s", get_raid_name(index));
 		if (ret) {
-			pr_warn("BTRFS: failed to add kobject for block cache. ignoring.\n");
-			kobject_put(&space_info->kobj);
+			kobject_put(&rkobj->kobj);
+			goto out_err;
 		}
+		space_info->block_group_kobjs[index] = &rkobj->kobj;
 	}
+
+	return;
+out_err:
+	pr_warn("BTRFS: failed to add kobject for block cache. ignoring.\n");
 }
 
 static struct btrfs_block_group_cache *

@@ -8796,8 +8804,9 @@ int btrfs_remove_block_group(struct btrf
 	 */
 	list_del_init(&block_group->list);
 	if (list_empty(&block_group->space_info->block_groups[index])) {
-		kobject_del(&block_group->space_info->block_group_kobjs[index]);
-		kobject_put(&block_group->space_info->block_group_kobjs[index]);
+		kobject_del(block_group->space_info->block_group_kobjs[index]);
+		kobject_put(block_group->space_info->block_group_kobjs[index]);
+		block_group->space_info->block_group_kobjs[index] = NULL;
 		clear_avail_alloc_bits(root->fs_info, block_group->flags);
 	}
 	up_write(&block_group->space_info->groups_sem);

fs/btrfs/sysfs.c +3 -2

@@ -254,6 +254,7 @@ static ssize_t global_rsv_reserved_show(
 BTRFS_ATTR(global_rsv_reserved, 0444, global_rsv_reserved_show);
 
 #define to_space_info(_kobj) container_of(_kobj, struct btrfs_space_info, kobj)
+#define to_raid_kobj(_kobj) container_of(_kobj, struct raid_kobject, kobj)
 
 static ssize_t raid_bytes_show(struct kobject *kobj,
 			       struct kobj_attribute *attr, char *buf);

@@ -266,7 +267,7 @@ static ssize_t raid_bytes_show(struct ko
 {
 	struct btrfs_space_info *sinfo = to_space_info(kobj->parent);
 	struct btrfs_block_group_cache *block_group;
-	int index = kobj - sinfo->block_group_kobjs;
+	int index = to_raid_kobj(kobj)->raid_type;
 	u64 val = 0;
 
 	down_read(&sinfo->groups_sem);

@@ -288,7 +289,7 @@ static struct attribute *raid_attributes
 
 static void release_raid_kobj(struct kobject *kobj)
 {
-	kobject_put(kobj->parent);
+	kfree(to_raid_kobj(kobj));
 }
 
 struct kobj_type btrfs_raid_ktype = {

Signature · 2 lines

Jeff Mahoney
SUSE Labs

Jeff Mahoney

author NAK

2014-05-23 19:41 UTC #1 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 14 lines · Jeff Mahoney

We are currently allocating space_info objects in an array when we allocate space_info. When a user does something like:
# btrfs balance start -mconvert=raid1 -dconvert=raid1 /mnt # btrfs
balance start -mconvert=single -dconvert=single /mnt -f # btrfs
balance start -mconvert=raid1 -dconvert=raid1 /
We can end up with memory corruption since the kobject hasn't been reinitialized properly and the name pointer was left set.
The rationale behind allocating them statically was to avoid creating a separate kobject container that just contained the raid type. It used the index in the array to determine the index.

Quoted 7 lines

Ultimately, though, this wastes more memory than it saves in all but the most complex scenarios and introduces kobject lifetime questions.
This patch allocates the kobjects dynamically instead. Note that we also remove the kobject_get/put of the parent kobject since kobject_add and kobject_del do that internally.

Nack.

Works with switching raid groups but crashes on umount.

- -Jeff

Quoted 82 lines

Signed-off-by Jeff Mahoney <[email protected]> --- fs/btrfs/ctree.h
|    8 +++++++- fs/btrfs/extent-tree.c |   33
+++++++++++++++++++++------------ fs/btrfs/sysfs.c       |    5
+++-- 3 files changed, 31 insertions(+), 15 deletions(-)
--- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -1113,6 +1113,12 @@ struct btrfs_qgroup_limit_item { __le64 rsv_excl; } __attribute__ ((__packed__));
+/* For raid type sysfs entries */ +struct raid_kobject { + int
raid_type; + struct kobject kobj; +}; + struct btrfs_space_info {
spinlock_t lock;
(quoted diff) +0 -0
@@ -1163,7 +1169,7 @@ struct btrfs_space_info { wait_queue_head_t
 wait;
 
struct kobject kobj; - struct kobject
block_group_kobjs[BTRFS_NR_RAID_TYPES]; + struct kobject
*block_group_kobjs[BTRFS_NR_RAID_TYPES]; };
#define BTRFS_BLOCK_RSV_GLOBAL 1 --- a/fs/btrfs/extent-tree.c +++
b/fs/btrfs/extent-tree.c @@ -3401,10 +3401,8 @@ static int update_space_info(struct btrf return ret; }
(quoted diff) +0 -1
-	for (i = 0; i < BTRFS_NR_RAID_TYPES; i++) { +	for (i = 0; i <
BTRFS_NR_RAID_TYPES; i++) INIT_LIST_HEAD(&found->block_groups[i]);
(quoted diff) +0 -1
-		kobject_init(&found->block_group_kobjs[i], &btrfs_raid_ktype); -
} init_rwsem(&found->groups_sem); spin_lock_init(&found->lock); found->flags = flags & BTRFS_BLOCK_GROUP_TYPE_MASK; @@ -8327,7 +8325,8 @@ int btrfs_free_block_groups(struct btrfs list_del(&space_info->list); for (i = 0; i < BTRFS_NR_RAID_TYPES;
i++) { struct kobject *kobj; - kobj =
&space_info->block_group_kobjs[i]; + kobj =
space_info->block_group_kobjs[i]; + space_info->block_group_kobjs[i] = NULL; if (kobj->parent) { kobject_del(kobj); kobject_put(kobj); @@ -8352,17 +8351,26 @@ static void __link_block_group(struct bt up_write(&space_info->groups_sem);
if (first) { - struct kobject *kobj =
&space_info->block_group_kobjs[index]; + struct raid_kobject
*rkobj; int ret;
(quoted diff) +0 -1
-		kobject_get(&space_info->kobj); /* put in release */ -		ret =
kobject_add(kobj, &space_info->kobj, "%s", -
get_raid_name(index)); + rkobj = kzalloc(sizeof(*rkobj),
GFP_KERNEL); + if (!rkobj) + goto out_err; + rkobj->raid_type =
index; + kobject_init(&rkobj->kobj, &btrfs_raid_ktype); + ret =
kobject_add(&rkobj->kobj, &space_info->kobj, + "%s",
get_raid_name(index)); if (ret) { - pr_warn("BTRFS: failed to add
kobject for block cache. ignoring.\n"); -
kobject_put(&space_info->kobj); + kobject_put(&rkobj->kobj); +
goto out_err; } + space_info->block_group_kobjs[index] =
&rkobj->kobj; } + + return; +out_err: + pr_warn("BTRFS: failed to
add kobject for block cache. ignoring.\n"); }
static struct btrfs_block_group_cache * @@ -8796,8 +8804,9 @@ int btrfs_remove_block_group(struct btrf */ list_del_init(&block_group->list); if (list_empty(&block_group->space_info->block_groups[index])) { - kobject_del(&block_group->space_info->block_group_kobjs[index]); - kobject_put(&block_group->space_info->block_group_kobjs[index]); + kobject_del(block_group->space_info->block_group_kobjs[index]); + kobject_put(block_group->space_info->block_group_kobjs[index]); + block_group->space_info->block_group_kobjs[index] = NULL; clear_avail_alloc_bits(root->fs_info, block_group->flags); } up_write(&block_group->space_info->groups_sem); --- a/fs/btrfs/sysfs.c +++ b/fs/btrfs/sysfs.c @@ -254,6 +254,7 @@ static ssize_t global_rsv_reserved_show( BTRFS_ATTR(global_rsv_reserved, 0444, global_rsv_reserved_show);
#define to_space_info(_kobj) container_of(_kobj, struct btrfs_space_info, kobj) +#define to_raid_kobj(_kobj) container_of(_kobj, struct raid_kobject, kobj)
static ssize_t raid_bytes_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf); @@ -266,7 +267,7 @@ static ssize_t raid_bytes_show(struct ko { struct btrfs_space_info *sinfo = to_space_info(kobj->parent); struct btrfs_block_group_cache
*block_group; - int index = kobj - sinfo->block_group_kobjs; + int
index = to_raid_kobj(kobj)->raid_type; u64 val = 0;

Quoted 9 lines

down_read(&sinfo->groups_sem); @@ -288,7 +289,7 @@ static struct attribute *raid_attributes
static void release_raid_kobj(struct kobject *kobj) { -
kobject_put(kobj->parent); + kfree(to_raid_kobj(kobj)); }
struct kobj_type btrfs_raid_ktype = {

- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.22 (Darwin)

iQIcBAEBAgAGBQJTf6RhAAoJEB57S2MheeWyMMsP/0qXabjwUETeCxa/VIKbAMP8 C/xzcOPrVIsxK/3EjD865o8dEoUI56FT0pWgxYxysafwdG8VVTCTJi1GXb8gl+yo JQZOS/JzLSPM+KElzBDZ+2KkDxz3wtx/XVrWQruOCQ+ycx1KVSS5TQ5vNFPMucik /MBYSx3rDmD9Np5iyv2McbZLqZDlqe57dsuCjZd0p9cBR1DguhZKuhgl1Bf4j7RR +O7Gip15SAuwYbU+Qntwe6e4i7OAuVIqeYDXP/EJtN/PWzoopwnSatYIHUFhjAXj Y+GIzxNAZP9rpCjc0akZM1zxgHi9Xrowg/2bWWGfu0GU+ChL02+CdQFt8ZUylsQQ AndOwTW8QU+PMV9qOzkDj38yrSTWTWeh9SAE85JSzaP4bRaFKqNudnwGrlYUwRqe TFbuR4JMubGCM4+vSzVgPznY7tiapNtRuoE7Ij5TYQW6arB4aXSUuk351qFGozI+ 7N1G1S7KOSFvXGCPgIiz7QqREq9uahb9nEtEBOdkuSZWxMJBmaxfBI5o6n4E0MQA UEt2IDsZV6p6FbEZGG49eXBVV6JYD0UGrW4I0HAgYFbIRx/0E+E+VuxMsTqjpGl1 NprRDOO6UdzbGM+hCYIc++wXJgq0PYJYBbOM7PKLaCLdTyRlgeZWmmUoIcXkm1oh tODf44NEMvOVF/sKIMZz
=3iv4
-----END PGP SIGNATURE-----

Jeff Mahoney

author

2014-05-23 20:06 UTC #2 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 20 lines · Jeff Mahoney

Quoted 18 lines · Jeff Mahoney
We are currently allocating space_info objects in an array when we allocate space_info. When a user does something like:
# btrfs balance start -mconvert=raid1 -dconvert=raid1 /mnt #
btrfs balance start -mconvert=single -dconvert=single /mnt -f #
btrfs balance start -mconvert=raid1 -dconvert=raid1 /
We can end up with memory corruption since the kobject hasn't been reinitialized properly and the name pointer was left set.
The rationale behind allocating them statically was to avoid creating a separate kobject container that just contained the raid type. It used the index in the array to determine the index.
Ultimately, though, this wastes more memory than it saves in all
but the most complex scenarios and introduces kobject lifetime
questions.

Quoted 7 lines

Quoted 3 lines
This patch allocates the kobjects dynamically instead. Note that we also remove the kobject_get/put of the parent kobject since kobject_add and kobject_del do that internally.
Nack.
Works with switching raid groups but crashes on umount.

It was due to if (kobj->parent) not being switched over to if (kobj) in the cleanup-for-umount code.

I've posted a fixed version.

- -Jeff

Quoted 99 lines

-Jeff
Quoted 92 lines
Signed-off-by Jeff Mahoney <[email protected]> ---
fs/btrfs/ctree.h | 8 +++++++- fs/btrfs/extent-tree.c | 33
+++++++++++++++++++++------------ fs/btrfs/sysfs.c | 5
+++-- 3 files changed, 31 insertions(+), 15 deletions(-)
--- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -1113,6 +1113,12 @@ struct btrfs_qgroup_limit_item { __le64 rsv_excl; } __attribute__ ((__packed__));
+/* For raid type sysfs entries */ +struct raid_kobject { + int
raid_type; + struct kobject kobj; +}; + struct btrfs_space_info {
spinlock_t lock;
fs/btrfs/ctree.h +0 -0
@@ -1163,7 +1169,7 @@ struct btrfs_space_info {
 wait_queue_head_t wait;
 
struct kobject kobj; - struct kobject
block_group_kobjs[BTRFS_NR_RAID_TYPES]; + struct kobject
*block_group_kobjs[BTRFS_NR_RAID_TYPES]; };
#define BTRFS_BLOCK_RSV_GLOBAL 1 --- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c @@ -3401,10 +3401,8 @@ static int update_space_info(struct btrf return ret; }
(quoted diff) +0 -1
-	for (i = 0; i < BTRFS_NR_RAID_TYPES; i++) { +	for (i = 0; i < 
BTRFS_NR_RAID_TYPES; i++) INIT_LIST_HEAD(&found->block_groups[i]); - kobject_init(&found->block_group_kobjs[i], &btrfs_raid_ktype); - } init_rwsem(&found->groups_sem); spin_lock_init(&found->lock); found->flags = flags & BTRFS_BLOCK_GROUP_TYPE_MASK; @@ -8327,7 +8325,8 @@ int btrfs_free_block_groups(struct btrfs list_del(&space_info->list); for (i = 0; i <
BTRFS_NR_RAID_TYPES; i++) { struct kobject *kobj; - kobj =
&space_info->block_group_kobjs[i]; + kobj =
space_info->block_group_kobjs[i]; + space_info->block_group_kobjs[i] = NULL; if (kobj->parent) { kobject_del(kobj); kobject_put(kobj); @@ -8352,17 +8351,26 @@ static void __link_block_group(struct bt up_write(&space_info->groups_sem);
if (first) { - struct kobject *kobj =
&space_info->block_group_kobjs[index]; + struct raid_kobject
*rkobj; int ret;
(quoted diff) +0 -1
-		kobject_get(&space_info->kobj); /* put in release */ -		ret = 
kobject_add(kobj, &space_info->kobj, "%s", -
get_raid_name(index)); + rkobj = kzalloc(sizeof(*rkobj),
GFP_KERNEL); + if (!rkobj) + goto out_err; + rkobj->raid_type
= index; + kobject_init(&rkobj->kobj, &btrfs_raid_ktype); + ret
= kobject_add(&rkobj->kobj, &space_info->kobj, + "%s",
get_raid_name(index)); if (ret) { - pr_warn("BTRFS: failed to
add kobject for block cache. ignoring.\n"); -
kobject_put(&space_info->kobj); + kobject_put(&rkobj->kobj); +
goto out_err; } + space_info->block_group_kobjs[index] =
&rkobj->kobj; } + + return; +out_err: + pr_warn("BTRFS: failed
to add kobject for block cache. ignoring.\n"); }
static struct btrfs_block_group_cache * @@ -8796,8 +8804,9 @@ int btrfs_remove_block_group(struct btrf */ list_del_init(&block_group->list); if (list_empty(&block_group->space_info->block_groups[index])) { - kobject_del(&block_group->space_info->block_group_kobjs[index]); - kobject_put(&block_group->space_info->block_group_kobjs[index]);
+ kobject_del(block_group->space_info->block_group_kobjs[index]);
+ kobject_put(block_group->space_info->block_group_kobjs[index]);
+ block_group->space_info->block_group_kobjs[index] = NULL; clear_avail_alloc_bits(root->fs_info, block_group->flags); } up_write(&block_group->space_info->groups_sem); --- a/fs/btrfs/sysfs.c +++ b/fs/btrfs/sysfs.c @@ -254,6 +254,7 @@ static ssize_t global_rsv_reserved_show( BTRFS_ATTR(global_rsv_reserved, 0444, global_rsv_reserved_show);
#define to_space_info(_kobj) container_of(_kobj, struct btrfs_space_info, kobj) +#define to_raid_kobj(_kobj) container_of(_kobj, struct raid_kobject, kobj)
static ssize_t raid_bytes_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf); @@ -266,7 +267,7 @@ static ssize_t raid_bytes_show(struct ko { struct btrfs_space_info *sinfo = to_space_info(kobj->parent); struct
btrfs_block_group_cache *block_group; - int index = kobj -
sinfo->block_group_kobjs; + int index =
to_raid_kobj(kobj)->raid_type; u64 val = 0;
down_read(&sinfo->groups_sem); @@ -288,7 +289,7 @@ static struct attribute *raid_attributes
static void release_raid_kobj(struct kobject *kobj) { -
kobject_put(kobj->parent); + kfree(to_raid_kobj(kobj)); }
struct kobj_type btrfs_raid_ktype = {

- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.22 (Darwin)

iQIcBAEBAgAGBQJTf6pVAAoJEB57S2MheeWylvwP/jaG8w4Pnu/0+FvxW10xMcFN HPJyS98JfekVDvnGgVfOA4UrI1ioK3Luwp8jAbk4tnbmZS4pryeJFhTDMpXYLzgY w5LuXDxMxwQK8U6P47IuYnnRNL0+k4yiBAVT+ULOc2Ly0y8MItl27a1UxWMofzbm KtNRKwpJpA1QQWDz3aTeLgCGEHa+fGm9BBu5l7RE4wLAFYzYdWYXf0//kv/P0tZo rruAHIPggsd4tSJwnQBbXTlfJMTf4vFBm1wceeLeXSKTgcZjO9oHOCKlo019nrkw nR8FwBiWvmFox4m1Ub4zHs9AtIEdbnkUly9/6+L7Bc+L41ro5iMuG1tOrZo+5KJy vjfcYJTRtn9T3ThoKojsP3Uu8HxFSB5ggi8znmZuzLU5yZMB4dwbGojoq6l4hAvj XnDXYZapp3FQeBIMGUnEaValmiaYfCV2HjSMf07NvxTY40/Ce3XM6b35J5ESr0ms ilzFdXPTg2GifWFzSXprunXsvxRRviQrys740U8nW1MOnyTIEipb75fMNmcfT49J ABzkuvFnI6z82bVK02q1XifKgVsQ4T35aDW2O/AzHWfmIPistdstDFUrRntWLZCS km29dtdSQC8Ql5uJrw7pRby75O9TRlMtKrD6pic/BfD3aM77DCudnWEhkvm1WEEf toOjMaX0w5Nk5/VYSCHc
=JmMQ
-----END PGP SIGNATURE-----