Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 20 Nov 2017 16:57:19 +0100 (CET)
From:   Miroslav Benes <mbenes@suse.cz>
To:     jpoimboe@redhat.com, jeyu@kernel.org, jikos@kernel.org
cc:     pmladek@suse.com, lpechacek@suse.cz, pavel@ucw.cz,
        live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 2/2] livepatch: force transition to finish
In-Reply-To: <20171115135014.20594-3-mbenes@suse.cz>
Message-ID: <alpine.LSU.2.21.1711201647300.25182@pobox.suse.cz>
References: <20171115135014.20594-1-mbenes@suse.cz> <20171115135014.20594-3-mbenes@suse.cz>
User-Agent: Alpine 2.21 (LSU 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 15 Nov 2017, Miroslav Benes wrote:

> If a task sleeps in a set of patched functions uninterruptedly, it could
> block the whole transition indefinitely.  Thus it may be useful to clear
> its TIF_PATCH_PENDING to allow the process to finish.
> 
> Admin can do that now by writing to force sysfs attribute in livepatch
> sysfs directory. TIF_PATCH_PENDING is then cleared for all tasks and the
> transition can finish successfully.
> 
> Important note! Administrator should not use this feature without a
> clearance from a patch distributor. It must be checked that by doing so
> the consistency model guarantees are not violated.
> 
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>

While working on "immediate" removal, I realized we had the similar 
problem here with modules removal. There is no way out of the rabbit hole.

If a patch is forced, we obviously cannot say there is no task sleeping in 
the old code. This could be disastrous if such old module is then removed 
(either we disabled it and we want to rmmod it, or there is a new "atomic 
replace" patch and we want to remove the old one).

We need something like the following (at least as a starting point)

diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 566ab210853f..df4f2bbd9731 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -33,6 +33,8 @@ struct klp_patch *klp_transition_patch;
 
 static int klp_target_state = KLP_UNDEFINED;
 
+static bool klp_forced = false;
+
 /*
  * This work can be performed periodically to finish patching or unpatching any
  * "straggler" tasks which failed to transition in the first attempt.
@@ -109,7 +111,7 @@ static void klp_complete_transition(void)
                }
        }
 
-       if (klp_target_state == KLP_UNPATCHED && !immediate_func)
+       if (klp_target_state == KLP_UNPATCHED && !klp_forced && !immediate_func)
                module_put(klp_transition_patch->mod);
 
        /* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */
@@ -642,4 +644,6 @@ void klp_force_transition(void)
 
        for_each_possible_cpu(cpu)
                klp_update_patch_state(idle_task(cpu));
+
+       klp_forced = true;
 }


It is still better than immediate, because it is a "ex post" action. We 
can also try to improve later. We could remember all forced tasks and 
reenable rmmod once those tasks are really migrated ("shadow migration"). 

Thoughts are really welcome here.

Miroslav

From 1584140406062067527@xxx Wed Nov 15 13:53:38 +0000 2017
X-GM-THRID: 1584140406062067527
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread