From: David Vernet <void@manifault.com> Date: 2021-12-14 22:02:21
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 <void@manifault.com>
---
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(-)
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 <void@manifault.com>
---
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
From: Petr Mladek <pmladek@suse.com> Date: 2021-12-15 10:06:18
On Tue 2021-12-14 15:51:28, Josh Poimboeuf wrote:
On Tue, Dec 14, 2021 at 02:01:26PM -0800, David Vernet wrote:
quoted
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
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
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.
quoted
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().
quoted
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
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.
quoted
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().
quoted
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
From: Petr Mladek <pmladek@suse.com> Date: 2021-12-20 09:48:49
On Fri 2021-12-17 13:50:42, Josh Poimboeuf wrote:
On Fri, Dec 17, 2021 at 02:51:42PM +0100, Petr Mladek wrote:
quoted
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.
quoted
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