lkml.org 
[lkml]   [2021]   [Dec]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] livepatch: Fix leak on klp_init_patch_early failure path
On Wed 2021-12-15 07:20:04, David Vernet wrote:
> Petr Mladek <pmladek@suse.com> wrote on Wed [2021-Dec-15 11:06:15 +0100]:
> > 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/
>
> No problem, I won't send out v3 until we've finished the discussion and
> have consensus. I'll assume that the discussion on whether or not there is
> a leak will continue on the thread you linked to above, so I won't comment
> on it here.
>
> > 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().
>
> Correct, as I've understood it, klp_for_each_*_safe() should only iterate
> over the objects that have been added to the patch and klp_object's lists,
> and thus for which kobject_init() has been invoked. So if we fail a check
> on 'struct klp_object' N, then we'll only iterate over the first N - 1
> objects in klp_for_each_*_safe().
>
> > We should not need the pre-early-init check when the lists include only
> > structures with initialized kobjects.
>
> Not sure I quite follow. We have to do NULL checks for obj->funcs at some
> point, and per Josh's suggestion it seems cleaner to do it outside the
> critical section, and before we actually invoke kobject_init(). Apologies
> if I've misunderstood your point.

The original purpose of klp_init_*_early() was to allow calling
klp_free_patch_*() when klp_init_*() fails. The idea was to
initialize all fields properly so that free functions would
do the right thing.

Josh's proposal adds pre-early-init() to allow calling
klp_free_patch_*() already when klp_init_*_early() fails.
The purpose is to make sure that klp_init_*_early()
will actually never fail.

This might make things somehow complicated. Any future change
in klp_init_*_early() might require change in pre-early-init()
to catch eventual problems earlier.

Also I am not sure what to do with the existing checks
in klp_init_patch_early(). I am uneasy with removing them
and hoping that pre-early-init() did the right job.
But if we keep the checks then klp_init_patch_early() then it
might fail and the code will not be ready for this.


My proposal is to use simple trick. klp_free_patch_*() iterate
using the dynamic list (list_for_each_entry) while klp_init_*_early()
iterate using the arrays.

Now, we just need to make sure that klp_init_*_early() will only add
fully initialized structures into the dynamic list. As a result,
klp_free_patch() will see only the fully initialized structures
and could release them. It will not see that not-yet-initialized
structures but it is fine. They are not initialized and they do not
need to be freed.

In fact, I think that klp_init_*_early() functions already do
the right thing. IMHO, if you move the module_get() then you
could safely do:

int klp_enable_patch(struct klp_patch *patch)
{
[...]
if (!try_module_get(patch->mod)) {
mutex_unlock(&klp_mutex);
return -ENODEV;
}

ret = klp_init_patch_early(patch);
if (ret)
goto err;


Note that it would need to get tested.

Anyway, does it make sense now?

Best Regards,
Petr

\
 
 \ /
  Last update: 2021-12-17 14:52    [W:1.000 / U:0.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site