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 <to...@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.