linux-kernel - Re: [PATCH v2 2/2] livepatch: Replace tasklist

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CALOAHbDcKoO4Wicva_qtNy4fNyS+ey7_PybbmXSk_xhKM=ZG=A@mail.gmail.com>
Date: Wed, 26 Feb 2025 11:46:50 +0800
From: Yafang Shao <laoar.shao@...il.com>
To: Josh Poimboeuf <jpoimboe@...nel.org>
Cc: jikos@...nel.org, mbenes@...e.cz, pmladek@...e.com, 
	joe.lawrence@...hat.com, live-patching@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] livepatch: Replace tasklist_lock with RCU

On Wed, Feb 26, 2025 at 2:33 AM Josh Poimboeuf <jpoimboe@...nel.org> wrote:
>
> On Sun, Feb 23, 2025 at 02:20:46PM +0800, Yafang Shao wrote:
> > +++ 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;
>
> Hm, but then the following line is:
>
> >               if (patch_state == KLP_TRANSITION_UNPATCHED) {
>
> Shouldn't the local 'patch_state' variable be updated?

Ah, I missed it.

>
> It also seems unnecessary to update 'current->patch_state' here.

Got it.

>
> > @@ -294,6 +294,13 @@ 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(). For
> > +      * this newly forked task, it is now safe to perform the switch.
> > +      */
> > +     if (task->patch_state == KLP_TRANSITION_IDLE)
> > +             goto out;
> > +
>
> This also seems unnecessary.  No need to transition the patch if the
> ftrace handler is already doing the right thing.  klp_try_switch_task()
> can just return early on !TIF_PATCH_PENDING.

Good suggestion.

>
> > @@ -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();
>
> Can this also be done for the idle tasks?

The cpus_read_lock() around the idle tasks is in place to protect
against CPU hotplug operations. If we aim to eliminate this lock
during the KLP transition, the CPU hotplug logic would need to be
adjusted accordingly. For instance, we would need to address how to
handle wake_up_if_idle() when a CPU is in the process of hotplugging.

Given that the number of CPUs is unlikely to be large enough to create
a bottleneck in the current implementation, optimizing this mechanism
may not be a priority at the moment. It might be more practical to
address this issue at a later stage.


--
Regards
Yafang