Hi,
Is it ok to send patches during the merge window? Applies on top of
Linus' mainline tag: v5.1, happy to rebase if there are conflicts.
While auditing kobject_init_and_add() calls throughout the kernel it was
found that btrfs potentially has a couple of memleaks in the error path
code for kobject_init_and_add().
Failing calls to kobject_init_and_add() should be followed by a call to
kobject_put() since kobject_init_and_add() always calls kobject_init().
Of note, adding kobject_put() causes the release method to be called if
kobject_init_and_add() fails. For patch #1 this means we don't have to
manually free the space_info or call percpu_counter_destroy() since
these are both done by the release method. In the second patch, I
believe the added call to kobject_put() fits in with the fs_devices
lifecycle assumptions of open_ctree() but please could you review since
I am new to this code.
CC'ing the kobject maintainers/reviewers also.
Thanks,
Tobin.
Tobin C. Harding (2):
fs: btrfs: Fix error path kobject memory leak
fs: btrfs: Don't leak memory when failing add fsid
fs/btrfs/extent-tree.c | 3 +--
fs/btrfs/sysfs.c | 7 ++++++-
2 files changed, 7 insertions(+), 3 deletions(-)
--
2.21.0
If a call to kobject_init_and_add() fails we must call kobject_put()
otherwise we leak memory.
Calling kobject_put() when kobject_init_and_add() fails drops the
refcount back to 0 and calls the ktype release method.
Add call to kobject_put() in the error path of call to
kobject_init_and_add().
Signed-off-by: Tobin C. Harding <[email protected]>
---
fs/btrfs/extent-tree.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index c5880329ae37..5e40c8f1e97a 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3981,8 +3981,7 @@ static int create_space_info(struct btrfs_fs_info *info, u64 flags)
info->space_info_kobj, "%s",
alloc_name(space_info->flags));
if (ret) {
- percpu_counter_destroy(&space_info->total_bytes_pinned);
- kfree(space_info);
+ kobject_put(&space_info->kobj);
return ret;
}
--
2.21.0
A failed call to kobject_init_and_add() must be followed by a call to
kobject_put(). Currently in the error path when adding fs_devices we
are missing this call. This could be fixed by calling
btrfs_sysfs_remove_fsid() if btrfs_sysfs_add_fsid() returns an error or
by adding a call to kobject_put() directly in btrfs_sysfs_add_fsid().
Here we choose the second option because it prevents the slightly
unusual error path handling requirements of kobject from leaking out
into btrfs functions.
Add a call to kobject_put() in the error path of kobject_add_and_init().
This causes the release method to be called if kobject_init_and_add()
fails. open_tree() is the function that calls btrfs_sysfs_add_fsid()
and the error code in this function is already written with the
assumption that the release method is called during the error path of
open_tree() (as seen by the call to btrfs_sysfs_remove_fsid() under the
fail_fsdev_sysfs label).
Signed-off-by: Tobin C. Harding <[email protected]>
---
fs/btrfs/sysfs.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 5a5930e3d32b..2f078b77fe14 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -825,7 +825,12 @@ int btrfs_sysfs_add_fsid(struct btrfs_fs_devices *fs_devs,
fs_devs->fsid_kobj.kset = btrfs_kset;
error = kobject_init_and_add(&fs_devs->fsid_kobj,
&btrfs_ktype, parent, "%pU", fs_devs->fsid);
- return error;
+ if (error) {
+ kobject_put(&fs_devs->fsid_kobj);
+ return error;
+ }
+
+ return 0;
}
int btrfs_sysfs_add_mounted(struct btrfs_fs_info *fs_info)
--
2.21.0
On 13.05.19 г. 6:39 ч., Tobin C. Harding wrote:
> If a call to kobject_init_and_add() fails we must call kobject_put()
> otherwise we leak memory.
>
> Calling kobject_put() when kobject_init_and_add() fails drops the
> refcount back to 0 and calls the ktype release method.
>
> Add call to kobject_put() in the error path of call to
> kobject_init_and_add().
>
> Signed-off-by: Tobin C. Harding <[email protected]>
> ---
> fs/btrfs/extent-tree.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index c5880329ae37..5e40c8f1e97a 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -3981,8 +3981,7 @@ static int create_space_info(struct btrfs_fs_info *info, u64 flags)
> info->space_info_kobj, "%s",
> alloc_name(space_info->flags));
> if (ret) {
> - percpu_counter_destroy(&space_info->total_bytes_pinned);
> - kfree(space_info);
> + kobject_put(&space_info->kobj);
If you are only fixing kobject-related code then why do you delete
correct code as well? percpu_counter_Destroy is needed to dispose of the
percpu state which might have been allocated in percpu_counter_init
based on whether CONFIG_SMP is enabled or not? Also, the call to kfree
is required.
> return ret;
> }
>
>
On 13.05.19 г. 6:39 ч., Tobin C. Harding wrote:
> A failed call to kobject_init_and_add() must be followed by a call to
> kobject_put(). Currently in the error path when adding fs_devices we
> are missing this call. This could be fixed by calling
> btrfs_sysfs_remove_fsid() if btrfs_sysfs_add_fsid() returns an error or
> by adding a call to kobject_put() directly in btrfs_sysfs_add_fsid().
> Here we choose the second option because it prevents the slightly
> unusual error path handling requirements of kobject from leaking out
> into btrfs functions.
>
> Add a call to kobject_put() in the error path of kobject_add_and_init().
> This causes the release method to be called if kobject_init_and_add()
> fails. open_tree() is the function that calls btrfs_sysfs_add_fsid()
> and the error code in this function is already written with the
> assumption that the release method is called during the error path of
> open_tree() (as seen by the call to btrfs_sysfs_remove_fsid() under the
> fail_fsdev_sysfs label).
I'm not familiar with the internals of kobject but
btrfs_sysfs_remove_fsid calls __btrfs_sysfs_remove_fsid which in turn
does kobject_del followed by kobject_put so its sequence is not exactly
identical with your change. Presumably kobject_del is only required if
you want to dispose of successfully registered sysfs node. This implies
that __btrfs_sysfs_remove_fsid is actually broken when it comes to
handling failed sysfs_add_fsid?
>
> Signed-off-by: Tobin C. Harding <[email protected]>
> ---
> fs/btrfs/sysfs.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index 5a5930e3d32b..2f078b77fe14 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -825,7 +825,12 @@ int btrfs_sysfs_add_fsid(struct btrfs_fs_devices *fs_devs,
> fs_devs->fsid_kobj.kset = btrfs_kset;
> error = kobject_init_and_add(&fs_devs->fsid_kobj,
> &btrfs_ktype, parent, "%pU", fs_devs->fsid);
> - return error;
> + if (error) {
> + kobject_put(&fs_devs->fsid_kobj);
> + return error;
> + }
> +
> + return 0;
> }
>
> int btrfs_sysfs_add_mounted(struct btrfs_fs_info *fs_info)
>
On Mon, May 13, 2019 at 08:59:56AM +0300, Nikolay Borisov wrote:
>
>
> On 13.05.19 г. 6:39 ч., Tobin C. Harding wrote:
> > If a call to kobject_init_and_add() fails we must call kobject_put()
> > otherwise we leak memory.
> >
> > Calling kobject_put() when kobject_init_and_add() fails drops the
> > refcount back to 0 and calls the ktype release method.
> >
> > Add call to kobject_put() in the error path of call to
> > kobject_init_and_add().
> >
> > Signed-off-by: Tobin C. Harding <[email protected]>
> > ---
> > fs/btrfs/extent-tree.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> > index c5880329ae37..5e40c8f1e97a 100644
> > --- a/fs/btrfs/extent-tree.c
> > +++ b/fs/btrfs/extent-tree.c
> > @@ -3981,8 +3981,7 @@ static int create_space_info(struct btrfs_fs_info *info, u64 flags)
> > info->space_info_kobj, "%s",
> > alloc_name(space_info->flags));
> > if (ret) {
> > - percpu_counter_destroy(&space_info->total_bytes_pinned);
> > - kfree(space_info);
> > + kobject_put(&space_info->kobj);
>
> If you are only fixing kobject-related code then why do you delete
> correct code as well? percpu_counter_Destroy is needed to dispose of the
> percpu state which might have been allocated in percpu_counter_init
> based on whether CONFIG_SMP is enabled or not? Also, the call to kfree
> is required.
Both of those will happen in space_info_release() when the kobject is
properly disposed of with this last put to the kobject reference.
Reviewed-by: Greg Kroah-Hartman <[email protected]>
On Mon, May 13, 2019 at 01:39:12PM +1000, Tobin C. Harding wrote:
> A failed call to kobject_init_and_add() must be followed by a call to
> kobject_put(). Currently in the error path when adding fs_devices we
> are missing this call. This could be fixed by calling
> btrfs_sysfs_remove_fsid() if btrfs_sysfs_add_fsid() returns an error or
> by adding a call to kobject_put() directly in btrfs_sysfs_add_fsid().
> Here we choose the second option because it prevents the slightly
> unusual error path handling requirements of kobject from leaking out
> into btrfs functions.
>
> Add a call to kobject_put() in the error path of kobject_add_and_init().
> This causes the release method to be called if kobject_init_and_add()
> fails. open_tree() is the function that calls btrfs_sysfs_add_fsid()
> and the error code in this function is already written with the
> assumption that the release method is called during the error path of
> open_tree() (as seen by the call to btrfs_sysfs_remove_fsid() under the
> fail_fsdev_sysfs label).
>
> Signed-off-by: Tobin C. Harding <[email protected]>
> ---
> fs/btrfs/sysfs.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index 5a5930e3d32b..2f078b77fe14 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -825,7 +825,12 @@ int btrfs_sysfs_add_fsid(struct btrfs_fs_devices *fs_devs,
> fs_devs->fsid_kobj.kset = btrfs_kset;
> error = kobject_init_and_add(&fs_devs->fsid_kobj,
> &btrfs_ktype, parent, "%pU", fs_devs->fsid);
> - return error;
> + if (error) {
> + kobject_put(&fs_devs->fsid_kobj);
> + return error;
> + }
> +
> + return 0;
> }
>
> int btrfs_sysfs_add_mounted(struct btrfs_fs_info *fs_info)
Reviewed-by: Greg Kroah-Hartman <[email protected]>
On Mon, May 13, 2019 at 09:04:49AM +0300, Nikolay Borisov wrote:
>
>
> On 13.05.19 г. 6:39 ч., Tobin C. Harding wrote:
> > A failed call to kobject_init_and_add() must be followed by a call to
> > kobject_put(). Currently in the error path when adding fs_devices we
> > are missing this call. This could be fixed by calling
> > btrfs_sysfs_remove_fsid() if btrfs_sysfs_add_fsid() returns an error or
> > by adding a call to kobject_put() directly in btrfs_sysfs_add_fsid().
> > Here we choose the second option because it prevents the slightly
> > unusual error path handling requirements of kobject from leaking out
> > into btrfs functions.
> >
> > Add a call to kobject_put() in the error path of kobject_add_and_init().
> > This causes the release method to be called if kobject_init_and_add()
> > fails. open_tree() is the function that calls btrfs_sysfs_add_fsid()
> > and the error code in this function is already written with the
> > assumption that the release method is called during the error path of
> > open_tree() (as seen by the call to btrfs_sysfs_remove_fsid() under the
> > fail_fsdev_sysfs label).
>
> I'm not familiar with the internals of kobject but
> btrfs_sysfs_remove_fsid calls __btrfs_sysfs_remove_fsid which in turn
> does kobject_del followed by kobject_put so its sequence is not exactly
> identical with your change. Presumably kobject_del is only required if
> you want to dispose of successfully registered sysfs node. This implies
> that __btrfs_sysfs_remove_fsid is actually broken when it comes to
> handling failed sysfs_add_fsid?
kobject_del() is not technically required in __btrfs_sysfs_remove_fsid()
since if kobject_put() drops the reference count to 0 and kobject_del()
has not been called then the kobject infrastructure will call
kobject_del() for us (and we get a pr_debug() message). The code
sequence is correct although not _exactly_ written as the kobject
authors intended (I am not one of those authors, I'm just learning).
Thanks for looking at this.
Tobin
On Mon, May 13, 2019 at 01:39:10PM +1000, Tobin C. Harding wrote:
> Is it ok to send patches during the merge window?
Yes (depends on subsystem), the feedback for patches that are not fixes
could be delayed after the merge window closes.
> Applies on top of
> Linus' mainline tag: v5.1, happy to rebase if there are conflicts.
>
> While auditing kobject_init_and_add() calls throughout the kernel it was
> found that btrfs potentially has a couple of memleaks in the error path
> code for kobject_init_and_add().
>
> Failing calls to kobject_init_and_add() should be followed by a call to
> kobject_put() since kobject_init_and_add() always calls kobject_init().
>
> Of note, adding kobject_put() causes the release method to be called if
> kobject_init_and_add() fails. For patch #1 this means we don't have to
> manually free the space_info or call percpu_counter_destroy() since
> these are both done by the release method. In the second patch, I
> believe the added call to kobject_put() fits in with the fs_devices
> lifecycle assumptions of open_ctree() but please could you review since
> I am new to this code.
We use the cleanup-after-error pattern where it's up to the callee to
clean up, so it's right to do it like as you did. Patches added to the
queue that's for 5.2-rcX. Thanks.