[PATCH v3 0/2] livepatch: some improvements

[PATCH v3 0/2] livepatch: some improvements

[Yafang Shao]

  #1: Clarify klp_add_nops()
  #2: Replace the tasklist_lock with RCU in the KLP transition

v2->v3:
- Skip the newly fokred tasks during klp_check_and_switch_task() (Josh)
- Fix the local variable patch_state in the ftrace handler (Josh)

v1->v2: https://lore.kernel.org/live-patching/20250223062046.2943-1-laoar.shao@gmail.com/
- Enhance the comment in #1 for better clarity and detail. (Petr)
- Replace the tasklist_lock with RCU (Josh)
- Remove the fix for RCU warnings as the root cause is currently unclear.
  Once the root cause is identified, I will submit the fix separately.

v1: https://lore.kernel.org/live-patching/20250211062437.46811-1-laoar.shao@gmail.com/

Yafang Shao (2):
  livepatch: Add comment to clarify klp_add_nops()
  livepatch: Replace tasklist_lock with RCU

 include/linux/livepatch.h     |  4 ++--
 kernel/fork.c                 |  2 +-
 kernel/livepatch/core.c       |  9 ++++++---
 kernel/livepatch/patch.c      |  8 +++++++-
 kernel/livepatch/transition.c | 35 ++++++++++++++---------------------
 kernel/livepatch/transition.h |  1 +
 6 files changed, 31 insertions(+), 28 deletions(-)

-- 
2.43.5

[PATCH v3 1/2] livepatch: Add comment to clarify klp_add_nops()

[Yafang Shao]

Add detailed comments to clarify the purpose of klp_add_nops() function.
These comments are based on Petr's explanation[0].

Link: https://lore.kernel.org/all/Z6XUA7D0eU_YDMVp@pathway.suse.cz/ [0]
Suggested-by: Petr Mladek <pmladek@suse.com>
Suggested-by: Josh Poimboeuf <jpoimboe@kernel.org>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 kernel/livepatch/core.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 0cd39954d5a1..4a0fb7978d0d 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -601,9 +601,12 @@ static int klp_add_object_nops(struct klp_patch *patch,
 }
 
 /*
- * Add 'nop' functions which simply return to the caller to run
- * the original function. The 'nop' functions are added to a
- * patch to facilitate a 'replace' mode.
+ * Add 'nop' functions which simply return to the caller to run the
+ * original function.
+ *
+ * They are added only when the atomic replace mode is used and only for
+ * functions which are currently livepatched but are no longer included
+ * in the new livepatch.
  */
 static int klp_add_nops(struct klp_patch *patch)
 {
-- 
2.43.5

[PATCH v3 2/2] livepatch: Replace tasklist_lock with RCU

[Yafang Shao]

The tasklist_lock in the KLP transition might result high latency under
specific workload. We can replace it with RCU.

After a new task is forked, its kernel stack is always set to empty[0].
Therefore, we can init these new tasks to KLP_TRANSITION_IDLE state
after they are forked. If these tasks are forked during the KLP
transition but before klp_check_and_switch_task(), they can be safely
skipped during klp_check_and_switch_task(). Additionally, if
klp_ftrace_handler() is triggered right after forking, the task can
determine which function to use based on the klp_target_state.

With the above change, we can safely convert the tasklist_lock to RCU.

Link: https://lore.kernel.org/all/20250213173253.ovivhuq2c5rmvkhj@jpoimboe/ [0]
Link: https://lore.kernel.org/all/20250214181206.xkvxohoc4ft26uhf@jpoimboe/ [1]
Suggested-by: Josh Poimboeuf <jpoimboe@kernel.org>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 include/linux/livepatch.h     |  4 ++--
 kernel/fork.c                 |  2 +-
 kernel/livepatch/patch.c      |  8 +++++++-
 kernel/livepatch/transition.c | 35 ++++++++++++++---------------------
 kernel/livepatch/transition.h |  1 +
 5 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 51a258c24ff5..41c424120f49 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -198,7 +198,7 @@ int klp_enable_patch(struct klp_patch *);
 int klp_module_coming(struct module *mod);
 void klp_module_going(struct module *mod);
 
-void klp_copy_process(struct task_struct *child);
+void klp_init_process(struct task_struct *child);
 void klp_update_patch_state(struct task_struct *task);
 
 static inline bool klp_patch_pending(struct task_struct *task)
@@ -241,7 +241,7 @@ static inline int klp_module_coming(struct module *mod) { return 0; }
 static inline void klp_module_going(struct module *mod) {}
 static inline bool klp_patch_pending(struct task_struct *task) { return false; }
 static inline void klp_update_patch_state(struct task_struct *task) {}
-static inline void klp_copy_process(struct task_struct *child) {}
+static inline void klp_init_process(struct task_struct *child) {}
 
 static inline
 int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
diff --git a/kernel/fork.c b/kernel/fork.c
index 735405a9c5f3..da247c4d5ec5 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2544,7 +2544,7 @@ __latent_entropy struct task_struct *copy_process(
 		p->exit_signal = args->exit_signal;
 	}
 
-	klp_copy_process(p);
+	klp_init_process(p);
 
 	sched_core_fork(p);
 
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 90408500e5a3..3da98877c785 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -95,7 +95,13 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 
 		patch_state = current->patch_state;
 
-		WARN_ON_ONCE(patch_state == KLP_TRANSITION_IDLE);
+		/* If the patch_state is KLP_TRANSITION_IDLE, it means the task
+		 * was forked after klp_init_transition(). In this case, the
+		 * newly forked task can determine which function to use based
+		 * on the klp_target_state.
+		 */
+		if (patch_state == KLP_TRANSITION_IDLE)
+			patch_state = klp_target_state;
 
 		if (patch_state == KLP_TRANSITION_UNPATCHED) {
 			/*
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index ba069459c101..af76defca67a 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_TRANSITION_IDLE;
+int klp_target_state = KLP_TRANSITION_IDLE;
 
 static unsigned int klp_signals_cnt;
 
@@ -294,6 +294,14 @@ static int klp_check_and_switch_task(struct task_struct *task, void *arg)
 {
 	int ret;
 
+	/*
+	 * If the patch_state remains KLP_TRANSITION_IDLE at this point, it
+	 * indicates that the task was forked after klp_init_transition().
+	 * In this case, it is safe to skip the task.
+	 */
+	if (!test_tsk_thread_flag(task, TIF_PATCH_PENDING))
+		return 0;
+
 	if (task_curr(task) && task != current)
 		return -EBUSY;
 
@@ -466,11 +474,11 @@ void klp_try_complete_transition(void)
 	 * Usually this will transition most (or all) of the tasks on a system
 	 * unless the patch includes changes to a very common function.
 	 */
-	read_lock(&tasklist_lock);
+	rcu_read_lock();
 	for_each_process_thread(g, task)
 		if (!klp_try_switch_task(task))
 			complete = false;
-	read_unlock(&tasklist_lock);
+	rcu_read_unlock();
 
 	/*
 	 * Ditto for the idle "swapper" tasks.
@@ -694,25 +702,10 @@ void klp_reverse_transition(void)
 }
 
 /* Called from copy_process() during fork */
-void klp_copy_process(struct task_struct *child)
+void klp_init_process(struct task_struct *child)
 {
-
-	/*
-	 * The parent process may have gone through a KLP transition since
-	 * the thread flag was copied in setup_thread_stack earlier. Bring
-	 * the task flag up to date with the parent here.
-	 *
-	 * The operation is serialized against all klp_*_transition()
-	 * operations by the tasklist_lock. The only exceptions are
-	 * klp_update_patch_state(current) and __klp_sched_try_switch(), but we
-	 * cannot race with them because we are current.
-	 */
-	if (test_tsk_thread_flag(current, TIF_PATCH_PENDING))
-		set_tsk_thread_flag(child, TIF_PATCH_PENDING);
-	else
-		clear_tsk_thread_flag(child, TIF_PATCH_PENDING);
-
-	child->patch_state = current->patch_state;
+	clear_tsk_thread_flag(child, TIF_PATCH_PENDING);
+	child->patch_state = KLP_TRANSITION_IDLE;
 }
 
 /*
diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
index 322db16233de..febcf1d50fc5 100644
--- a/kernel/livepatch/transition.h
+++ b/kernel/livepatch/transition.h
@@ -5,6 +5,7 @@
 #include <linux/livepatch.h>
 
 extern struct klp_patch *klp_transition_patch;
+extern int klp_target_state;
 
 void klp_init_transition(struct klp_patch *patch, int state);
 void klp_cancel_transition(void);
-- 
2.43.5

Re: [PATCH v3 1/2] livepatch: Add comment to clarify klp_add_nops()

[Petr Mladek]

On Thu 2025-02-27 10:47:32, Yafang Shao wrote:
> Add detailed comments to clarify the purpose of klp_add_nops() function.
> These comments are based on Petr's explanation[0].
> 
> Link: https://lore.kernel.org/all/Z6XUA7D0eU_YDMVp@pathway.suse.cz/ [0]
> Suggested-by: Petr Mladek <pmladek@suse.com>
> Suggested-by: Josh Poimboeuf <jpoimboe@kernel.org>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH v3 2/2] livepatch: Replace tasklist_lock with RCU

[Petr Mladek]

On Thu 2025-02-27 10:47:33, Yafang Shao wrote:
> The tasklist_lock in the KLP transition might result high latency under
> specific workload. We can replace it with RCU.
> 
> After a new task is forked, its kernel stack is always set to empty[0].
> Therefore, we can init these new tasks to KLP_TRANSITION_IDLE state
> after they are forked. If these tasks are forked during the KLP
> transition but before klp_check_and_switch_task(), they can be safely
> skipped during klp_check_and_switch_task(). Additionally, if
> klp_ftrace_handler() is triggered right after forking, the task can
> determine which function to use based on the klp_target_state.
> 
> With the above change, we can safely convert the tasklist_lock to RCU.
> 
> Link: https://lore.kernel.org/all/20250213173253.ovivhuq2c5rmvkhj@jpoimboe/ [0]
> Link: https://lore.kernel.org/all/20250214181206.xkvxohoc4ft26uhf@jpoimboe/ [1]
> Suggested-by: Josh Poimboeuf <jpoimboe@kernel.org>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
> --- a/kernel/livepatch/patch.c
> +++ b/kernel/livepatch/patch.c
> @@ -95,7 +95,13 @@ static void notrace klp_ftrace_handler(unsigned long ip,
>  
>  		patch_state = current->patch_state;
>  
> -		WARN_ON_ONCE(patch_state == KLP_TRANSITION_IDLE);
> +		/* If the patch_state is KLP_TRANSITION_IDLE, it means the task
> +		 * was forked after klp_init_transition(). In this case, the
> +		 * newly forked task can determine which function to use based
> +		 * on the klp_target_state.
> +		 */
> +		if (patch_state == KLP_TRANSITION_IDLE)
> +			patch_state = klp_target_state;
>  

CPU0				CPU1

task_0 (forked process):

funcA
  klp_ftrace_handler()
    if (patch_state == KLP_TRANSITION_IDLE)
       patch_state = klp_target_state
	# set to KLP_TRANSITION_PATCHED

  # redirected to klp_funcA()


				echo 0 >/sys/kernel/livepatch/patch1/enabled

				klp_reverse_transition()

				  klp_target_state = !klp_target_state;
				    # set to KLP_TRANSITION_UNPATCHED


   funcB
     if (patch_state == KLP_TRANSITION_IDLE)
       patch_state = klp_target_state
	 # set to KLP_TRANSITION_UNPATCHED

   # staying in origianl funcB


BANG: livepatched and original function called at the same time

      => broken consistency model.

BTW: This is what I tried to warn you about at
     https://lore.kernel.org/r/Z69Wuhve2vnsrtp_@pathway.suse.cz

See below for more:

>  		if (patch_state == KLP_TRANSITION_UNPATCHED) {
>  			/*
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index ba069459c101..af76defca67a 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_TRANSITION_IDLE;
> +int klp_target_state = KLP_TRANSITION_IDLE;
>  
>  static unsigned int klp_signals_cnt;
>  
> @@ -294,6 +294,14 @@ static int klp_check_and_switch_task(struct task_struct *task, void *arg)
>  {
>  	int ret;
>  
> +	/*
> +	 * If the patch_state remains KLP_TRANSITION_IDLE at this point, it
> +	 * indicates that the task was forked after klp_init_transition().
> +	 * In this case, it is safe to skip the task.
> +	 */
> +	if (!test_tsk_thread_flag(task, TIF_PATCH_PENDING))
> +		return 0;
> +
>  	if (task_curr(task) && task != current)
>  		return -EBUSY;
>  
> @@ -466,11 +474,11 @@ void klp_try_complete_transition(void)
>  	 * Usually this will transition most (or all) of the tasks on a system
>  	 * unless the patch includes changes to a very common function.
>  	 */
> -	read_lock(&tasklist_lock);
> +	rcu_read_lock();
>  	for_each_process_thread(g, task)
>  		if (!klp_try_switch_task(task))
>  			complete = false;
> -	read_unlock(&tasklist_lock);
> +	rcu_read_unlock();
>  
>  	/*
>  	 * Ditto for the idle "swapper" tasks.
> @@ -694,25 +702,10 @@ void klp_reverse_transition(void)
>  }
>  
>  /* Called from copy_process() during fork */
> -void klp_copy_process(struct task_struct *child)
> +void klp_init_process(struct task_struct *child)
>  {
> -
> -	/*
> -	 * The parent process may have gone through a KLP transition since
> -	 * the thread flag was copied in setup_thread_stack earlier. Bring
> -	 * the task flag up to date with the parent here.
> -	 *
> -	 * The operation is serialized against all klp_*_transition()
> -	 * operations by the tasklist_lock. The only exceptions are
> -	 * klp_update_patch_state(current) and __klp_sched_try_switch(), but we
> -	 * cannot race with them because we are current.
> -	 */
> -	if (test_tsk_thread_flag(current, TIF_PATCH_PENDING))
> -		set_tsk_thread_flag(child, TIF_PATCH_PENDING);
> -	else
> -		clear_tsk_thread_flag(child, TIF_PATCH_PENDING);
> -
> -	child->patch_state = current->patch_state;
> +	clear_tsk_thread_flag(child, TIF_PATCH_PENDING);
> +	child->patch_state = KLP_TRANSITION_IDLE;

I thought that we might do:

	child->patch_state = klp_target_state;

to avoid the race in the klp_ftrace_handler() described above.

But the following might happen:

CPU0				CPU1

klp_init_process(child)

  child->patch_state = KLP_TRANSITION_IDLE

				klp_enable_patch()
				  # setup ftrace handlers
				  # initialize all processes
				  # in the task list

  # add "child" into the task list

  schedule()

[running child now]

funcA()

  klp_ftrace_handler()

    child->patch_state = KLP_TRANSITION_IDLE

BANG: Same problem as with the original patch.


Hmm, the 2nd version of this patchset tried to do:

diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 90408500e5a3..5e523a3fbb3c 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -95,7 +95,12 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 
 		patch_state = current->patch_state;
 
-		WARN_ON_ONCE(patch_state == KLP_TRANSITION_IDLE);
+		/* If the patch_state is KLP_TRANSITION_IDLE, it indicates the
+		 * task was forked after klp_init_transition(). For this newly
+		 * forked task, it is safe to switch it to klp_target_state.
+		 */
+		if (patch_state == KLP_TRANSITION_IDLE)
+			current->patch_state = klp_target_state;
 
 		if (patch_state == KLP_TRANSITION_UNPATCHED) {
 			/*

Note: It is broken. It sets current->patch_state but it later
      checks the local variable "patch_state" which is still
      KLP_TRANSITION_IDLE.

But is is safe when we fix it?

It might be as long as we run klp_synchronize_transition() between
updating the global @klp_target_state and the other operations.

Especially, we need to make sure that @klp_target_state always have
either KLP_TRANSITION_PATCHED or KLP_TRANSITION_UNPATCHED when
func->transition is set.

For this, we would need to add klp_synchronize_transition() into

  + klp_init_transition() between setting @klp_target_state
    and func->transition = true.

  + klp_complete_transition() also for KLP_TRANSITION_UNPATCHED way.
    It is currently called only for the PATCHED target state.

Will this be enough?

FACT: It is more complicated than it looked.
QUESTION: Is this worth the effort?

NOTE: The rcu_read_lock() does not solve the reported stall.
      We are spending time on it only because it looked nice and
      simple to you.

My opinion: I would personally prefer to focus on solving
	the stall and the use-after-free access in do_exit().

Best Regards,
Petr

Re: [PATCH v3 2/2] livepatch: Replace tasklist_lock with RCU

[Yafang Shao]

On Fri, Feb 28, 2025 at 12:22 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Thu 2025-02-27 10:47:33, Yafang Shao wrote:
> > The tasklist_lock in the KLP transition might result high latency under
> > specific workload. We can replace it with RCU.
> >
> > After a new task is forked, its kernel stack is always set to empty[0].
> > Therefore, we can init these new tasks to KLP_TRANSITION_IDLE state
> > after they are forked. If these tasks are forked during the KLP
> > transition but before klp_check_and_switch_task(), they can be safely
> > skipped during klp_check_and_switch_task(). Additionally, if
> > klp_ftrace_handler() is triggered right after forking, the task can
> > determine which function to use based on the klp_target_state.
> >
> > With the above change, we can safely convert the tasklist_lock to RCU.
> >
> > Link: https://lore.kernel.org/all/20250213173253.ovivhuq2c5rmvkhj@jpoimboe/ [0]
> > Link: https://lore.kernel.org/all/20250214181206.xkvxohoc4ft26uhf@jpoimboe/ [1]
> > Suggested-by: Josh Poimboeuf <jpoimboe@kernel.org>
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> > --- a/kernel/livepatch/patch.c
> > +++ b/kernel/livepatch/patch.c
> > @@ -95,7 +95,13 @@ static void notrace klp_ftrace_handler(unsigned long ip,
> >
> >               patch_state = current->patch_state;
> >
> > -             WARN_ON_ONCE(patch_state == KLP_TRANSITION_IDLE);
> > +             /* If the patch_state is KLP_TRANSITION_IDLE, it means the task
> > +              * was forked after klp_init_transition(). In this case, the
> > +              * newly forked task can determine which function to use based
> > +              * on the klp_target_state.
> > +              */
> > +             if (patch_state == KLP_TRANSITION_IDLE)
> > +                     patch_state = klp_target_state;
> >
>

I'm a bit confused by your example. Let me break it down to ensure I
understand correctly:

> CPU0                            CPU1
>
> task_0 (forked process):
>
> funcA
>   klp_ftrace_handler()
>     if (patch_state == KLP_TRANSITION_IDLE)
>        patch_state = klp_target_state
>         # set to KLP_TRANSITION_PATCHED
>
>   # redirected to klp_funcA()

It seems that the KLP  is still in the process of transitioning, right?

>
>
>                                 echo 0 >/sys/kernel/livepatch/patch1/enabled
>
>                                 klp_reverse_transition()
>
>                                   klp_target_state = !klp_target_state;
>                                     # set to KLP_TRANSITION_UNPATCHED

If you attempt to initiate a new transition while the current one is
still ongoing, the __klp_disable_patch function will return -EBUSY,
correct?

__klp_disable_patch
    if (klp_transition_patch)
        return -EBUSY;

On the other hand, if klp_transition_patch is NULL, it indicates that
the first KLP transition has completed successfully.

>
>
>    funcB
>      if (patch_state == KLP_TRANSITION_IDLE)
>        patch_state = klp_target_state
>          # set to KLP_TRANSITION_UNPATCHED
>
>    # staying in origianl funcB
>
>
> BANG: livepatched and original function called at the same time
>
>       => broken consistency model.
>
> BTW: This is what I tried to warn you about at
>      https://lore.kernel.org/r/Z69Wuhve2vnsrtp_@pathway.suse.cz
>
> See below for more:
>
> >               if (patch_state == KLP_TRANSITION_UNPATCHED) {
> >                       /*
> > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> > index ba069459c101..af76defca67a 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_TRANSITION_IDLE;
> > +int klp_target_state = KLP_TRANSITION_IDLE;
> >
> >  static unsigned int klp_signals_cnt;
> >
> > @@ -294,6 +294,14 @@ static int klp_check_and_switch_task(struct task_struct *task, void *arg)
> >  {
> >       int ret;
> >
> > +     /*
> > +      * If the patch_state remains KLP_TRANSITION_IDLE at this point, it
> > +      * indicates that the task was forked after klp_init_transition().
> > +      * In this case, it is safe to skip the task.
> > +      */
> > +     if (!test_tsk_thread_flag(task, TIF_PATCH_PENDING))
> > +             return 0;
> > +
> >       if (task_curr(task) && task != current)
> >               return -EBUSY;
> >
> > @@ -466,11 +474,11 @@ void klp_try_complete_transition(void)
> >        * Usually this will transition most (or all) of the tasks on a system
> >        * unless the patch includes changes to a very common function.
> >        */
> > -     read_lock(&tasklist_lock);
> > +     rcu_read_lock();
> >       for_each_process_thread(g, task)
> >               if (!klp_try_switch_task(task))
> >                       complete = false;
> > -     read_unlock(&tasklist_lock);
> > +     rcu_read_unlock();
> >
> >       /*
> >        * Ditto for the idle "swapper" tasks.
> > @@ -694,25 +702,10 @@ void klp_reverse_transition(void)
> >  }
> >
> >  /* Called from copy_process() during fork */
> > -void klp_copy_process(struct task_struct *child)
> > +void klp_init_process(struct task_struct *child)
> >  {
> > -
> > -     /*
> > -      * The parent process may have gone through a KLP transition since
> > -      * the thread flag was copied in setup_thread_stack earlier. Bring
> > -      * the task flag up to date with the parent here.
> > -      *
> > -      * The operation is serialized against all klp_*_transition()
> > -      * operations by the tasklist_lock. The only exceptions are
> > -      * klp_update_patch_state(current) and __klp_sched_try_switch(), but we
> > -      * cannot race with them because we are current.
> > -      */
> > -     if (test_tsk_thread_flag(current, TIF_PATCH_PENDING))
> > -             set_tsk_thread_flag(child, TIF_PATCH_PENDING);
> > -     else
> > -             clear_tsk_thread_flag(child, TIF_PATCH_PENDING);
> > -
> > -     child->patch_state = current->patch_state;
> > +     clear_tsk_thread_flag(child, TIF_PATCH_PENDING);
> > +     child->patch_state = KLP_TRANSITION_IDLE;
>
> I thought that we might do:
>
>         child->patch_state = klp_target_state;
>
> to avoid the race in the klp_ftrace_handler() described above.
>
> But the following might happen:
>
> CPU0                            CPU1
>
> klp_init_process(child)
>
>   child->patch_state = KLP_TRANSITION_IDLE
>
>                                 klp_enable_patch()
>                                   # setup ftrace handlers
>                                   # initialize all processes
>                                   # in the task list
>
>   # add "child" into the task list
>
>   schedule()
>
> [running child now]
>
> funcA()
>
>   klp_ftrace_handler()
>
>     child->patch_state = KLP_TRANSITION_IDLE
>
> BANG: Same problem as with the original patch.
>
>
> Hmm, the 2nd version of this patchset tried to do:
>
> diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> index 90408500e5a3..5e523a3fbb3c 100644
> --- a/kernel/livepatch/patch.c
> +++ b/kernel/livepatch/patch.c
> @@ -95,7 +95,12 @@ static void notrace klp_ftrace_handler(unsigned long ip,
>
>                 patch_state = current->patch_state;
>
> -               WARN_ON_ONCE(patch_state == KLP_TRANSITION_IDLE);
> +               /* If the patch_state is KLP_TRANSITION_IDLE, it indicates the
> +                * task was forked after klp_init_transition(). For this newly
> +                * forked task, it is safe to switch it to klp_target_state.
> +                */
> +               if (patch_state == KLP_TRANSITION_IDLE)
> +                       current->patch_state = klp_target_state;
>
>                 if (patch_state == KLP_TRANSITION_UNPATCHED) {
>                         /*
>
> Note: It is broken. It sets current->patch_state but it later
>       checks the local variable "patch_state" which is still
>       KLP_TRANSITION_IDLE.

You're right—Josh has already highlighted this. I overlooked the local variable.

>
> But is is safe when we fix it?
>
> It might be as long as we run klp_synchronize_transition() between
> updating the global @klp_target_state and the other operations.
>
> Especially, we need to make sure that @klp_target_state always have
> either KLP_TRANSITION_PATCHED or KLP_TRANSITION_UNPATCHED when
> func->transition is set.
>
> For this, we would need to add klp_synchronize_transition() into
>
>   + klp_init_transition() between setting @klp_target_state
>     and func->transition = true.
>
>   + klp_complete_transition() also for KLP_TRANSITION_UNPATCHED way.
>     It is currently called only for the PATCHED target state.
>
> Will this be enough?
>
> FACT: It is more complicated than it looked.
> QUESTION: Is this worth the effort?
>
> NOTE: The rcu_read_lock() does not solve the reported stall.
>       We are spending time on it only because it looked nice and
>       simple to you.

It can help avoid contention on the tasklist_lock. We often encounter
latency spikes caused by lock contention, but identifying the root
cause isn't always straightforward. If we can eliminate this global
lock, I believe it’s a worthwhile improvement.

>
> My opinion: I would personally prefer to focus on solving
>         the stall and the use-after-free access in do_exit().

These are distinct issues, and I believe it would be more effective to
discuss them separately.

-- 
Regards
Yafang

Re: [PATCH v3 2/2] livepatch: Replace tasklist_lock with RCU

[Yafang Shao]

On Fri, Feb 28, 2025 at 10:38 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Fri, Feb 28, 2025 at 12:22 AM Petr Mladek <pmladek@suse.com> wrote:
> >
> > On Thu 2025-02-27 10:47:33, Yafang Shao wrote:
> > > The tasklist_lock in the KLP transition might result high latency under
> > > specific workload. We can replace it with RCU.
> > >
> > > After a new task is forked, its kernel stack is always set to empty[0].
> > > Therefore, we can init these new tasks to KLP_TRANSITION_IDLE state
> > > after they are forked. If these tasks are forked during the KLP
> > > transition but before klp_check_and_switch_task(), they can be safely
> > > skipped during klp_check_and_switch_task(). Additionally, if
> > > klp_ftrace_handler() is triggered right after forking, the task can
> > > determine which function to use based on the klp_target_state.
> > >
> > > With the above change, we can safely convert the tasklist_lock to RCU.
> > >
> > > Link: https://lore.kernel.org/all/20250213173253.ovivhuq2c5rmvkhj@jpoimboe/ [0]
> > > Link: https://lore.kernel.org/all/20250214181206.xkvxohoc4ft26uhf@jpoimboe/ [1]
> > > Suggested-by: Josh Poimboeuf <jpoimboe@kernel.org>
> > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > > ---
> > > --- a/kernel/livepatch/patch.c
> > > +++ b/kernel/livepatch/patch.c
> > > @@ -95,7 +95,13 @@ static void notrace klp_ftrace_handler(unsigned long ip,
> > >
> > >               patch_state = current->patch_state;
> > >
> > > -             WARN_ON_ONCE(patch_state == KLP_TRANSITION_IDLE);
> > > +             /* If the patch_state is KLP_TRANSITION_IDLE, it means the task
> > > +              * was forked after klp_init_transition(). In this case, the
> > > +              * newly forked task can determine which function to use based
> > > +              * on the klp_target_state.
> > > +              */
> > > +             if (patch_state == KLP_TRANSITION_IDLE)
> > > +                     patch_state = klp_target_state;
> > >
> >
>
> I'm a bit confused by your example. Let me break it down to ensure I
> understand correctly:
>
> > CPU0                            CPU1
> >
> > task_0 (forked process):
> >
> > funcA
> >   klp_ftrace_handler()
> >     if (patch_state == KLP_TRANSITION_IDLE)
> >        patch_state = klp_target_state
> >         # set to KLP_TRANSITION_PATCHED
> >
> >   # redirected to klp_funcA()
>
> It seems that the KLP  is still in the process of transitioning, right?
>
> >
> >
> >                                 echo 0 >/sys/kernel/livepatch/patch1/enabled
> >
> >                                 klp_reverse_transition()
> >
> >                                   klp_target_state = !klp_target_state;
> >                                     # set to KLP_TRANSITION_UNPATCHED
>
> If you attempt to initiate a new transition while the current one is
> still ongoing, the __klp_disable_patch function will return -EBUSY,
> correct?
>
> __klp_disable_patch
>     if (klp_transition_patch)
>         return -EBUSY;
>
> On the other hand, if klp_transition_patch is NULL, it indicates that
> the first KLP transition has completed successfully.

Please disregard my previous comment.
I missed the fact that it is called from klp_reverse_transition().
I will review your example again and give it more thought.

-- 
Regards
Yafang

Re: [PATCH v3 2/2] livepatch: Replace tasklist_lock with RCU

[Miroslav Benes]

fwiw,

> +	/*
> +	 * If the patch_state remains KLP_TRANSITION_IDLE at this point, it
> +	 * indicates that the task was forked after klp_init_transition().
> +	 * In this case, it is safe to skip the task.
> +	 */
> +	if (!test_tsk_thread_flag(task, TIF_PATCH_PENDING))
> +		return 0;

  if (!klp_patch_pending(task))

Miroslav

Re: [PATCH v3 1/2] livepatch: Add comment to clarify klp_add_nops()

[Petr Mladek]

On Thu 2025-02-27 15:45:57, Petr Mladek wrote:
> On Thu 2025-02-27 10:47:32, Yafang Shao wrote:
> > Add detailed comments to clarify the purpose of klp_add_nops() function.
> > These comments are based on Petr's explanation[0].
> > 
> > Link: https://lore.kernel.org/all/Z6XUA7D0eU_YDMVp@pathway.suse.cz/ [0]
> > Suggested-by: Petr Mladek <pmladek@suse.com>
> > Suggested-by: Josh Poimboeuf <jpoimboe@kernel.org>
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>

JFYI, I have pushed this patch into livepatching.git,
branch for-6.15/trivial.

It is a trivial patch and is independent on the other problems.

Best Regards,
Petr