2017-11-20 15:58:12

by Miroslav Benes

[permalink] [raw]
Subject: Re: [PATCH v4 2/2] livepatch: force transition to finish

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 <[email protected]>

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