lkml.org 
[lkml]   [2019]   [May]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [RFC PATCH] kobject: Clean up allocated memory on failure
On Thu, May 16, 2019 at 08:40:29AM +0200, Greg Kroah-Hartman wrote:
> On Thu, May 16, 2019 at 10:07:16AM +1000, Tobin C. Harding wrote:
> > Currently kobject_add_varg() calls kobject_set_name_vargs() then returns
> > the return value of kobject_add_internal(). kobject_set_name_vargs()
> > allocates memory for the name string. When kobject_add_varg() returns
> > an error we do not know if memory was allocated or not. If we check the
> > return value of kobject_add_internal() instead of returning it directly
> > we can free the allocated memory if kobject_add_internal() fails. Doing
> > this means that we now know that if kobject_add_varg() fails we do not
> > have to do any clean up, this benefit goes back up the call chain
> > meaning that we now do not need to do any cleanup if kobject_del()
> > fails. Moving further back (in a theoretical kobject user callchain)
> > this means we now no longer need to call kobject_put() after calling
> > kobject_init_and_add(), we can just call kfree() on the enclosing
> > structure. This makes the kobject API better follow the principle of
> > least surprise.
> >
> > Check return value of kobject_add_internal() and free previously
> > allocated memory on failure.
> >
> > Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> > ---
> >
> > Hi Greg,
> >
> > Pretty excited by this one, if this is correct it means that kobject
> > initialisation code, in the error path, can now use either kobject_put()
> > (to trigger the release method) OR kfree(). This means most of the
> > call sites of kobject_init_and_add() will get fixed for free!
> >
> > I've been wrong before so I'll state here that this is based on the
> > assumption that kobject_init() does nothing that causes leaked memory.
> > This is _not_ what the function docs in kobject.c say but it _is_ what
> > the code seems to say since kobject_init() does nothing except
> > initialise kobject data member values? Or have I got the dog by the
> > tail?
>
> I think you are correct here. In looking at the code paths, all should
> be good and safe.
>
> But, if you use your patch, then you have to call kfree, and you can not
> call kobject_put(), otherwise kfree_const() will be called twice on the
> same pointer, right? So you will have to audit the kernel and change
> everything again :)

Oh my bad, I got so excited by this I read the 'if (name) {' in kobject
to be guarding the double call to kfree_const(), which clearly it doesn't.

> Or, maybe this patch would prevent that:
>
>
> diff --git a/lib/kobject.c b/lib/kobject.c
> index f2ccdbac8ed9..03cdec1d450a 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -387,7 +387,14 @@ static __printf(3, 0) int kobject_add_varg(struct kobject *kobj,
> return retval;
> }
> kobj->parent = parent;
> - return kobject_add_internal(kobj);
> +
> + retval = kobject_add_internal(kobj);
> + if (retval && !is_kernel_rodata((unsigned long)(kobj->name))) {
> + kfree_const(kobj->name);
> + kobj->name = NULL;
> + }
> +
> + return retval;
> }
>
> /**
>
>
> But that feels like a huge hack to me.

I agree, does the job but too ugly.

> I think, to be safe, we should
> keep the existing lifetime rules, as it mirrors what happens with
> 'struct device', and that is what people _should_ be using, not "raw"
> kobjects if at all possible.

Oh, I wasn't seeing this through the eyes of a driver developer, perhaps
I should have started in drivers/ not in fs/

> Yeah, I know filesystems don't do that, my fault, I never thought a
> filesystem would care about sysfs all those years ago :)

Tough business that, predicting the future.

Let's drop this and I'll keep plugging away.

Thanks,
Tobin.
\
 
 \ /
  Last update: 2019-05-16 14:03    [W:0.026 / U:7.120 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site