Subject: Re: [PATCH v4.1 2/2] livepatch: force transition to finish
To: Miroslav Benes <mbenes@suse.cz>, 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
References: <nycvar.YFH.7.76.1711210009590.32604@cbobk.fhfr.pm>
 <20171122102921.12289-1-mbenes@suse.cz>
From: Jason Baron <jbaron@akamai.com>
Message-ID: <70454f58-3d33-f305-e1df-5f9dc43222c7@akamai.com>
Date: Fri, 15 Dec 2017 13:04:05 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <20171122102921.12289-1-mbenes@suse.cz>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3709
Lines: 99

On 11/22/2017 05:29 AM, 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. Removal (rmmod) of
> patch modules is permanently disabled when the feature is used. It
> cannot be guaranteed there is no task sleeping in such module.
> 
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> ---
>  Documentation/ABI/testing/sysfs-kernel-livepatch | 14 ++++++++++
>  Documentation/livepatch/livepatch.txt            | 18 ++++++++++--
>  kernel/livepatch/core.c                          | 30 ++++++++++++++++++++
>  kernel/livepatch/transition.c                    | 35 +++++++++++++++++++++++-
>  kernel/livepatch/transition.h                    |  1 +
>  5 files changed, 95 insertions(+), 3 deletions(-)

....

> +
> +/*
> + * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an
> + * existing transition to finish.
> + *
> + * NOTE: klp_update_patch_state(task) requires the task to be inactive or
> + * 'current'. This is not the case here and the consistency model could be
> + * broken. Administrator, who is the only one to execute the
> + * klp_force_transitions(), has to be aware of this.
> + */
> +void klp_force_transition(void)
> +{
> +	struct task_struct *g, *task;
> +	unsigned int cpu;
> +
> +	pr_warn("forcing remaining tasks to the patched state\n");
> +
> +	read_lock(&tasklist_lock);
> +	for_each_process_thread(g, task)
> +		klp_update_patch_state(task);
> +	read_unlock(&tasklist_lock);
> +
> +	for_each_possible_cpu(cpu)
> +		klp_update_patch_state(idle_task(cpu));
> +
> +	klp_forced = true;
> +}

I had a question on this bit. If say cpu 0 executes
klp_force_transition(void), right up until klp_forced is set to true,
and then cpu 1 does klp_complete_transition() (since all threads have
the correct state), wouldn't it be possible then for
klp_complete_transition() to not see klp_forced set to true, and thus
the module could be potentially removed even though it was forced?

If so, I think that the force path just needs to be set before the
threads are updated (as below). I don't think that the
klp_complete_transition() needs the corresponding rmb, b/c there is
sufficient ordering there already (although it would deserve a comment).

Thanks,

-Jason

diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index be5bfa5..cca6a3a 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -671,6 +671,15 @@ void klp_force_transition(void)

        pr_warn("forcing remaining tasks to the patched state\n");

+       klp_forced = true;
+
+       /*
+        * ensure that if klp_complete_transition() sees that all
+        * the threads have been updated to desired task->patch_state
+        * that we also see klp_forced = true;
+        */
+       smp_wmb();
+
        read_lock(&tasklist_lock);
        for_each_process_thread(g, task)
                klp_update_patch_state(task);
@@ -678,6 +687,4 @@ void klp_force_transition(void)

        for_each_possible_cpu(cpu)
                klp_update_patch_state(idle_task(cpu));
-
-       klp_forced = true;
 }