From: Miroslav Benes <[email protected]>
Make kobj variable (of type struct kobject) statically allocated in
klp_object structure. It will allow us to move in the func-object-patch
hierarchy through kobject links.
The only reason to have it dynamic was to not have empty release
callback in the code. However we have empty callbacks for function and
patch in the code now, so it is no longer valid and the advantage of
static allocation is clear.
Signed-off-by: Miroslav Benes <[email protected]>
Signed-off-by: Jiri Slaby <[email protected]>
---
include/linux/livepatch.h | 2 +-
kernel/livepatch/core.c | 22 ++++++++++++++++------
2 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index ee6dbb39a809..fe45f2f02c8d 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -99,7 +99,7 @@ struct klp_object {
struct klp_func *funcs;
/* internal */
- struct kobject *kobj;
+ struct kobject kobj;
struct module *mod;
enum klp_state state;
};
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index c0ae3d80e6c0..e42654d72eba 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -652,6 +652,15 @@ static struct kobj_type klp_ktype_patch = {
.default_attrs = klp_patch_attrs,
};
+static void klp_kobj_release_object(struct kobject *kobj)
+{
+}
+
+static struct kobj_type klp_ktype_object = {
+ .release = klp_kobj_release_object,
+ .sysfs_ops = &kobj_sysfs_ops,
+};
+
static void klp_kobj_release_func(struct kobject *kobj)
{
}
@@ -696,7 +705,7 @@ static void klp_free_objects_limited(struct klp_patch *patch,
for (obj = patch->objs; obj->funcs && obj != limit; obj++) {
klp_free_funcs_limited(obj, NULL);
- kobject_put(obj->kobj);
+ kobject_put(&obj->kobj);
}
}
@@ -714,7 +723,7 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
func->state = KLP_DISABLED;
return kobject_init_and_add(&func->kobj, &klp_ktype_func,
- obj->kobj, "%s", func->old_name);
+ &obj->kobj, "%s", func->old_name);
}
/* parts of the initialization that is done only when the object is loaded */
@@ -754,9 +763,10 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
klp_find_object_module(obj);
name = klp_is_module(obj) ? obj->name : "vmlinux";
- obj->kobj = kobject_create_and_add(name, &patch->kobj);
- if (!obj->kobj)
- return -ENOMEM;
+ ret = kobject_init_and_add(&obj->kobj, &klp_ktype_object,
+ &patch->kobj, "%s", name);
+ if (ret)
+ return ret;
for (func = obj->funcs; func->old_name; func++) {
ret = klp_init_func(obj, func);
@@ -774,7 +784,7 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
free:
klp_free_funcs_limited(obj, func);
- kobject_put(obj->kobj);
+ kobject_put(&obj->kobj);
return ret;
}
--
2.4.1
klp_for_each_object and klp_for_each_func are now used all over the
code. One need not think what is the proper condition to check in the
for loop now.
Signed-off-by: Jiri Slaby <[email protected]>
---
include/linux/livepatch.h | 6 ++++++
kernel/livepatch/core.c | 18 +++++++++---------
2 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index fe45f2f02c8d..31db7a05dd36 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -123,6 +123,12 @@ struct klp_patch {
enum klp_state state;
};
+#define klp_for_each_object(patch, obj) \
+ for (obj = patch->objs; obj->funcs; obj++)
+
+#define klp_for_each_func(obj, func) \
+ for (func = obj->funcs; func->old_name; func++)
+
int klp_register_patch(struct klp_patch *);
int klp_unregister_patch(struct klp_patch *);
int klp_enable_patch(struct klp_patch *);
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index e42654d72eba..a545541dcbab 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -423,7 +423,7 @@ static void klp_disable_object(struct klp_object *obj)
{
struct klp_func *func;
- for (func = obj->funcs; func->old_name; func++)
+ klp_for_each_func(obj, func)
if (func->state == KLP_ENABLED)
klp_disable_func(func);
@@ -441,7 +441,7 @@ static int klp_enable_object(struct klp_object *obj)
if (WARN_ON(!klp_is_object_loaded(obj)))
return -EINVAL;
- for (func = obj->funcs; func->old_name; func++) {
+ klp_for_each_func(obj, func) {
ret = klp_enable_func(func);
if (ret) {
klp_disable_object(obj);
@@ -464,7 +464,7 @@ static int __klp_disable_patch(struct klp_patch *patch)
pr_notice("disabling patch '%s'\n", patch->mod->name);
- for (obj = patch->objs; obj->funcs; obj++) {
+ klp_for_each_object(patch, obj) {
if (obj->state == KLP_ENABLED)
klp_disable_object(obj);
}
@@ -524,7 +524,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
pr_notice("enabling patch '%s'\n", patch->mod->name);
- for (obj = patch->objs; obj->funcs; obj++) {
+ klp_for_each_object(patch, obj) {
if (!klp_is_object_loaded(obj))
continue;
@@ -690,7 +690,7 @@ static void klp_free_object_loaded(struct klp_object *obj)
obj->mod = NULL;
- for (func = obj->funcs; func->old_name; func++)
+ klp_for_each_func(obj, func)
func->old_addr = 0;
}
@@ -739,7 +739,7 @@ static int klp_init_object_loaded(struct klp_patch *patch,
return ret;
}
- for (func = obj->funcs; func->old_name; func++) {
+ klp_for_each_func(obj, func) {
ret = klp_find_verify_func_addr(obj, func);
if (ret)
return ret;
@@ -768,7 +768,7 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
if (ret)
return ret;
- for (func = obj->funcs; func->old_name; func++) {
+ klp_for_each_func(obj, func) {
ret = klp_init_func(obj, func);
if (ret)
goto free;
@@ -805,7 +805,7 @@ static int klp_init_patch(struct klp_patch *patch)
if (ret)
goto unlock;
- for (obj = patch->objs; obj->funcs; obj++) {
+ klp_for_each_object(patch, obj) {
ret = klp_init_object(patch, obj);
if (ret)
goto free;
@@ -960,7 +960,7 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
mod->klp_alive = false;
list_for_each_entry(patch, &klp_patches, list) {
- for (obj = patch->objs; obj->funcs; obj++) {
+ klp_for_each_object(patch, obj) {
if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
continue;
--
2.4.1
On 05/19/15 at 12:01P, Jiri Slaby wrote:
> klp_for_each_object and klp_for_each_func are now used all over the
> code. One need not think what is the proper condition to check in the
> for loop now.
>
> Signed-off-by: Jiri Slaby <[email protected]>
> ---
> include/linux/livepatch.h | 6 ++++++
> kernel/livepatch/core.c | 18 +++++++++---------
> 2 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index fe45f2f02c8d..31db7a05dd36 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -805,7 +805,7 @@ static int klp_init_patch(struct klp_patch *patch)
> if (ret)
> goto unlock;
>
> - for (obj = patch->objs; obj->funcs; obj++) {
> + klp_for_each_object(patch, obj) {
> ret = klp_init_object(patch, obj);
> if (ret)
> goto free;
> @@ -960,7 +960,7 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> mod->klp_alive = false;
>
> list_for_each_entry(patch, &klp_patches, list) {
> - for (obj = patch->objs; obj->funcs; obj++) {
> + klp_for_each_object(patch, obj) {
The code is more clearly to use "if", instead of the loop, although we will take
more than one line than previous, since we will always get the first function
from the object.
Thanks
Minfei
> if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
> continue;
>
On Tue, May 19, 2015 at 12:01:18PM +0200, Jiri Slaby wrote:
> From: Miroslav Benes <[email protected]>
>
> Make kobj variable (of type struct kobject) statically allocated in
> klp_object structure. It will allow us to move in the func-object-patch
> hierarchy through kobject links.
>
> The only reason to have it dynamic was to not have empty release
> callback in the code. However we have empty callbacks for function and
> patch in the code now, so it is no longer valid and the advantage of
> static allocation is clear.
>
> Signed-off-by: Miroslav Benes <[email protected]>
> Signed-off-by: Jiri Slaby <[email protected]>
Acked-by: Josh Poimboeuf <[email protected]>
--
Josh
On Tue, May 19, 2015 at 12:01:19PM +0200, Jiri Slaby wrote:
> klp_for_each_object and klp_for_each_func are now used all over the
> code. One need not think what is the proper condition to check in the
> for loop now.
>
> Signed-off-by: Jiri Slaby <[email protected]>
Acked-by: Josh Poimboeuf <[email protected]>
--
Josh
On Tue, 19 May 2015, Minfei Huang wrote:
> > klp_for_each_object and klp_for_each_func are now used all over the
> > code. One need not think what is the proper condition to check in the
> > for loop now.
> >
> > Signed-off-by: Jiri Slaby <[email protected]>
> > ---
> > include/linux/livepatch.h | 6 ++++++
> > kernel/livepatch/core.c | 18 +++++++++---------
> > 2 files changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > index fe45f2f02c8d..31db7a05dd36 100644
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -805,7 +805,7 @@ static int klp_init_patch(struct klp_patch *patch)
> > if (ret)
> > goto unlock;
> >
> > - for (obj = patch->objs; obj->funcs; obj++) {
> > + klp_for_each_object(patch, obj) {
> > ret = klp_init_object(patch, obj);
> > if (ret)
> > goto free;
> > @@ -960,7 +960,7 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> > mod->klp_alive = false;
> >
> > list_for_each_entry(patch, &klp_patches, list) {
> > - for (obj = patch->objs; obj->funcs; obj++) {
> > + klp_for_each_object(patch, obj) {
>
> The code is more clearly to use "if", instead of the loop, although we will take
> more than one line than previous, since we will always get the first function
> from the object.
I have absolutely no idea what you are trying to say here, sorry. Could
you please try to rephrase your review comment?
Thanks,
--
Jiri Kosina
SUSE Labs
On Tue, 19 May 2015, Jiri Slaby wrote:
> From: Miroslav Benes <[email protected]>
>
> Make kobj variable (of type struct kobject) statically allocated in
> klp_object structure. It will allow us to move in the func-object-patch
> hierarchy through kobject links.
>
> The only reason to have it dynamic was to not have empty release
> callback in the code. However we have empty callbacks for function and
> patch in the code now, so it is no longer valid and the advantage of
> static allocation is clear.
Applied to for-4.2/upstream.
--
Jiri Kosina
SUSE Labs
On Tue, 19 May 2015, Jiri Slaby wrote:
> klp_for_each_object and klp_for_each_func are now used all over the
> code. One need not think what is the proper condition to check in the
> for loop now.
>
> Signed-off-by: Jiri Slaby <[email protected]>
Applied to for-4.2/upstream.
--
Jiri Kosina
SUSE Labs
On 05/19/15 at 11:58P, Jiri Kosina wrote:
> On Tue, 19 May 2015, Minfei Huang wrote:
>
> > > klp_for_each_object and klp_for_each_func are now used all over the
> > > code. One need not think what is the proper condition to check in the
> > > for loop now.
> > >
> > > Signed-off-by: Jiri Slaby <[email protected]>
> > > ---
> > > include/linux/livepatch.h | 6 ++++++
> > > kernel/livepatch/core.c | 18 +++++++++---------
> > > 2 files changed, 15 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > > index fe45f2f02c8d..31db7a05dd36 100644
> > > --- a/include/linux/livepatch.h
> > > +++ b/include/linux/livepatch.h
> > > @@ -805,7 +805,7 @@ static int klp_init_patch(struct klp_patch *patch)
> > > if (ret)
> > > goto unlock;
> > >
> > > - for (obj = patch->objs; obj->funcs; obj++) {
> > > + klp_for_each_object(patch, obj) {
> > > ret = klp_init_object(patch, obj);
> > > if (ret)
> > > goto free;
> > > @@ -960,7 +960,7 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> > > mod->klp_alive = false;
> > >
> > > list_for_each_entry(patch, &klp_patches, list) {
> > > - for (obj = patch->objs; obj->funcs; obj++) {
> > > + klp_for_each_object(patch, obj) {
> >
> > The code is more clearly to use "if", instead of the loop, although we will take
> > more than one line than previous, since we will always get the first function
> > from the object.
>
> I have absolutely no idea what you are trying to say here, sorry. Could
> you please try to rephrase your review comment?
>
Sure. Sorry for confuse you with my comment.
Following is the livepatch code.
list_for_each_entry(patch, &klp_patches, list) {
for (obj = patch->objs; obj->funcs; obj++) {
----------------------------------
We get the fisrt object from the patch, then we break the loop. The code is more clearly to
use "if", instead of the loop.
----------------------------------
if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
continue;
if (action == MODULE_STATE_COMING) {
obj->mod = mod;
klp_module_notify_coming(patch, obj);
} else /* MODULE_STATE_GOING */
klp_module_notify_going(patch, obj);
break;
----------------------------------
Here we break the loop.
----------------------------------
}
}
Thanks
Minfei
On 05/20/2015, 03:51 AM, Minfei Huang wrote:
> Sure. Sorry for confuse you with my comment.
Oh, I see now, but:
> list_for_each_entry(patch, &klp_patches, list) {
> for (obj = patch->objs; obj->funcs; obj++) {
> ----------------------------------
> We get the fisrt object from the patch, then we break the loop. The code is more clearly to
> use "if", instead of the loop.
> ----------------------------------
> if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
> continue;
See 'continue' here. This *is* a loop and we do not fetch the first
object. We look for the one with the same name.
> if (action == MODULE_STATE_COMING) {
> obj->mod = mod;
> klp_module_notify_coming(patch, obj);
> } else /* MODULE_STATE_GOING */
> klp_module_notify_going(patch, obj);
>
> break;
> ----------------------------------
> Here we break the loop.
Only if we found the one.
> ----------------------------------
> }
> }
thanks,
--
js
suse labs
On 05/20/15 at 09:11am, Jiri Slaby wrote:
> On 05/20/2015, 03:51 AM, Minfei Huang wrote:
> > Sure. Sorry for confuse you with my comment.
>
> Oh, I see now, but:
>
> > list_for_each_entry(patch, &klp_patches, list) {
> > for (obj = patch->objs; obj->funcs; obj++) {
> > ----------------------------------
> > We get the fisrt object from the patch, then we break the loop. The code is more clearly to
> > use "if", instead of the loop.
> > ----------------------------------
> > if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
> > continue;
>
> See 'continue' here. This *is* a loop and we do not fetch the first
> object. We look for the one with the same name.
>
> > if (action == MODULE_STATE_COMING) {
> > obj->mod = mod;
> > klp_module_notify_coming(patch, obj);
> > } else /* MODULE_STATE_GOING */
> > klp_module_notify_going(patch, obj);
> >
> > break;
> > ----------------------------------
> > Here we break the loop.
>
> Only if we found the one.
>
Hi, Jiri.
Ooops!
Since it is impossible for the differect objects which have the some
name in one patch, it is right to break the loop once we find a matched
object. It confuses me with the logic in __klp_enable_patch.
Thanks for your explanation.
Thanks
Minfei