[PATCH 0/1] *** Replace KLP_* to KLP_TRANSITION_* ***

[PATCH 0/1] *** Replace KLP_* to KLP_TRANSITION_* ***

[zhangwarden]

From: Wardenjohn <zhangwarden@gmail.com>

As the previous described, renaming variables is not a good idead
becase it will break userspace. This patch rename macros of KLP_*
to KLP_TRANSITION_* to fix the confusing description of the klp
transition state. With this marcos renamed, comments are not 
necessary at this point.

Wardenjohn (1):
  livepatch: Rename KLP_* to KLP_TRANSITION_*

 include/linux/livepatch.h     |  6 ++--
 init/init_task.c              |  2 +-
 kernel/livepatch/core.c       |  5 ++--
 kernel/livepatch/patch.c      |  4 +--
 kernel/livepatch/transition.c | 54 +++++++++++++++++------------------
 5 files changed, 36 insertions(+), 35 deletions(-)

-- 
2.37.3

[PATCH 1/1] livepatch: Rename KLP_* to KLP_TRANSITION_*

[zhangwarden]

From: Wardenjohn <zhangwarden@gmail.com>

The original macros of KLP_* is about the state of the transition.
Rename macros of KLP_* to KLP_TRANSITION_* to fix the confusing
description of klp transition state.

Signed-off-by: Wardenjohn <zhangwarden@gmail.com>
---
 include/linux/livepatch.h     |  6 ++--
 init/init_task.c              |  2 +-
 kernel/livepatch/core.c       |  5 ++--
 kernel/livepatch/patch.c      |  4 +--
 kernel/livepatch/transition.c | 54 +++++++++++++++++------------------
 5 files changed, 36 insertions(+), 35 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 9b9b38e89563..25db447367a3 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -18,9 +18,9 @@
 #if IS_ENABLED(CONFIG_LIVEPATCH)
 
 /* task patch states */
-#define KLP_UNDEFINED	-1
-#define KLP_UNPATCHED	 0
-#define KLP_PATCHED	 1
+#define KLP_TRANSITION_IDLE	        -1 /* idle, no transition in progress */
+#define KLP_TRANSITION_UNPATCHED	 0 /* transitioning to unpatched state */
+#define KLP_TRANSITION_PATCHED		 1 /* transitioning to patched state */
 
 /**
  * struct klp_func - function structure for live patching
diff --git a/init/init_task.c b/init/init_task.c
index 2558b719e053..eeb110c65fe2 100644
--- a/init/init_task.c
+++ b/init/init_task.c
@@ -199,7 +199,7 @@ struct task_struct init_task __aligned(L1_CACHE_BYTES) = {
 	.trace_recursion = 0,
 #endif
 #ifdef CONFIG_LIVEPATCH
-	.patch_state	= KLP_UNDEFINED,
+	.patch_state	= KLP_TRANSITION_IDLE,
 #endif
 #ifdef CONFIG_SECURITY
 	.security	= NULL,
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index ecbc9b6aba3a..39b767b22cd7 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -40,6 +40,7 @@ DEFINE_MUTEX(klp_mutex);
 /*
  * Actively used patches: enabled or in transition. Note that replaced
  * or disabled patches are not listed even though the related kernel
+a
  * module still can be loaded.
  */
 LIST_HEAD(klp_patches);
@@ -973,7 +974,7 @@ static int __klp_disable_patch(struct klp_patch *patch)
 	if (klp_transition_patch)
 		return -EBUSY;
 
-	klp_init_transition(patch, KLP_UNPATCHED);
+	klp_init_transition(patch, KLP_TRANSITION_UNPATCHED);
 
 	klp_for_each_object(patch, obj)
 		if (obj->patched)
@@ -1008,7 +1009,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
 
 	pr_notice("enabling patch '%s'\n", patch->mod->name);
 
-	klp_init_transition(patch, KLP_PATCHED);
+	klp_init_transition(patch, KLP_TRANSITION_PATCHED);
 
 	/*
 	 * Enforce the order of the func->transition writes in
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 4152c71507e2..90408500e5a3 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -95,9 +95,9 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 
 		patch_state = current->patch_state;
 
-		WARN_ON_ONCE(patch_state == KLP_UNDEFINED);
+		WARN_ON_ONCE(patch_state == KLP_TRANSITION_IDLE);
 
-		if (patch_state == KLP_UNPATCHED) {
+		if (patch_state == KLP_TRANSITION_UNPATCHED) {
 			/*
 			 * Use the previously patched version of the function.
 			 * If no previous patches exist, continue with the
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index e54c3d60a904..ba069459c101 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -23,7 +23,7 @@ static DEFINE_PER_CPU(unsigned long[MAX_STACK_ENTRIES], klp_stack_entries);
 
 struct klp_patch *klp_transition_patch;
 
-static int klp_target_state = KLP_UNDEFINED;
+static int klp_target_state = KLP_TRANSITION_IDLE;
 
 static unsigned int klp_signals_cnt;
 
@@ -96,16 +96,16 @@ static void klp_complete_transition(void)
 
 	pr_debug("'%s': completing %s transition\n",
 		 klp_transition_patch->mod->name,
-		 klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
+		 klp_target_state == KLP_TRANSITION_PATCHED ? "patching" : "unpatching");
 
-	if (klp_transition_patch->replace && klp_target_state == KLP_PATCHED) {
+	if (klp_transition_patch->replace && klp_target_state == KLP_TRANSITION_PATCHED) {
 		klp_unpatch_replaced_patches(klp_transition_patch);
 		klp_discard_nops(klp_transition_patch);
 	}
 
-	if (klp_target_state == KLP_UNPATCHED) {
+	if (klp_target_state == KLP_TRANSITION_UNPATCHED) {
 		/*
-		 * All tasks have transitioned to KLP_UNPATCHED so we can now
+		 * All tasks have transitioned to KLP_TRANSITION_UNPATCHED so we can now
 		 * remove the new functions from the func_stack.
 		 */
 		klp_unpatch_objects(klp_transition_patch);
@@ -123,36 +123,36 @@ static void klp_complete_transition(void)
 		klp_for_each_func(obj, func)
 			func->transition = false;
 
-	/* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */
-	if (klp_target_state == KLP_PATCHED)
+	/* Prevent klp_ftrace_handler() from seeing KLP_TRANSITION_IDLE state */
+	if (klp_target_state == KLP_TRANSITION_PATCHED)
 		klp_synchronize_transition();
 
 	read_lock(&tasklist_lock);
 	for_each_process_thread(g, task) {
 		WARN_ON_ONCE(test_tsk_thread_flag(task, TIF_PATCH_PENDING));
-		task->patch_state = KLP_UNDEFINED;
+		task->patch_state = KLP_TRANSITION_IDLE;
 	}
 	read_unlock(&tasklist_lock);
 
 	for_each_possible_cpu(cpu) {
 		task = idle_task(cpu);
 		WARN_ON_ONCE(test_tsk_thread_flag(task, TIF_PATCH_PENDING));
-		task->patch_state = KLP_UNDEFINED;
+		task->patch_state = KLP_TRANSITION_IDLE;
 	}
 
 	klp_for_each_object(klp_transition_patch, obj) {
 		if (!klp_is_object_loaded(obj))
 			continue;
-		if (klp_target_state == KLP_PATCHED)
+		if (klp_target_state == KLP_TRANSITION_PATCHED)
 			klp_post_patch_callback(obj);
-		else if (klp_target_state == KLP_UNPATCHED)
+		else if (klp_target_state == KLP_TRANSITION_UNPATCHED)
 			klp_post_unpatch_callback(obj);
 	}
 
 	pr_notice("'%s': %s complete\n", klp_transition_patch->mod->name,
-		  klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
+		  klp_target_state == KLP_TRANSITION_PATCHED ? "patching" : "unpatching");
 
-	klp_target_state = KLP_UNDEFINED;
+	klp_target_state = KLP_TRANSITION_IDLE;
 	klp_transition_patch = NULL;
 }
 
@@ -164,13 +164,13 @@ static void klp_complete_transition(void)
  */
 void klp_cancel_transition(void)
 {
-	if (WARN_ON_ONCE(klp_target_state != KLP_PATCHED))
+	if (WARN_ON_ONCE(klp_target_state != KLP_TRANSITION_PATCHED))
 		return;
 
 	pr_debug("'%s': canceling patching transition, going to unpatch\n",
 		 klp_transition_patch->mod->name);
 
-	klp_target_state = KLP_UNPATCHED;
+	klp_target_state = KLP_TRANSITION_UNPATCHED;
 	klp_complete_transition();
 }
 
@@ -218,7 +218,7 @@ static int klp_check_stack_func(struct klp_func *func, unsigned long *entries,
 	struct klp_ops *ops;
 	int i;
 
-	if (klp_target_state == KLP_UNPATCHED) {
+	if (klp_target_state == KLP_TRANSITION_UNPATCHED) {
 		 /*
 		  * Check for the to-be-unpatched function
 		  * (the func itself).
@@ -455,7 +455,7 @@ void klp_try_complete_transition(void)
 	struct klp_patch *patch;
 	bool complete = true;
 
-	WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED);
+	WARN_ON_ONCE(klp_target_state == KLP_TRANSITION_IDLE);
 
 	/*
 	 * Try to switch the tasks to the target patch state by walking their
@@ -532,11 +532,11 @@ void klp_start_transition(void)
 	struct task_struct *g, *task;
 	unsigned int cpu;
 
-	WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED);
+	WARN_ON_ONCE(klp_target_state == KLP_TRANSITION_IDLE);
 
 	pr_notice("'%s': starting %s transition\n",
 		  klp_transition_patch->mod->name,
-		  klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
+		  klp_target_state == KLP_TRANSITION_PATCHED ? "patching" : "unpatching");
 
 	/*
 	 * Mark all normal tasks as needing a patch state update.  They'll
@@ -578,7 +578,7 @@ void klp_init_transition(struct klp_patch *patch, int state)
 	struct klp_func *func;
 	int initial_state = !state;
 
-	WARN_ON_ONCE(klp_target_state != KLP_UNDEFINED);
+	WARN_ON_ONCE(klp_target_state != KLP_TRANSITION_IDLE);
 
 	klp_transition_patch = patch;
 
@@ -589,7 +589,7 @@ void klp_init_transition(struct klp_patch *patch, int state)
 	klp_target_state = state;
 
 	pr_debug("'%s': initializing %s transition\n", patch->mod->name,
-		 klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
+		 klp_target_state == KLP_TRANSITION_PATCHED ? "patching" : "unpatching");
 
 	/*
 	 * Initialize all tasks to the initial patch state to prepare them for
@@ -597,7 +597,7 @@ void klp_init_transition(struct klp_patch *patch, int state)
 	 */
 	read_lock(&tasklist_lock);
 	for_each_process_thread(g, task) {
-		WARN_ON_ONCE(task->patch_state != KLP_UNDEFINED);
+		WARN_ON_ONCE(task->patch_state != KLP_TRANSITION_IDLE);
 		task->patch_state = initial_state;
 	}
 	read_unlock(&tasklist_lock);
@@ -607,19 +607,19 @@ void klp_init_transition(struct klp_patch *patch, int state)
 	 */
 	for_each_possible_cpu(cpu) {
 		task = idle_task(cpu);
-		WARN_ON_ONCE(task->patch_state != KLP_UNDEFINED);
+		WARN_ON_ONCE(task->patch_state != KLP_TRANSITION_IDLE);
 		task->patch_state = initial_state;
 	}
 
 	/*
 	 * Enforce the order of the task->patch_state initializations and the
 	 * func->transition updates to ensure that klp_ftrace_handler() doesn't
-	 * see a func in transition with a task->patch_state of KLP_UNDEFINED.
+	 * see a func in transition with a task->patch_state of KLP_TRANSITION_IDLE.
 	 *
 	 * Also enforce the order of the klp_target_state write and future
 	 * TIF_PATCH_PENDING writes to ensure klp_update_patch_state() and
 	 * __klp_sched_try_switch() don't set a task->patch_state to
-	 * KLP_UNDEFINED.
+	 * KLP_TRANSITION_IDLE.
 	 */
 	smp_wmb();
 
@@ -652,7 +652,7 @@ void klp_reverse_transition(void)
 
 	pr_debug("'%s': reversing transition from %s\n",
 		 klp_transition_patch->mod->name,
-		 klp_target_state == KLP_PATCHED ? "patching to unpatching" :
+		 klp_target_state == KLP_TRANSITION_PATCHED ? "patching to unpatching" :
 						   "unpatching to patching");
 
 	/*
@@ -741,7 +741,7 @@ void klp_force_transition(void)
 		klp_update_patch_state(idle_task(cpu));
 
 	/* Set forced flag for patches being removed. */
-	if (klp_target_state == KLP_UNPATCHED)
+	if (klp_target_state == KLP_TRANSITION_UNPATCHED)
 		klp_transition_patch->forced = true;
 	else if (klp_transition_patch->replace) {
 		klp_for_each_patch(patch) {
-- 
2.37.3

Re: [PATCH 0/1] *** Replace KLP_* to KLP_TRANSITION_* ***

[zhang warden]

> 
> transition state. With this marcos renamed, comments are not 
> necessary at this point.
> 
Sorry for my careless, the comment still remains in the code. However, comment in the code do no harms here. Maybe it can be kept.

Re: [PATCH 0/1] *** Replace KLP_* to KLP_TRANSITION_* ***

[Josh Poimboeuf]

On Tue, May 07, 2024 at 10:21:40AM +0800, zhang warden wrote:
> 
> 
> > 
> > transition state. With this marcos renamed, comments are not 
> > necessary at this point.
> > 
> Sorry for my careless, the comment still remains in the code. However,
> comment in the code do no harms here. Maybe it can be kept.

The comments aren't actually correct.

For example, KLP_TRANSITION_UNPATCHED doesn't always mean "transitioning
to unpatched state".  Sometimes it means "transitioning *from* unpatched
state".

-- 
Josh

Re: [PATCH 0/1] *** Replace KLP_* to KLP_TRANSITION_* ***

[zhang warden]

> On May 7, 2024, at 10:41, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> 
> On Tue, May 07, 2024 at 10:21:40AM +0800, zhang warden wrote:
>> 
>> 
>>> 
>>> transition state. With this marcos renamed, comments are not 
>>> necessary at this point.
>>> 
>> Sorry for my careless, the comment still remains in the code. However,
>> comment in the code do no harms here. Maybe it can be kept.
> 
> The comments aren't actually correct.
> 
> For example, KLP_TRANSITION_UNPATCHED doesn't always mean "transitioning
> to unpatched state".  Sometimes it means "transitioning *from* unpatched
> state".
> 
> -- 
> Josh

OK, I got it. I will remove the comment later. After all, comment is not necessary at this point after we rename the macros.

Re: [PATCH 0/1] *** Replace KLP_* to KLP_TRANSITION_* ***

[Josh Poimboeuf]

On Tue, May 07, 2024 at 10:56:09AM +0800, zhang warden wrote:
> 
> 
> > On May 7, 2024, at 10:41, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > 
> > On Tue, May 07, 2024 at 10:21:40AM +0800, zhang warden wrote:
> >> 
> >> 
> >>> 
> >>> transition state. With this marcos renamed, comments are not 
> >>> necessary at this point.
> >>> 
> >> Sorry for my careless, the comment still remains in the code. However,
> >> comment in the code do no harms here. Maybe it can be kept.
> > 
> > The comments aren't actually correct.
> > 
> > For example, KLP_TRANSITION_UNPATCHED doesn't always mean "transitioning
> > to unpatched state".  Sometimes it means "transitioning *from* unpatched
> > state".
> > 
> > -- 
> > Josh
> 
> OK, I got it. I will remove the comment later. After all, comment is
> not necessary at this point after we rename the macros.

Yeah, removing them altogether might be best, as the meaning of these
can vary slightly depending on the operation (patching vs unpatching),
and also depending on where it's stored (task->patch_state vs klp_target_state).

-- 
Josh