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

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(-)

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

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

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