On Tue 2021-12-14 15:51:28, Josh Poimboeuf wrote:
> On Tue, Dec 14, 2021 at 02:01:26PM -0800, David Vernet wrote:
> > When enabling a klp patch with klp_enable_patch(), klp_init_patch_early()
> > is invoked to initialize the kobjects for the patch itself, as well as the
> > 'struct klp_object' and 'struct klp_func' objects that comprise it.
> > However, there are some error paths in klp_enable_patch() where some
> > kobjects may have been initialized with kobject_init(), but an error code
> > is still returned due to e.g. a 'struct klp_object' having a NULL funcs
> > pointer.
> >
> > In these paths, the kobject of the 'struct klp_patch' may be leaked, along
> > with one or more of its objects and their functions, as kobject_put() is
> > not invoked on the cleanup path if klp_init_patch_early() returns an error
> > code.
> >
> > For example, if an object entry such as the following were added to the
> > sample livepatch module's klp patch, it would cause the vmlinux klp_object,
> > and its klp_func which updates 'cmdline_proc_show', to be leaked:
> >
> > static struct klp_object objs[] = {
> > {
> > /* name being NULL means vmlinux */
> > .funcs = funcs,
> > },
> > {
> > .name = "kvm",
> > /* NULL funcs -- would cause leak */
I see in the subject and the commit message:
"Fix leak"
"may be leaked"
"to be leaked"
"would cause leak"
But the discussion suggests that nobody sees any real leak. I would
like to make this clear in the commit message.
Well, I still believe that this is just a cargo cult. And I would prefer
to finish the discussion about it, first, see
https://lore.kernel.org/all/YbmlL0ZyfSuek9OB@alley/
> Though, klp_init_patch_early() still has a failure mode which looks a
> little sketchy:
>
> klp_for_each_object_static(patch, obj) {
> if (!obj->funcs)
> return -EINVAL;
>
> klp_init_object_early(patch, obj);
>
> klp_for_each_func_static(obj, func) {
> klp_init_func_early(obj, func);
Note that klp_init_*_early() functions iterate through the arrays
using klp_for_each_*_static. While klp_free_*() functions iterate
via the lists using klp_for_each_*_safe().
> }
> }
>
>
> While I don't see any actual leaks associated with it, it'd be cleaner
> and more robust to move the per-object !obj->funcs check to the top of
> klp_enable_patch(), with the other EINVAL checks. Like:
>
>
> int klp_enable_patch(struct klp_patch *patch)
> {
> struct klp_object *obj;
> int ret;
>
> if (!patch || !patch->mod || !patch->objs)
> return -EINVAL;
>
> klp_for_each_object_static(patch, obj) {
> if (!obj->funcs)
> return -EINVAL;
> }
We should not need the pre-early-init check when the lists include only
structures with initialized kobjects.
Otherwise, I like the idea to do module_get() before
klp_init_patch_early(). I was never happy with the "hidden"
side effect.
I am also fine with calling klp_free() when the early init fails
if we agreed that it is a good practice. I just do want to pretend
that it fixes a leak what nobody sees any leak.
Please, wait few days until the discussion finishes before sending v3.
Best Regards,
Petr
