Re: [PATCH v3 08/15] livepatch: separate enabled and patched states

[Kamalesh Babulal]

On Thursday 08 December 2016 11:38 PM, Josh Poimboeuf wrote:

Once we have a consistency model, patches and their objects will be
enabled and disabled at different times.  For example, when a patch is
disabled, its loaded objects' funcs can remain registered with ftrace
indefinitely until the unpatching operation is complete and they're no
longer in use.

It's less confusing if we give them different names: patches can be
enabled or disabled; objects (and their funcs) can be patched or
unpatched:

- Enabled means that a patch is logically enabled (but not necessarily
  fully applied).

- Patched means that an object's funcs are registered with ftrace and
  added to the klp_ops func stack.

Also, since these states are binary, represent them with booleans
instead of ints.

Signed-off-by: Josh Poimboeuf 


Reviewed-by: Kamalesh Babulal 

--
cheers,
Kamalesh.

Re: [PATCH v3 08/15] livepatch: separate enabled and patched states

[Miroslav Benes]

On Thu, 8 Dec 2016, Josh Poimboeuf wrote:

> Once we have a consistency model, patches and their objects will be
> enabled and disabled at different times.  For example, when a patch is
> disabled, its loaded objects' funcs can remain registered with ftrace
> indefinitely until the unpatching operation is complete and they're no
> longer in use.
> 
> It's less confusing if we give them different names: patches can be
> enabled or disabled; objects (and their funcs) can be patched or
> unpatched:
> 
> - Enabled means that a patch is logically enabled (but not necessarily
>   fully applied).
> 
> - Patched means that an object's funcs are registered with ftrace and
>   added to the klp_ops func stack.
> 
> Also, since these states are binary, represent them with booleans
> instead of ints.
> 
> Signed-off-by: Josh Poimboeuf 

Acked-by: Miroslav Benes 

Miroslav

Re: [PATCH v3 08/15] livepatch: separate enabled and patched states

[Petr Mladek]

On Thu 2016-12-08 12:08:33, Josh Poimboeuf wrote:
> Once we have a consistency model, patches and their objects will be
> enabled and disabled at different times.  For example, when a patch is
> disabled, its loaded objects' funcs can remain registered with ftrace
> indefinitely until the unpatching operation is complete and they're no
> longer in use.
> 
> It's less confusing if we give them different names: patches can be
> enabled or disabled; objects (and their funcs) can be patched or
> unpatched:
> 
> - Enabled means that a patch is logically enabled (but not necessarily
>   fully applied).
> 
> - Patched means that an object's funcs are registered with ftrace and
>   added to the klp_ops func stack.
> 
> Also, since these states are binary, represent them with booleans
> instead of ints.
> 
> Signed-off-by: Josh Poimboeuf 

Makes sense. The patch is pretty straightforward.

Reviewed-by: Petr Mladek 

Best Regards,
Petr

[PATCH v3 08/15] livepatch: separate enabled and patched states

[Josh Poimboeuf]

Once we have a consistency model, patches and their objects will be
enabled and disabled at different times.  For example, when a patch is
disabled, its loaded objects' funcs can remain registered with ftrace
indefinitely until the unpatching operation is complete and they're no
longer in use.

It's less confusing if we give them different names: patches can be
enabled or disabled; objects (and their funcs) can be patched or
unpatched:

- Enabled means that a patch is logically enabled (but not necessarily
  fully applied).

- Patched means that an object's funcs are registered with ftrace and
  added to the klp_ops func stack.

Also, since these states are binary, represent them with booleans
instead of ints.

Signed-off-by: Josh Poimboeuf 
---
 include/linux/livepatch.h | 17 ---
 kernel/livepatch/core.c   | 72 +++
 2 files changed, 42 insertions(+), 47 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 60558d8..1e2eb91 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -28,11 +28,6 @@
 
 #include 
 
-enum klp_state {
-   KLP_DISABLED,
-   KLP_ENABLED
-};
-
 /**
  * struct klp_func - function structure for live patching
  * @old_name:  name of the function to be patched
@@ -41,8 +36,8 @@ enum klp_state {
  * can be found (optional)
  * @old_addr:  the address of the function being patched
  * @kobj:  kobject for sysfs resources
- * @state: tracks function-level patch application state
  * @stack_node:list node for klp_ops func_stack list
+ * @patched:   the func has been added to the klp_ops list
  */
 struct klp_func {
/* external */
@@ -60,8 +55,8 @@ struct klp_func {
/* internal */
unsigned long old_addr;
struct kobject kobj;
-   enum klp_state state;
struct list_head stack_node;
+   bool patched;
 };
 
 /**
@@ -71,7 +66,7 @@ struct klp_func {
  * @kobj:  kobject for sysfs resources
  * @mod:   kernel module associated with the patched object
  * (NULL for vmlinux)
- * @state: tracks object-level patch application state
+ * @patched:   the object's funcs have been added to the klp_ops list
  */
 struct klp_object {
/* external */
@@ -81,7 +76,7 @@ struct klp_object {
/* internal */
struct kobject kobj;
struct module *mod;
-   enum klp_state state;
+   bool patched;
 };
 
 /**
@@ -90,7 +85,7 @@ struct klp_object {
  * @objs:  object entries for kernel objects to be patched
  * @list:  list node for global list of registered patches
  * @kobj:  kobject for sysfs resources
- * @state: tracks patch-level application state
+ * @enabled:   the patch is enabled (but operation may be incomplete)
  */
 struct klp_patch {
/* external */
@@ -100,7 +95,7 @@ struct klp_patch {
/* internal */
struct list_head list;
struct kobject kobj;
-   enum klp_state state;
+   bool enabled;
 };
 
 #define klp_for_each_object(patch, obj) \
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 217b39d..2dbd355 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -348,11 +348,11 @@ static unsigned long klp_get_ftrace_location(unsigned 
long faddr)
 }
 #endif
 
-static void klp_disable_func(struct klp_func *func)
+static void klp_unpatch_func(struct klp_func *func)
 {
struct klp_ops *ops;
 
-   if (WARN_ON(func->state != KLP_ENABLED))
+   if (WARN_ON(!func->patched))
return;
if (WARN_ON(!func->old_addr))
return;
@@ -378,10 +378,10 @@ static void klp_disable_func(struct klp_func *func)
list_del_rcu(>stack_node);
}
 
-   func->state = KLP_DISABLED;
+   func->patched = false;
 }
 
-static int klp_enable_func(struct klp_func *func)
+static int klp_patch_func(struct klp_func *func)
 {
struct klp_ops *ops;
int ret;
@@ -389,7 +389,7 @@ static int klp_enable_func(struct klp_func *func)
if (WARN_ON(!func->old_addr))
return -EINVAL;
 
-   if (WARN_ON(func->state != KLP_DISABLED))
+   if (WARN_ON(func->patched))
return -EINVAL;
 
ops = klp_find_ops(func->old_addr);
@@ -437,7 +437,7 @@ static int klp_enable_func(struct klp_func *func)
list_add_rcu(>stack_node, >func_stack);
}
 
-   func->state = KLP_ENABLED;
+   func->patched = true;
 
return 0;
 
@@ -448,36 +448,36 @@ static int klp_enable_func(struct klp_func *func)
return ret;
 }
 
-static void klp_disable_object(struct klp_object *obj)
+static void klp_unpatch_object(struct klp_object *obj)
 {
struct klp_func *func;
 
klp_for_each_func(obj, func)
-   if (func->state == KLP_ENABLED)
-   klp_disable_func(func);
+   if (func->patched)
+