Currently we use custom logic to track kobject initialization. Recently
a predicate function was added to the kobject API so we now no longer
need to do this.
Use kobject API to check for initialized state of kobjects instead of
using custom logic to track state.
Signed-off-by: Tobin C. Harding <[email protected]>
---
include/linux/livepatch.h | 6 ------
kernel/livepatch/core.c | 18 +++++-------------
2 files changed, 5 insertions(+), 19 deletions(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 53551f470722..955d46f37b72 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -47,7 +47,6 @@
* @stack_node: list node for klp_ops func_stack list
* @old_size: size of the old function
* @new_size: size of the new function
- * @kobj_added: @kobj has been added and needs freeing
* @nop: temporary patch to use the original code again; dyn. allocated
* @patched: the func has been added to the klp_ops list
* @transition: the func is currently being applied or reverted
@@ -86,7 +85,6 @@ struct klp_func {
struct list_head node;
struct list_head stack_node;
unsigned long old_size, new_size;
- bool kobj_added;
bool nop;
bool patched;
bool transition;
@@ -126,7 +124,6 @@ struct klp_callbacks {
* @node: list node for klp_patch obj_list
* @mod: kernel module associated with the patched object
* (NULL for vmlinux)
- * @kobj_added: @kobj has been added and needs freeing
* @dynamic: temporary object for nop functions; dynamically allocated
* @patched: the object's funcs have been added to the klp_ops list
*/
@@ -141,7 +138,6 @@ struct klp_object {
struct list_head func_list;
struct list_head node;
struct module *mod;
- bool kobj_added;
bool dynamic;
bool patched;
};
@@ -154,7 +150,6 @@ struct klp_object {
* @list: list node for global list of actively used patches
* @kobj: kobject for sysfs resources
* @obj_list: dynamic list of the object entries
- * @kobj_added: @kobj has been added and needs freeing
* @enabled: the patch is enabled (but operation may be incomplete)
* @forced: was involved in a forced transition
* @free_work: patch cleanup from workqueue-context
@@ -170,7 +165,6 @@ struct klp_patch {
struct list_head list;
struct kobject kobj;
struct list_head obj_list;
- bool kobj_added;
bool enabled;
bool forced;
struct work_struct free_work;
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 98295de2172b..0b94aa5b38c9 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -590,7 +590,7 @@ static void __klp_free_funcs(struct klp_object *obj, bool nops_only)
list_del(&func->node);
/* Might be called from klp_init_patch() error path. */
- if (func->kobj_added) {
+ if (kobject_is_initialized(&func->kobj)) {
kobject_put(&func->kobj);
} else if (func->nop) {
klp_free_func_nop(func);
@@ -626,7 +626,7 @@ static void __klp_free_objects(struct klp_patch *patch, bool nops_only)
list_del(&obj->node);
/* Might be called from klp_init_patch() error path. */
- if (obj->kobj_added) {
+ if (kobject_is_initialized(&obj->kobj)) {
kobject_put(&obj->kobj);
} else if (obj->dynamic) {
klp_free_object_dynamic(obj);
@@ -675,7 +675,7 @@ static void klp_free_patch_finish(struct klp_patch *patch)
* this is called when the patch gets disabled and it
* cannot get enabled again.
*/
- if (patch->kobj_added) {
+ if (kobject_is_initialized(&patch->kobj)) {
kobject_put(&patch->kobj);
wait_for_completion(&patch->finish);
}
@@ -729,8 +729,6 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
func->old_sympos ? func->old_sympos : 1);
if (ret)
kobject_put(&func->kobj);
- else
- func->kobj_added = true;
return ret;
}
@@ -809,7 +807,6 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
kobject_put(&obj->kobj);
return ret;
}
- obj->kobj_added = true;
klp_for_each_func(obj, func) {
ret = klp_init_func(obj, func);
@@ -833,7 +830,6 @@ static int klp_init_patch_early(struct klp_patch *patch)
INIT_LIST_HEAD(&patch->list);
INIT_LIST_HEAD(&patch->obj_list);
- patch->kobj_added = false;
patch->enabled = false;
patch->forced = false;
INIT_WORK(&patch->free_work, klp_free_patch_work_fn);
@@ -844,13 +840,10 @@ static int klp_init_patch_early(struct klp_patch *patch)
return -EINVAL;
INIT_LIST_HEAD(&obj->func_list);
- obj->kobj_added = false;
list_add_tail(&obj->node, &patch->obj_list);
- klp_for_each_func_static(obj, func) {
- func->kobj_added = false;
+ klp_for_each_func_static(obj, func)
list_add_tail(&func->node, &obj->func_list);
- }
}
if (!try_module_get(patch->mod))
@@ -870,7 +863,6 @@ static int klp_init_patch(struct klp_patch *patch)
kobject_put(&patch->kobj);
return ret;
}
- patch->kobj_added = true;
if (patch->replace) {
ret = klp_add_nops(patch);
@@ -932,7 +924,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
if (WARN_ON(patch->enabled))
return -EINVAL;
- if (!patch->kobj_added)
+ if (kobject_is_initialized(&patch->kobj))
return -EINVAL;
pr_notice("enabling patch '%s'\n", patch->mod->name);
--
2.21.0
On Thu, May 02, 2019 at 12:31:42PM +1000, Tobin C. Harding wrote:
> Currently we use custom logic to track kobject initialization. Recently
> a predicate function was added to the kobject API so we now no longer
> need to do this.
>
> Use kobject API to check for initialized state of kobjects instead of
> using custom logic to track state.
>
> Signed-off-by: Tobin C. Harding <[email protected]>
> ---
> include/linux/livepatch.h | 6 ------
> kernel/livepatch/core.c | 18 +++++-------------
> 2 files changed, 5 insertions(+), 19 deletions(-)
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 53551f470722..955d46f37b72 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -47,7 +47,6 @@
> * @stack_node: list node for klp_ops func_stack list
> * @old_size: size of the old function
> * @new_size: size of the new function
> - * @kobj_added: @kobj has been added and needs freeing
> * @nop: temporary patch to use the original code again; dyn. allocated
> * @patched: the func has been added to the klp_ops list
> * @transition: the func is currently being applied or reverted
> @@ -86,7 +85,6 @@ struct klp_func {
> struct list_head node;
> struct list_head stack_node;
> unsigned long old_size, new_size;
> - bool kobj_added;
> bool nop;
> bool patched;
> bool transition;
> @@ -126,7 +124,6 @@ struct klp_callbacks {
> * @node: list node for klp_patch obj_list
> * @mod: kernel module associated with the patched object
> * (NULL for vmlinux)
> - * @kobj_added: @kobj has been added and needs freeing
> * @dynamic: temporary object for nop functions; dynamically allocated
> * @patched: the object's funcs have been added to the klp_ops list
> */
> @@ -141,7 +138,6 @@ struct klp_object {
> struct list_head func_list;
> struct list_head node;
> struct module *mod;
> - bool kobj_added;
> bool dynamic;
> bool patched;
> };
> @@ -154,7 +150,6 @@ struct klp_object {
> * @list: list node for global list of actively used patches
> * @kobj: kobject for sysfs resources
> * @obj_list: dynamic list of the object entries
> - * @kobj_added: @kobj has been added and needs freeing
> * @enabled: the patch is enabled (but operation may be incomplete)
> * @forced: was involved in a forced transition
> * @free_work: patch cleanup from workqueue-context
> @@ -170,7 +165,6 @@ struct klp_patch {
> struct list_head list;
> struct kobject kobj;
> struct list_head obj_list;
> - bool kobj_added;
> bool enabled;
> bool forced;
> struct work_struct free_work;
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 98295de2172b..0b94aa5b38c9 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -590,7 +590,7 @@ static void __klp_free_funcs(struct klp_object *obj, bool nops_only)
> list_del(&func->node);
>
> /* Might be called from klp_init_patch() error path. */
> - if (func->kobj_added) {
> + if (kobject_is_initialized(&func->kobj)) {
> kobject_put(&func->kobj);
> } else if (func->nop) {
> klp_free_func_nop(func);
This feels really odd to me, why do we need to keep track of this type
of thing?
How can the kobject _not_ be initialized? If it's just because we don't
want to write two separate error cleanup paths, that's not ok.
> @@ -626,7 +626,7 @@ static void __klp_free_objects(struct klp_patch *patch, bool nops_only)
> list_del(&obj->node);
>
> /* Might be called from klp_init_patch() error path. */
> - if (obj->kobj_added) {
> + if (kobject_is_initialized(&obj->kobj)) {
> kobject_put(&obj->kobj);
> } else if (obj->dynamic) {
> klp_free_object_dynamic(obj);
Same here, let's not be lazy.
The code should "know" if the kobject has been initialized or not
because it is the entity that asked for it to be initialized. Don't add
extra logic to the kobject core (like the patch before this did) just
because this one subsystem wanted to only write 1 "cleanup" function.
thanks,
greg k-h
On Thu 2019-05-02 09:12:32, Greg Kroah-Hartman wrote:
> On Thu, May 02, 2019 at 12:31:42PM +1000, Tobin C. Harding wrote:
> > Currently we use custom logic to track kobject initialization. Recently
> > a predicate function was added to the kobject API so we now no longer
> > need to do this.
> >
> > Use kobject API to check for initialized state of kobjects instead of
> > using custom logic to track state.
> >
> > Signed-off-by: Tobin C. Harding <[email protected]>
> > ---
> > include/linux/livepatch.h | 6 ------
> > kernel/livepatch/core.c | 18 +++++-------------
> > 2 files changed, 5 insertions(+), 19 deletions(-)
> >
> > @@ -626,7 +626,7 @@ static void __klp_free_objects(struct klp_patch *patch, bool nops_only)
> > list_del(&obj->node);
> >
> > /* Might be called from klp_init_patch() error path. */
> > - if (obj->kobj_added) {
> > + if (kobject_is_initialized(&obj->kobj)) {
> > kobject_put(&obj->kobj);
> > } else if (obj->dynamic) {
> > klp_free_object_dynamic(obj);
>
> Same here, let's not be lazy.
>
> The code should "know" if the kobject has been initialized or not
> because it is the entity that asked for it to be initialized. Don't add
> extra logic to the kobject core (like the patch before this did) just
> because this one subsystem wanted to only write 1 "cleanup" function.
We use kobject for a mix of statically and dynamically defined
structures[*]. And we misunderstood the behavior of kobject_init().
Anyway, the right solution is to call kobject_init()
already in klp_init_patch_early() for the statically
defined structures and in klp_alloc*() for the dynamically
allocated ones. Then we could simply call kobject_put()
every time.
Tobin, this goes deeper into the livepatching code that
you probably expected. Do you want to do the above
suggested change or should I prepare the patch?
Anyway, thanks for working on this.
[*] Yes, we know that kobject was not designed for
static structures. We even tried to use them but
there was a lot of extra code with not a big benefit.
Best Regards,
Petr
On Thu, May 02, 2019 at 09:30:44AM +0200, Petr Mladek wrote:
> On Thu 2019-05-02 09:12:32, Greg Kroah-Hartman wrote:
> > On Thu, May 02, 2019 at 12:31:42PM +1000, Tobin C. Harding wrote:
> > > Currently we use custom logic to track kobject initialization. Recently
> > > a predicate function was added to the kobject API so we now no longer
> > > need to do this.
> > >
> > > Use kobject API to check for initialized state of kobjects instead of
> > > using custom logic to track state.
> > >
> > > Signed-off-by: Tobin C. Harding <[email protected]>
> > > ---
> > > include/linux/livepatch.h | 6 ------
> > > kernel/livepatch/core.c | 18 +++++-------------
> > > 2 files changed, 5 insertions(+), 19 deletions(-)
> > >
> > > @@ -626,7 +626,7 @@ static void __klp_free_objects(struct klp_patch *patch, bool nops_only)
> > > list_del(&obj->node);
> > >
> > > /* Might be called from klp_init_patch() error path. */
> > > - if (obj->kobj_added) {
> > > + if (kobject_is_initialized(&obj->kobj)) {
> > > kobject_put(&obj->kobj);
> > > } else if (obj->dynamic) {
> > > klp_free_object_dynamic(obj);
> >
> > Same here, let's not be lazy.
> >
> > The code should "know" if the kobject has been initialized or not
> > because it is the entity that asked for it to be initialized. Don't add
> > extra logic to the kobject core (like the patch before this did) just
> > because this one subsystem wanted to only write 1 "cleanup" function.
>
> We use kobject for a mix of statically and dynamically defined
> structures[*]. And we misunderstood the behavior of kobject_init().
Eeek, no, a kobject should never be used for a static structure, that's
just wrong.
Well, almost wrong, ignore how the driver core itself does this in
places :)
thanks,
greg k-h
On Thu, May 02, 2019 at 09:30:44AM +0200, Petr Mladek wrote:
> On Thu 2019-05-02 09:12:32, Greg Kroah-Hartman wrote:
> > On Thu, May 02, 2019 at 12:31:42PM +1000, Tobin C. Harding wrote:
> > > Currently we use custom logic to track kobject initialization. Recently
> > > a predicate function was added to the kobject API so we now no longer
> > > need to do this.
> > >
> > > Use kobject API to check for initialized state of kobjects instead of
> > > using custom logic to track state.
> > >
> > > Signed-off-by: Tobin C. Harding <[email protected]>
> > > ---
> > > include/linux/livepatch.h | 6 ------
> > > kernel/livepatch/core.c | 18 +++++-------------
> > > 2 files changed, 5 insertions(+), 19 deletions(-)
> > >
> > > @@ -626,7 +626,7 @@ static void __klp_free_objects(struct klp_patch *patch, bool nops_only)
> > > list_del(&obj->node);
> > >
> > > /* Might be called from klp_init_patch() error path. */
> > > - if (obj->kobj_added) {
> > > + if (kobject_is_initialized(&obj->kobj)) {
> > > kobject_put(&obj->kobj);
> > > } else if (obj->dynamic) {
> > > klp_free_object_dynamic(obj);
> >
> > Same here, let's not be lazy.
> >
> > The code should "know" if the kobject has been initialized or not
> > because it is the entity that asked for it to be initialized. Don't add
> > extra logic to the kobject core (like the patch before this did) just
> > because this one subsystem wanted to only write 1 "cleanup" function.
>
> We use kobject for a mix of statically and dynamically defined
> structures[*]. And we misunderstood the behavior of kobject_init().
>
> Anyway, the right solution is to call kobject_init()
> already in klp_init_patch_early() for the statically
> defined structures and in klp_alloc*() for the dynamically
> allocated ones. Then we could simply call kobject_put()
> every time.
>
> Tobin, this goes deeper into the livepatching code that
> you probably expected. Do you want to do the above
> suggested change or should I prepare the patch?
I'd love for you to handle this one Petr, I'd say its a net gain
time wise that way since if I do it you'll have to review it too
carefully anyways.
So that will mean patch #1 and #5 of this series are dropped and handed
off to you (thanks). Patch #2 and #3 Greg said he will take. Patch #4
is not needed. That's a win in my books :)
Thanks,
Tobin.
On Thu 2019-05-02 18:31:27, Tobin C. Harding wrote:
> On Thu, May 02, 2019 at 09:30:44AM +0200, Petr Mladek wrote:
> > On Thu 2019-05-02 09:12:32, Greg Kroah-Hartman wrote:
> > > On Thu, May 02, 2019 at 12:31:42PM +1000, Tobin C. Harding wrote:
> > > > Currently we use custom logic to track kobject initialization. Recently
> > > > a predicate function was added to the kobject API so we now no longer
> > > > need to do this.
> > > >
> > > > Use kobject API to check for initialized state of kobjects instead of
> > > > using custom logic to track state.
> > > >
> > > > Signed-off-by: Tobin C. Harding <[email protected]>
> > > > ---
> > > > include/linux/livepatch.h | 6 ------
> > > > kernel/livepatch/core.c | 18 +++++-------------
> > > > 2 files changed, 5 insertions(+), 19 deletions(-)
> > > >
> > > > @@ -626,7 +626,7 @@ static void __klp_free_objects(struct klp_patch *patch, bool nops_only)
> > > > list_del(&obj->node);
> > > >
> > > > /* Might be called from klp_init_patch() error path. */
> > > > - if (obj->kobj_added) {
> > > > + if (kobject_is_initialized(&obj->kobj)) {
> > > > kobject_put(&obj->kobj);
> > > > } else if (obj->dynamic) {
> > > > klp_free_object_dynamic(obj);
> > >
> > > Same here, let's not be lazy.
> > >
> > > The code should "know" if the kobject has been initialized or not
> > > because it is the entity that asked for it to be initialized. Don't add
> > > extra logic to the kobject core (like the patch before this did) just
> > > because this one subsystem wanted to only write 1 "cleanup" function.
> >
> > We use kobject for a mix of statically and dynamically defined
> > structures[*]. And we misunderstood the behavior of kobject_init().
> >
> > Anyway, the right solution is to call kobject_init()
> > already in klp_init_patch_early() for the statically
> > defined structures and in klp_alloc*() for the dynamically
> > allocated ones. Then we could simply call kobject_put()
> > every time.
> >
> > Tobin, this goes deeper into the livepatching code that
> > you probably expected. Do you want to do the above
> > suggested change or should I prepare the patch?
>
> I'd love for you to handle this one Petr, I'd say its a net gain
> time wise that way since if I do it you'll have to review it too
> carefully anyways.
>
> So that will mean patch #1 and #5 of this series are dropped and handed
> off to you (thanks). Patch #2 and #3 Greg said he will take. Patch #4
> is not needed. That's a win in my books :)
Sound like a great plan. I am going to work on the patch for
the livepatching code.
Anyway, thanks a lot for your patches. It is a big relief to realize
that we could remove some hacks and do it clearly, modulo the static
structures ;-)
Best Regards,
Petr