The completion finish is just for waiting release of the klp_patch
object, then releases module refcnt. We can simply drop the module
refcnt in the kobject release handler of klp_patch.
This way also helps to support allocating klp_patch from heap.
Signed-off-by: Ming Lei <[email protected]>
---
include/linux/livepatch.h | 1 -
kernel/livepatch/core.c | 12 +++---------
2 files changed, 3 insertions(+), 10 deletions(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 2614247a9781..9712818997c5 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -170,7 +170,6 @@ struct klp_patch {
bool enabled;
bool forced;
struct work_struct free_work;
- struct completion finish;
};
#define klp_for_each_object_static(patch, obj) \
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 335d988bd811..b967b4b0071b 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -551,10 +551,10 @@ static int klp_add_nops(struct klp_patch *patch)
static void klp_kobj_release_patch(struct kobject *kobj)
{
- struct klp_patch *patch;
+ struct klp_patch *patch = container_of(kobj, struct klp_patch, kobj);
- patch = container_of(kobj, struct klp_patch, kobj);
- complete(&patch->finish);
+ if (!patch->forced)
+ module_put(patch->mod);
}
static struct kobj_type klp_ktype_patch = {
@@ -678,11 +678,6 @@ static void klp_free_patch_finish(struct klp_patch *patch)
* cannot get enabled again.
*/
kobject_put(&patch->kobj);
- wait_for_completion(&patch->finish);
-
- /* Put the module after the last access to struct klp_patch. */
- if (!patch->forced)
- module_put(patch->mod);
}
/*
@@ -876,7 +871,6 @@ static int klp_init_patch_early(struct klp_patch *patch)
patch->enabled = false;
patch->forced = false;
INIT_WORK(&patch->free_work, klp_free_patch_work_fn);
- init_completion(&patch->finish);
klp_for_each_object_static(patch, obj) {
if (!obj->funcs)
--
2.31.1
On Tue 2021-11-02 22:59:30, Ming Lei wrote:
> The completion finish is just for waiting release of the klp_patch
> object, then releases module refcnt. We can simply drop the module
> refcnt in the kobject release handler of klp_patch.
>
> This way also helps to support allocating klp_patch from heap.
IMHO, this is wrong assumption. kobject_put() might do everyting
asynchronously, see:
kobject_put()
kobject_release()
INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
schedule_delayed_work(&kobj->release, delay);
asynchronously:
kobject_delayed_cleanup()
kobject_cleanup()
__kobject_del()
> Signed-off-by: Ming Lei <[email protected]>
> ---
> include/linux/livepatch.h | 1 -
> kernel/livepatch/core.c | 12 +++---------
> 2 files changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 2614247a9781..9712818997c5 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -170,7 +170,6 @@ struct klp_patch {
> bool enabled;
> bool forced;
> struct work_struct free_work;
> - struct completion finish;
> };
>
> #define klp_for_each_object_static(patch, obj) \
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 335d988bd811..b967b4b0071b 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -551,10 +551,10 @@ static int klp_add_nops(struct klp_patch *patch)
>
> static void klp_kobj_release_patch(struct kobject *kobj)
> {
> - struct klp_patch *patch;
> + struct klp_patch *patch = container_of(kobj, struct klp_patch, kobj);
>
> - patch = container_of(kobj, struct klp_patch, kobj);
> - complete(&patch->finish);
> + if (!patch->forced)
> + module_put(patch->mod);
> }
>
> static struct kobj_type klp_ktype_patch = {
> @@ -678,11 +678,6 @@ static void klp_free_patch_finish(struct klp_patch *patch)
> * cannot get enabled again.
> */
> kobject_put(&patch->kobj);
> - wait_for_completion(&patch->finish);
> -
> - /* Put the module after the last access to struct klp_patch. */
> - if (!patch->forced)
> - module_put(patch->mod);
klp_free_patch_finish() does not longer wait until the release
callbacks are called.
klp_free_patch_finish() is called also in klp_enable_patch() error
path.
klp_enable_patch() is called in module_init(). For example, see
samples/livepatch/livepatch-sample.c
The module must not get removed until the release callbacks are called.
Does the module loader check the module reference counter when
module_init() fails?
Best Regards,
Petr
On Tue, Nov 02, 2021 at 04:56:10PM +0100, Petr Mladek wrote:
> On Tue 2021-11-02 22:59:30, Ming Lei wrote:
> > The completion finish is just for waiting release of the klp_patch
> > object, then releases module refcnt. We can simply drop the module
> > refcnt in the kobject release handler of klp_patch.
> >
> > This way also helps to support allocating klp_patch from heap.
>
> IMHO, this is wrong assumption. kobject_put() might do everyting
> asynchronously, see:
>
> kobject_put()
> kobject_release()
> INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
> schedule_delayed_work(&kobj->release, delay);
>
> asynchronously:
>
> kobject_delayed_cleanup()
> kobject_cleanup()
> __kobject_del()
OK, this is one generic kobject release vs. module unloading issue to
solve, not unique for klp module, and there should be lots of drivers
suffering from it.
>
>
> > Signed-off-by: Ming Lei <[email protected]>
> > ---
> > include/linux/livepatch.h | 1 -
> > kernel/livepatch/core.c | 12 +++---------
> > 2 files changed, 3 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > index 2614247a9781..9712818997c5 100644
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -170,7 +170,6 @@ struct klp_patch {
> > bool enabled;
> > bool forced;
> > struct work_struct free_work;
> > - struct completion finish;
> > };
> >
> > #define klp_for_each_object_static(patch, obj) \
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 335d988bd811..b967b4b0071b 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -551,10 +551,10 @@ static int klp_add_nops(struct klp_patch *patch)
> >
> > static void klp_kobj_release_patch(struct kobject *kobj)
> > {
> > - struct klp_patch *patch;
> > + struct klp_patch *patch = container_of(kobj, struct klp_patch, kobj);
> >
> > - patch = container_of(kobj, struct klp_patch, kobj);
> > - complete(&patch->finish);
> > + if (!patch->forced)
> > + module_put(patch->mod);
> > }
> >
> > static struct kobj_type klp_ktype_patch = {
> > @@ -678,11 +678,6 @@ static void klp_free_patch_finish(struct klp_patch *patch)
> > * cannot get enabled again.
> > */
> > kobject_put(&patch->kobj);
> > - wait_for_completion(&patch->finish);
> > -
> > - /* Put the module after the last access to struct klp_patch. */
> > - if (!patch->forced)
> > - module_put(patch->mod);
>
> klp_free_patch_finish() does not longer wait until the release
> callbacks are called.
>
> klp_free_patch_finish() is called also in klp_enable_patch() error
> path.
>
> klp_enable_patch() is called in module_init(). For example, see
> samples/livepatch/livepatch-sample.c
>
> The module must not get removed until the release callbacks are called.
> Does the module loader check the module reference counter when
> module_init() fails?
Good catch, that is really one corner case, in which the kobject has to
be cleaned up before returning from mod->init(), cause there can't be
module unloading in case of mod->init() failure.
Yeah, it should be more related with async kobject_put().
Also looks it is reasonable to add check when cleaning module loading
failure.
thanks,
Ming
On Wed 2021-11-03 08:51:32, Ming Lei wrote:
> On Tue, Nov 02, 2021 at 04:56:10PM +0100, Petr Mladek wrote:
> > On Tue 2021-11-02 22:59:30, Ming Lei wrote:
> > > The completion finish is just for waiting release of the klp_patch
> > > object, then releases module refcnt. We can simply drop the module
> > > refcnt in the kobject release handler of klp_patch.
> > >
> > > This way also helps to support allocating klp_patch from heap.
First, I am sorry for confusion. The above description is correct.
I does not say anything about that kobject_put() is synchronous.
> > IMHO, this is wrong assumption. kobject_put() might do everyting
> > asynchronously, see:
I see that you are aware of this behavior.
> > kobject_put()
> > kobject_release()
> > INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
> > schedule_delayed_work(&kobj->release, delay);
> >
> > asynchronously:
> >
> > kobject_delayed_cleanup()
> > kobject_cleanup()
> > __kobject_del()
>
> OK, this is one generic kobject release vs. module unloading issue to
> solve, not unique for klp module, and there should be lots of drivers
> suffering from it.
Yup, the problem is generic. It would be nice to have a generic
solution. For example, add kobject_release_sync() that would return
only when the object is really released.
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > > @@ -678,11 +678,6 @@ static void klp_free_patch_finish(struct klp_patch *patch)
> > > * cannot get enabled again.
> > > */
> > > kobject_put(&patch->kobj);
> > > - wait_for_completion(&patch->finish);
> > > -
> > > - /* Put the module after the last access to struct klp_patch. */
> > > - if (!patch->forced)
> > > - module_put(patch->mod);
> >
> > klp_free_patch_finish() does not longer wait until the release
> > callbacks are called.
> >
> > klp_free_patch_finish() is called also in klp_enable_patch() error
> > path.
> >
> > klp_enable_patch() is called in module_init(). For example, see
> > samples/livepatch/livepatch-sample.c
> >
> > The module must not get removed until the release callbacks are called.
> > Does the module loader check the module reference counter when
> > module_init() fails?
>
> Good catch, that is really one corner case, in which the kobject has to
> be cleaned up before returning from mod->init(), cause there can't be
> module unloading in case of mod->init() failure.
Just to be sure. We want to keep the safe behavior in this case.
There are many situations when klp_enable() might fail. And the error
handling must be safe.
In general, livepatch developers are very conservative.
Livepatches are not easy to create. They are used only by people
who really want to avoid reboot. We want to keep the livepatch kernel
framework as safe as possible to avoid any potential damage.
> Yeah, it should be more related with async kobject_put().
Yup, it would be nice to have some sychronous variant provided
by kobject API.
> Also looks it is reasonable to add check when cleaning module loading
> failure.
Which means that the completion has to stay until there is any generic
solution. And this patch should be dropped for now.
Best Regards,
Petr
On Wed, Nov 03, 2021 at 01:52:29PM +0100, Petr Mladek wrote:
> On Wed 2021-11-03 08:51:32, Ming Lei wrote:
> > On Tue, Nov 02, 2021 at 04:56:10PM +0100, Petr Mladek wrote:
> > > On Tue 2021-11-02 22:59:30, Ming Lei wrote:
> > > > The completion finish is just for waiting release of the klp_patch
> > > > object, then releases module refcnt. We can simply drop the module
> > > > refcnt in the kobject release handler of klp_patch.
> > > >
> > > > This way also helps to support allocating klp_patch from heap.
>
> First, I am sorry for confusion. The above description is correct.
> I does not say anything about that kobject_put() is synchronous.
>
> > > IMHO, this is wrong assumption. kobject_put() might do everyting
> > > asynchronously, see:
>
> I see that you are aware of this behavior.
>
> > > kobject_put()
> > > kobject_release()
> > > INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
> > > schedule_delayed_work(&kobj->release, delay);
> > >
> > > asynchronously:
> > >
> > > kobject_delayed_cleanup()
> > > kobject_cleanup()
> > > __kobject_del()
> >
> > OK, this is one generic kobject release vs. module unloading issue to
> > solve, not unique for klp module, and there should be lots of drivers
> > suffering from it.
>
> Yup, the problem is generic. It would be nice to have a generic
> solution. For example, add kobject_release_sync() that would return
> only when the object is really released.
The generic solution has been posted out:
https://lore.kernel.org/lkml/[email protected]/
which needn't any generic API change, just flushes all scheduled kobject
cleanup work before freeing module, and the change is transparent for
drivers.
IMO, kobject_release_sync() is one wrong direction for fixing this
issue, since it is basically impossible to audit if one kobject_put()
need to be replaced with kobject_release_sync().
>
> > > > --- a/kernel/livepatch/core.c
> > > > +++ b/kernel/livepatch/core.c
> > > > @@ -678,11 +678,6 @@ static void klp_free_patch_finish(struct klp_patch *patch)
> > > > * cannot get enabled again.
> > > > */
> > > > kobject_put(&patch->kobj);
> > > > - wait_for_completion(&patch->finish);
> > > > -
> > > > - /* Put the module after the last access to struct klp_patch. */
> > > > - if (!patch->forced)
> > > > - module_put(patch->mod);
> > >
> > > klp_free_patch_finish() does not longer wait until the release
> > > callbacks are called.
> > >
> > > klp_free_patch_finish() is called also in klp_enable_patch() error
> > > path.
> > >
> > > klp_enable_patch() is called in module_init(). For example, see
> > > samples/livepatch/livepatch-sample.c
> > >
> > > The module must not get removed until the release callbacks are called.
> > > Does the module loader check the module reference counter when
> > > module_init() fails?
> >
> > Good catch, that is really one corner case, in which the kobject has to
> > be cleaned up before returning from mod->init(), cause there can't be
> > module unloading in case of mod->init() failure.
>
> Just to be sure. We want to keep the safe behavior in this case.
> There are many situations when klp_enable() might fail. And the error
> handling must be safe.
>
> In general, livepatch developers are very conservative.
> Livepatches are not easy to create. They are used only by people
> who really want to avoid reboot. We want to keep the livepatch kernel
> framework as safe as possible to avoid any potential damage.
The posted patch can cover this situation in which module_init() fails.
Thanks,
Ming
On Tue, 2 Nov 2021, Ming Lei wrote:
> The completion finish is just for waiting release of the klp_patch
> object, then releases module refcnt. We can simply drop the module
> refcnt in the kobject release handler of klp_patch.
>
> This way also helps to support allocating klp_patch from heap.
>
> Signed-off-by: Ming Lei <[email protected]>
> ---
> include/linux/livepatch.h | 1 -
> kernel/livepatch/core.c | 12 +++---------
> 2 files changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 2614247a9781..9712818997c5 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -170,7 +170,6 @@ struct klp_patch {
> bool enabled;
> bool forced;
> struct work_struct free_work;
> - struct completion finish;
> };
>
> #define klp_for_each_object_static(patch, obj) \
Petr pointed out the main problem. I'll just add that if it comes to it,
you could also remove the inclusion of completion.h header file and a
description of finish struct member.
Miroslav
On Fri 2021-11-05 20:04:03, Ming Lei wrote:
> On Wed, Nov 03, 2021 at 01:52:29PM +0100, Petr Mladek wrote:
> > On Wed 2021-11-03 08:51:32, Ming Lei wrote:
> > > On Tue, Nov 02, 2021 at 04:56:10PM +0100, Petr Mladek wrote:
> > I see that you are aware of this behavior.
> >
> > > > kobject_put()
> > > > kobject_release()
> > > > INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
> > > > schedule_delayed_work(&kobj->release, delay);
> > > >
> > > > asynchronously:
> > > >
> > > > kobject_delayed_cleanup()
> > > > kobject_cleanup()
> > > > __kobject_del()
> > >
> > > OK, this is one generic kobject release vs. module unloading issue to
> > > solve, not unique for klp module, and there should be lots of drivers
> > > suffering from it.
> >
> > Yup, the problem is generic. It would be nice to have a generic
> > solution. For example, add kobject_release_sync() that would return
> > only when the object is really released.
>
> The generic solution has been posted out:
>
> https://lore.kernel.org/lkml/[email protected]/
>
> which needn't any generic API change, just flushes all scheduled kobject
> cleanup work before freeing module, and the change is transparent for
> drivers.
No, it is not enough. The proposed "generic solution" solves only
the problem introduced by CONFIG_DEBUG_KOBJECT_RELEASE. It does not
prevent unloading the module from another reasons. I mean that it
does not help when the last reference is not released in time
or never from some reasons.
> IMO, kobject_release_sync() is one wrong direction for fixing this
> issue, since it is basically impossible to audit if one kobject_put()
> need to be replaced with kobject_release_sync().
>
> >
> > > > > --- a/kernel/livepatch/core.c
> > > > > +++ b/kernel/livepatch/core.c
> > > > > @@ -678,11 +678,6 @@ static void klp_free_patch_finish(struct klp_patch *patch)
> > > > > * cannot get enabled again.
> > > > > */
> > > > > kobject_put(&patch->kobj);
> > > > > - wait_for_completion(&patch->finish);
> > > > > -
> > > > > - /* Put the module after the last access to struct klp_patch. */
> > > > > - if (!patch->forced)
> > > > > - module_put(patch->mod);
> > > >
> > > > klp_free_patch_finish() does not longer wait until the release
> > > > callbacks are called.
> > > >
> > > > klp_free_patch_finish() is called also in klp_enable_patch() error
> > > > path.
> > > >
> > > > klp_enable_patch() is called in module_init(). For example, see
> > > > samples/livepatch/livepatch-sample.c
> > > >
> > > > The module must not get removed until the release callbacks are called.
> > > > Does the module loader check the module reference counter when
> > > > module_init() fails?
> > >
> > > Good catch, that is really one corner case, in which the kobject has to
> > > be cleaned up before returning from mod->init(), cause there can't be
> > > module unloading in case of mod->init() failure.
> >
> > Just to be sure. We want to keep the safe behavior in this case.
> > There are many situations when klp_enable() might fail. And the error
> > handling must be safe.
> >
> > In general, livepatch developers are very conservative.
> > Livepatches are not easy to create. They are used only by people
> > who really want to avoid reboot. We want to keep the livepatch kernel
> > framework as safe as possible to avoid any potential damage.
>
> The posted patch can cover this situation in which module_init() fails.
No, it works only when the last reference was dropped, the delayed
release queued, and the kobject added into kobj_cleanup_list.
The current livepatch code is much more safe. klp_free_patch_finish()
waits for the completion. It will allows to remove the module only
when the kobject was really released. It will block the module
removal as long as needed, even forever, if there is a bug somewhere.
If we use wait_for_completion_timeout() in a while cycle, we could
even report when it takes suspiciously long and something likely
went wrong. This is one way to "always" catch and report the problem.
I am not sure if similar approach is usable for device drivers. But I
had this in mind when I proposed the kobject_release_sync() API.
Best Regards,
Petr