[PATCH 0/3] Improve kobject handling in fs/ext4

[PATCH 0/3] Improve kobject handling in fs/ext4

[Riccardo Schirone]

This patch tries to correctly use kobjects in fs/ext4. In particular it
allocates kobjects/ksets dynamically, instead of statically, and improve
error handling in case kobject_* functions fail.

It is based on
https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
branch.

Riccardo Schirone (3):
  fs/ext4: release kobject/kset even when init/register fail
  fs/ext4: create ext4_feat kobject dynamically
  fs/ext4: create ext4_kset dynamically

 fs/ext4/sysfs.c | 63 ++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 45 insertions(+), 18 deletions(-)

-- 
2.14.3

[PATCH 1/3] fs/ext4: release kobject/kset even when init/register fail

[Riccardo Schirone]

Even when kobject_init_and_add/kset_register fail, the kobject has been
already initialized and the refcount set to 1. Thus it is necessary to
release the kobject/kset, to avoid the memory associated with it hanging
around forever.

Signed-off-by: Riccardo Schirone <sirmy15@gmail.com>
---
 fs/ext4/sysfs.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
index 48c7a7d55ed3..b096e157934f 100644
--- a/fs/ext4/sysfs.c
+++ b/fs/ext4/sysfs.c
@@ -395,8 +395,11 @@ int ext4_register_sysfs(struct super_block *sb)
 	init_completion(&sbi->s_kobj_unregister);
 	err = kobject_init_and_add(&sbi->s_kobj, &ext4_sb_ktype, NULL,
 				   "%s", sb->s_id);
-	if (err)
+	if (err) {
+		kobject_put(&sbi->s_kobj);
+		wait_for_completion(&sbi->s_kobj_unregister);
 		return err;
+	}
 
 	if (ext4_proc_root)
 		sbi->s_proc = proc_mkdir(sb->s_id, ext4_proc_root);
@@ -429,15 +432,19 @@ int __init ext4_init_sysfs(void)
 	kobject_set_name(&ext4_kset.kobj, "ext4");
 	ext4_kset.kobj.parent = fs_kobj;
 	ret = kset_register(&ext4_kset);
-	if (ret)
+	if (ret) {
+		kset_unregister(&ext4_kset);
 		return ret;
+	}
 
 	ret = kobject_init_and_add(&ext4_feat, &ext4_feat_ktype,
 				   NULL, "features");
-	if (ret)
+	if (ret) {
+		kobject_put(&ext4_feat);
 		kset_unregister(&ext4_kset);
-	else
+	} else {
 		ext4_proc_root = proc_mkdir(proc_dirname, NULL);
+	}
 	return ret;
 }
 
-- 
2.14.3

Re: [PATCH 1/3] fs/ext4: release kobject/kset even when init/register fail

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 2104 bytes --]

On Nov 27, 2017, at 4:17 PM, Riccardo Schirone <sirmy15@gmail.com> wrote:
> 
> Even when kobject_init_and_add/kset_register fail, the kobject has been
> already initialized and the refcount set to 1. Thus it is necessary to
> release the kobject/kset, to avoid the memory associated with it hanging
> around forever.

This seems like a bad programming model.  It doesn't make sense if the
"init" or "register" function returns an error that you would still have
to call "put" or "unregister" on the object.  Why not just handle the
cleanup at the end of "kobject_init_and_add()" or "kobject_register()"
if there is an error instead of putting the burden on every caller?

If open() returns an error, I don't need to call close(), and if malloc()
fails I don't need to call free(), etc.

Cheers, Andreas

> Signed-off-by: Riccardo Schirone <sirmy15@gmail.com>
> ---
> fs/ext4/sysfs.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
> index 48c7a7d55ed3..b096e157934f 100644
> --- a/fs/ext4/sysfs.c
> +++ b/fs/ext4/sysfs.c
> @@ -395,8 +395,11 @@ int ext4_register_sysfs(struct super_block *sb)
> 	init_completion(&sbi->s_kobj_unregister);
> 	err = kobject_init_and_add(&sbi->s_kobj, &ext4_sb_ktype, NULL,
> 				   "%s", sb->s_id);
> -	if (err)
> +	if (err) {
> +		kobject_put(&sbi->s_kobj);
> +		wait_for_completion(&sbi->s_kobj_unregister);
> 		return err;
> +	}
> 
> 	if (ext4_proc_root)
> 		sbi->s_proc = proc_mkdir(sb->s_id, ext4_proc_root);
> @@ -429,15 +432,19 @@ int __init ext4_init_sysfs(void)
> 	kobject_set_name(&ext4_kset.kobj, "ext4");
> 	ext4_kset.kobj.parent = fs_kobj;
> 	ret = kset_register(&ext4_kset);
> -	if (ret)
> +	if (ret) {
> +		kset_unregister(&ext4_kset);
> 		return ret;
> +	}
> 
> 	ret = kobject_init_and_add(&ext4_feat, &ext4_feat_ktype,
> 				   NULL, "features");
> -	if (ret)
> +	if (ret) {
> +		kobject_put(&ext4_feat);
> 		kset_unregister(&ext4_kset);
> -	else
> +	} else {
> 		ext4_proc_root = proc_mkdir(proc_dirname, NULL);
> +	}
> 	return ret;
> }
> 
> --
> 2.14.3
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 1/3] fs/ext4: release kobject/kset even when init/register fail

[Riccardo S.]

On 11/27, Andreas Dilger wrote:
> On Nov 27, 2017, at 4:17 PM, Riccardo Schirone <sirmy15@gmail.com> wrote:
> > 
> > Even when kobject_init_and_add/kset_register fail, the kobject has been
> > already initialized and the refcount set to 1. Thus it is necessary to
> > release the kobject/kset, to avoid the memory associated with it hanging
> > around forever.
> 
> This seems like a bad programming model.  It doesn't make sense if the
> "init" or "register" function returns an error that you would still have
> to call "put" or "unregister" on the object.  Why not just handle the
> cleanup at the end of "kobject_init_and_add()" or "kobject_register()"
> if there is an error instead of putting the burden on every caller?
> 
> If open() returns an error, I don't need to call close(), and if malloc()
> fails I don't need to call free(), etc.
> 
> Cheers, Andreas
> 

I agree it seems weird at first, but it looks to me the examples you
made involve the *creation* of new objects, so it makes sense that the
function, in case of errors, does not create them at all.

In this case though the kobject_init_and_add is really just a shortcut
for kobject_init + kobject_add and none of these functions create
something for you. It's the caller's job to allocate the necessary
memory, so I think that's the reason why it's still the caller that
calls kobject_put when there's something wrong.

Instead, in kobject_create_and_add, where the creation of the kobject is
inside the function, the kobject is cleaned up if something wrong
happens.

Those are my thoughts anyway.
Regards,


Riccardo

Re: [PATCH 1/3] fs/ext4: release kobject/kset even when init/register fail

[Theodore Ts'o]

On Tue, Nov 28, 2017 at 12:17:59AM +0100, Riccardo Schirone wrote:
> Even when kobject_init_and_add/kset_register fail, the kobject has been
> already initialized and the refcount set to 1. Thus it is necessary to
> release the kobject/kset, to avoid the memory associated with it hanging
> around forever.
> 
> Signed-off-by: Riccardo Schirone <sirmy15@gmail.com>

Thanks, applied.

						- Ted

[PATCH 2/3] fs/ext4: create ext4_feat kobject dynamically

[Riccardo Schirone]

kobjects should always be allocated dynamically, because it is unknown
to whoever creates them when kobjects can be released.

Signed-off-by: Riccardo Schirone <sirmy15@gmail.com>
---
 fs/ext4/sysfs.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
index b096e157934f..c447f23cc876 100644
--- a/fs/ext4/sysfs.c
+++ b/fs/ext4/sysfs.c
@@ -10,6 +10,7 @@
 #include <linux/time.h>
 #include <linux/fs.h>
 #include <linux/seq_file.h>
+#include <linux/slab.h>
 #include <linux/proc_fs.h>
 
 #include "ext4.h"
@@ -350,11 +351,10 @@ static struct kset ext4_kset = {
 static struct kobj_type ext4_feat_ktype = {
 	.default_attrs	= ext4_feat_attrs,
 	.sysfs_ops	= &ext4_attr_ops,
+	.release	= (void (*)(struct kobject *))kfree,
 };
 
-static struct kobject ext4_feat = {
-	.kset	= &ext4_kset,
-};
+static struct kobject *ext4_feat;
 
 #define PROC_FILE_SHOW_DEFN(name) \
 static int name##_open(struct inode *inode, struct file *file) \
@@ -437,20 +437,31 @@ int __init ext4_init_sysfs(void)
 		return ret;
 	}
 
-	ret = kobject_init_and_add(&ext4_feat, &ext4_feat_ktype,
-				   NULL, "features");
-	if (ret) {
-		kobject_put(&ext4_feat);
-		kset_unregister(&ext4_kset);
-	} else {
-		ext4_proc_root = proc_mkdir(proc_dirname, NULL);
+	ext4_feat = kzalloc(sizeof(*ext4_feat), GFP_KERNEL);
+	if (!ext4_feat) {
+		ret = -ENOMEM;
+		goto kset_err;
 	}
+
+	ext4_feat->kset = &ext4_kset;
+	ret = kobject_init_and_add(ext4_feat, &ext4_feat_ktype,
+				   NULL, "features");
+	if (ret)
+		goto feat_err;
+
+	ext4_proc_root = proc_mkdir(proc_dirname, NULL);
+	return ret;
+
+feat_err:
+	kobject_put(ext4_feat);
+kset_err:
+	kset_unregister(&ext4_kset);
 	return ret;
 }
 
 void ext4_exit_sysfs(void)
 {
-	kobject_put(&ext4_feat);
+	kobject_put(ext4_feat);
 	kset_unregister(&ext4_kset);
 	remove_proc_entry(proc_dirname, NULL);
 	ext4_proc_root = NULL;
-- 
2.14.3

Re: [PATCH 2/3] fs/ext4: create ext4_feat kobject dynamically

[Theodore Ts'o]

On Tue, Nov 28, 2017 at 12:18:00AM +0100, Riccardo Schirone wrote:
> kobjects should always be allocated dynamically, because it is unknown
> to whoever creates them when kobjects can be released.
> 
> Signed-off-by: Riccardo Schirone <sirmy15@gmail.com>

Thanks, applied.

					- Ted

[PATCH 3/3] fs/ext4: create ext4_kset dynamically

[Riccardo Schirone]

ksets contain a kobject and they should always be allocated dynamically,
because it is unknown to whoever creates them when ksets can be
released.

Signed-off-by: Riccardo Schirone <sirmy15@gmail.com>
---
 fs/ext4/sysfs.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
index c447f23cc876..a33b406b8feb 100644
--- a/fs/ext4/sysfs.c
+++ b/fs/ext4/sysfs.c
@@ -329,6 +329,13 @@ static void ext4_sb_release(struct kobject *kobj)
 	complete(&sbi->s_kobj_unregister);
 }
 
+static void ext4_kset_release(struct kobject *kobj)
+{
+	struct kset *kset = container_of(kobj, struct kset, kobj);
+
+	kfree(kset);
+}
+
 static const struct sysfs_ops ext4_attr_ops = {
 	.show	= ext4_attr_show,
 	.store	= ext4_attr_store,
@@ -342,11 +349,10 @@ static struct kobj_type ext4_sb_ktype = {
 
 static struct kobj_type ext4_ktype = {
 	.sysfs_ops	= &ext4_attr_ops,
+	.release	= ext4_kset_release,
 };
 
-static struct kset ext4_kset = {
-	.kobj   = {.ktype = &ext4_ktype},
-};
+static struct kset *ext4_kset;
 
 static struct kobj_type ext4_feat_ktype = {
 	.default_attrs	= ext4_feat_attrs,
@@ -391,7 +397,7 @@ int ext4_register_sysfs(struct super_block *sb)
 	const struct ext4_proc_files *p;
 	int err;
 
-	sbi->s_kobj.kset = &ext4_kset;
+	sbi->s_kobj.kset = ext4_kset;
 	init_completion(&sbi->s_kobj_unregister);
 	err = kobject_init_and_add(&sbi->s_kobj, &ext4_sb_ktype, NULL,
 				   "%s", sb->s_id);
@@ -429,13 +435,16 @@ int __init ext4_init_sysfs(void)
 {
 	int ret;
 
-	kobject_set_name(&ext4_kset.kobj, "ext4");
-	ext4_kset.kobj.parent = fs_kobj;
-	ret = kset_register(&ext4_kset);
-	if (ret) {
-		kset_unregister(&ext4_kset);
-		return ret;
-	}
+	ext4_kset = kzalloc(sizeof(*ext4_kset), GFP_KERNEL);
+	if (!ext4_kset)
+		return -ENOMEM;
+
+	kobject_set_name(&ext4_kset->kobj, "ext4");
+	ext4_kset->kobj.parent = fs_kobj;
+	ext4_kset->kobj.ktype = &ext4_ktype;
+	ret = kset_register(ext4_kset);
+	if (ret)
+		goto kset_err;
 
 	ext4_feat = kzalloc(sizeof(*ext4_feat), GFP_KERNEL);
 	if (!ext4_feat) {
@@ -443,7 +452,7 @@ int __init ext4_init_sysfs(void)
 		goto kset_err;
 	}
 
-	ext4_feat->kset = &ext4_kset;
+	ext4_feat->kset = ext4_kset;
 	ret = kobject_init_and_add(ext4_feat, &ext4_feat_ktype,
 				   NULL, "features");
 	if (ret)
@@ -455,14 +464,14 @@ int __init ext4_init_sysfs(void)
 feat_err:
 	kobject_put(ext4_feat);
 kset_err:
-	kset_unregister(&ext4_kset);
+	kset_unregister(ext4_kset);
 	return ret;
 }
 
 void ext4_exit_sysfs(void)
 {
 	kobject_put(ext4_feat);
-	kset_unregister(&ext4_kset);
+	kset_unregister(ext4_kset);
 	remove_proc_entry(proc_dirname, NULL);
 	ext4_proc_root = NULL;
 }
-- 
2.14.3

Re: [PATCH 3/3] fs/ext4: create ext4_kset dynamically

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 3224 bytes --]

On Nov 27, 2017, at 4:18 PM, Riccardo Schirone <sirmy15@gmail.com> wrote:
> 
> ksets contain a kobject and they should always be allocated dynamically,
> because it is unknown to whoever creates them when ksets can be
> released.
> 
> Signed-off-by: Riccardo Schirone <sirmy15@gmail.com>
> ---
> fs/ext4/sysfs.c | 37 +++++++++++++++++++++++--------------
> 1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
> index c447f23cc876..a33b406b8feb 100644
> --- a/fs/ext4/sysfs.c
> +++ b/fs/ext4/sysfs.c
> @@ -329,6 +329,13 @@ static void ext4_sb_release(struct kobject *kobj)
> 	complete(&sbi->s_kobj_unregister);
> }
> 
> +static void ext4_kset_release(struct kobject *kobj)
> +{
> +	struct kset *kset = container_of(kobj, struct kset, kobj);
> +
> +	kfree(kset);
> +}
> +
> static const struct sysfs_ops ext4_attr_ops = {
> 	.show	= ext4_attr_show,
> 	.store	= ext4_attr_store,
> @@ -342,11 +349,10 @@ static struct kobj_type ext4_sb_ktype = {
> 
> static struct kobj_type ext4_ktype = {
> 	.sysfs_ops	= &ext4_attr_ops,
> +	.release	= ext4_kset_release,
> };
> 
> -static struct kset ext4_kset = {
> -	.kobj   = {.ktype = &ext4_ktype},
> -};
> +static struct kset *ext4_kset;
> 
> static struct kobj_type ext4_feat_ktype = {
> 	.default_attrs	= ext4_feat_attrs,
> @@ -391,7 +397,7 @@ int ext4_register_sysfs(struct super_block *sb)
> 	const struct ext4_proc_files *p;
> 	int err;
> 
> -	sbi->s_kobj.kset = &ext4_kset;
> +	sbi->s_kobj.kset = ext4_kset;
> 	init_completion(&sbi->s_kobj_unregister);
> 	err = kobject_init_and_add(&sbi->s_kobj, &ext4_sb_ktype, NULL,
> 				   "%s", sb->s_id);
> @@ -429,13 +435,16 @@ int __init ext4_init_sysfs(void)
> {
> 	int ret;
> 
> -	kobject_set_name(&ext4_kset.kobj, "ext4");
> -	ext4_kset.kobj.parent = fs_kobj;
> -	ret = kset_register(&ext4_kset);
> -	if (ret) {
> -		kset_unregister(&ext4_kset);
> -		return ret;
> -	}
> +	ext4_kset = kzalloc(sizeof(*ext4_kset), GFP_KERNEL);
> +	if (!ext4_kset)
> +		return -ENOMEM;
> +
> +	kobject_set_name(&ext4_kset->kobj, "ext4");
> +	ext4_kset->kobj.parent = fs_kobj;
> +	ext4_kset->kobj.ktype = &ext4_ktype;
> +	ret = kset_register(ext4_kset);
> +	if (ret)
> +		goto kset_err;
> 
> 	ext4_feat = kzalloc(sizeof(*ext4_feat), GFP_KERNEL);
> 	if (!ext4_feat) {
> @@ -443,7 +452,7 @@ int __init ext4_init_sysfs(void)
> 		goto kset_err;
> 	}
> 
> -	ext4_feat->kset = &ext4_kset;
> +	ext4_feat->kset = ext4_kset;
> 	ret = kobject_init_and_add(ext4_feat, &ext4_feat_ktype,
> 				   NULL, "features");
> 	if (ret)
> @@ -455,14 +464,14 @@ int __init ext4_init_sysfs(void)
> feat_err:
> 	kobject_put(ext4_feat);
> kset_err:
> -	kset_unregister(&ext4_kset);
> +	kset_unregister(ext4_kset);


It would be prudent in this case to set "ext4_kset = NULL" here
so that it isn't cleaned up again somewhere else.  Otherwise,
it seems possible that ext4_kset could be cleaned up twice.

Otherwise, the whole premise of this patch seems flawed.

> 	return ret;
> }
> 
> void ext4_exit_sysfs(void)
> {
> 	kobject_put(ext4_feat);
> -	kset_unregister(&ext4_kset);
> +	kset_unregister(ext4_kset);

Same here.

> 	remove_proc_entry(proc_dirname, NULL);
> 	ext4_proc_root = NULL;
> }
> --
> 2.14.3
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 3/3] fs/ext4: create ext4_kset dynamically

[Riccardo S.]

On 11/27, Andreas Dilger wrote:
> On Nov 27, 2017, at 4:18 PM, Riccardo Schirone <sirmy15@gmail.com> wrote:
> > 
> > -	ext4_feat->kset = &ext4_kset;
> > +	ext4_feat->kset = ext4_kset;
> > 	ret = kobject_init_and_add(ext4_feat, &ext4_feat_ktype,
> > 				   NULL, "features");
> > 	if (ret)
> > @@ -455,14 +464,14 @@ int __init ext4_init_sysfs(void)
> > feat_err:
> > 	kobject_put(ext4_feat);
> > kset_err:
> > -	kset_unregister(&ext4_kset);
> > +	kset_unregister(ext4_kset);
> 
> 
> It would be prudent in this case to set "ext4_kset = NULL" here
> so that it isn't cleaned up again somewhere else.  Otherwise,
> it seems possible that ext4_kset could be cleaned up twice.
> 
> Otherwise, the whole premise of this patch seems flawed.

Right, I'll do it in V2.

> 
> > 	return ret;
> > }
> > 
> > void ext4_exit_sysfs(void)
> > {
> > 	kobject_put(ext4_feat);
> > -	kset_unregister(&ext4_kset);
> > +	kset_unregister(ext4_kset);
> 
> Same here.
> 
> > 	remove_proc_entry(proc_dirname, NULL);
> > 	ext4_proc_root = NULL;
> > }
> > --
> > 2.14.3
> > 
> 
> 
> Cheers, Andreas
> 
> 

Thanks,

Riccardo

Re: [PATCH 3/3] fs/ext4: create ext4_kset dynamically

[Theodore Ts'o]

On Tue, Nov 28, 2017 at 11:51:47AM +0100, Riccardo S. wrote:
> On 11/27, Andreas Dilger wrote:
> > On Nov 27, 2017, at 4:18 PM, Riccardo Schirone <sirmy15@gmail.com> wrote:
> > > 
> > > -	ext4_feat->kset = &ext4_kset;
> > > +	ext4_feat->kset = ext4_kset;
> > > 	ret = kobject_init_and_add(ext4_feat, &ext4_feat_ktype,
> > > 				   NULL, "features");
> > > 	if (ret)
> > > @@ -455,14 +464,14 @@ int __init ext4_init_sysfs(void)
> > > feat_err:
> > > 	kobject_put(ext4_feat);
> > > kset_err:
> > > -	kset_unregister(&ext4_kset);
> > > +	kset_unregister(ext4_kset);
> > 
> > 
> > It would be prudent in this case to set "ext4_kset = NULL" here
> > so that it isn't cleaned up again somewhere else.  Otherwise,
> > it seems possible that ext4_kset could be cleaned up twice.
> > 
> > Otherwise, the whole premise of this patch seems flawed.
> 
> Right, I'll do it in V2.

I don't think we ever got a V2, so I've applied this patch with
Andreas's suggestions.

  	      	      	    	    	 - Ted
					 

Re: [PATCH 0/3] Improve kobject handling in fs/ext4

[Theodore Ts'o]

On Tue, Nov 28, 2017 at 12:17:58AM +0100, Riccardo Schirone wrote:
> This patch tries to correctly use kobjects in fs/ext4. In particular it
> allocates kobjects/ksets dynamically, instead of statically, and improve
> error handling in case kobject_* functions fail.

I don't see the point of allocating the kobjects in question
dynamically?  There is only one of them, so why not use static
allocation?  What is "incorrect" about not doing dynamically allocated
kobjects/ksets?

Regards,

						- Ted

Re: [PATCH 0/3] Improve kobject handling in fs/ext4

[Riccardo S.]

On 11/27, Theodore Ts'o wrote:
> On Tue, Nov 28, 2017 at 12:17:58AM +0100, Riccardo Schirone wrote:
> > This patch tries to correctly use kobjects in fs/ext4. In particular it
> > allocates kobjects/ksets dynamically, instead of statically, and improve
> > error handling in case kobject_* functions fail.
> 
> I don't see the point of allocating the kobjects in question
> dynamically?  There is only one of them, so why not use static
> allocation?  What is "incorrect" about not doing dynamically allocated
> kobjects/ksets?
> 
> Regards,
> 
> 						- Ted

According to Documentation/kobject.txt that's how they should be
allocated.  The reason for this being they are dynamic objects, so you
don't really know what is their life time. Other parts of the kernel
could get a reference to those kobjects (since they are registered in
the system) and it's not said the module is the last one to access them.

If you declare them statically, what would happen when you remove the
module that statically defines them? What if there's still someone
holding a reference to one of those kobjects?

Regards,


Riccardo