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 */
}, { }
};
Without this change, if CONFIG_DEBUG_KOBJECT is enabled, and the sample klp
patch is loaded, the kobjects (the patch, the vmlinux 'struct klp_obj', and
its func) are not observed to be released in the dmesg log output. With
the change, the kobjects are observed to be released.
Signed-off-by: David Vernet <[email protected]>
---
v2:
- Move try_module_get() and the patch->objs NULL check out of
klp_init_patch_early() to ensure that it's safe to jump to the 'err' label
on the error path in klp_enable_patch().
- Fix the patch description to not use markdown, and to use imperative
language.
kernel/livepatch/core.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 335d988bd811..98c2b0d02770 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -867,9 +867,6 @@ static int klp_init_patch_early(struct klp_patch *patch)
struct klp_object *obj;
struct klp_func *func;
- if (!patch->objs)
- return -EINVAL;
-
INIT_LIST_HEAD(&patch->list);
INIT_LIST_HEAD(&patch->obj_list);
kobject_init(&patch->kobj, &klp_ktype_patch);
@@ -889,9 +886,6 @@ static int klp_init_patch_early(struct klp_patch *patch)
}
}
- if (!try_module_get(patch->mod))
- return -ENODEV;
-
return 0;
}
@@ -1025,7 +1019,7 @@ int klp_enable_patch(struct klp_patch *patch)
{
int ret;
- if (!patch || !patch->mod)
+ if (!patch || !patch->mod || !patch->objs)
return -EINVAL;
if (!is_livepatch_module(patch->mod)) {
@@ -1051,11 +1045,12 @@ int klp_enable_patch(struct klp_patch *patch)
return -EINVAL;
}
+ if (!try_module_get(patch->mod))
+ return -ENODEV;
+
ret = klp_init_patch_early(patch);
- if (ret) {
- mutex_unlock(&klp_mutex);
- return ret;
- }
+ if (ret)
+ goto err;
ret = klp_init_patch(patch);
if (ret)
--
2.30.2
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 */
> }, { }
> };
>
> Without this change, if CONFIG_DEBUG_KOBJECT is enabled, and the sample klp
> patch is loaded, the kobjects (the patch, the vmlinux 'struct klp_obj', and
> its func) are not observed to be released in the dmesg log output. With
> the change, the kobjects are observed to be released.
This looks much better, thanks.
> Signed-off-by: David Vernet <[email protected]>
> ---
> v2:
> - Move try_module_get() and the patch->objs NULL check out of
> klp_init_patch_early() to ensure that it's safe to jump to the 'err' label
> on the error path in klp_enable_patch().
> - Fix the patch description to not use markdown, and to use imperative
> language.
Looking good overall.
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);
}
}
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;
}
Then klp_init_patch_early() can be changed to return void, and we can
more easily convince ourselves there aren't any remaining leaks.
--
Josh
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
Petr Mladek <[email protected]> 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.
> Otherwise, I like the idea to do module_get() before
> klp_init_patch_early(). I was never happy with the "hidden"
> side effect.
Ack!
> 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.
Ack, no problem, I'll wait until we're all in alignment. Thanks, Petr and
Josh for taking a look at the patch.
Regards,
David
On Wed 2021-12-15 07:20:04, David Vernet wrote:
> Petr Mladek <[email protected]> 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
On Fri, Dec 17, 2021 at 02:51:42PM +0100, Petr Mladek wrote:
> On Wed 2021-12-15 07:20:04, David Vernet wrote:
> > Petr Mladek <[email protected]> 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.
I'm not sure why that would be a problem. If we can separate out the
things which fail from the things which don't, it simplifies things.
And if klp_init_object_early() returns void then it would make sense for
klp_init_patch_early() to also return void.
> 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.
klp_init_patch_early() already depends on some of the other checks in
klp_enable_patch() anyway. I don't see that as much of a problem since
it only has one caller.
The benefit of moving the rest of the checks out is that it simplifies
the error handling, with no possibility of half-baked structures.
> 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?
It would work, but it's too clever/non-obvious. If we rely on tricks
then we may eventually forget about them and accidentally break
assumptions later.
--
Josh
On Fri 2021-12-17 13:50:42, Josh Poimboeuf wrote:
> On Fri, Dec 17, 2021 at 02:51:42PM +0100, Petr Mladek wrote:
> > On Wed 2021-12-15 07:20:04, David Vernet wrote:
> > 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.
>
> I'm not sure why that would be a problem. If we can separate out the
> things which fail from the things which don't, it simplifies things.
I think that there is no right answer. It depends on personal
preferences.
> > 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?
>
> It would work, but it's too clever/non-obvious. If we rely on tricks
> then we may eventually forget about them and accidentally break
> assumptions later.
It is not that super magic from my POV. But again, it is a personal
taste.
I do not want to bikeshed about it. Feel free to use the pre-early
thing.
Best Regards,
Petr