Date: Thu, 21 Dec 2017 15:58:12 +0100
From: Petr Mladek <pmladek@suse.com>
To: Miroslav Benes <mbenes@suse.cz>
Cc: jpoimboe@redhat.com, jeyu@kernel.org, jikos@kernel.org,
        linux-kernel@vger.kernel.org, live-patching@vger.kernel.org,
        jbaron@akamai.com
Subject: Re: [PATCH 1/2] livepatch: Remove immediate feature
Message-ID: <20171221145812.ijs4epzvwbbthxk5@pathway.suse.cz>
References: <20171208172523.12150-1-mbenes@suse.cz>
 <20171208172523.12150-2-mbenes@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20171208172523.12150-2-mbenes@suse.cz>
User-Agent: NeoMutt/20170421 (1.8.2)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4361
Lines: 108

Hello,

it seems that we are going to use this patch (I agree). Therefore
I am going to review the content.

On Fri 2017-12-08 18:25:22, Miroslav Benes wrote:
> immediate flag has been used to disable per-task consistency and patch
> all tasks immediately. It could be useful if the patch doesn't change any
> function or data semantics.
> 
> However, it causes problems on its own. The consistency problem is
> currently broken with respect to immediate patches.
> 
> func            a
> patches         1i
>                 2i
>                 3
> 
> When the patch 3 is applied, only 2i function is checked (by stack
> checking facility). There might be a task sleeping in 1i though. Such
> task is migrated to 3, because we do not check 1i in
> klp_check_stack_func() at all.
> 
> Coming atomic replace feature would be easier to implement and more
> reliable without immediate.
> 
> Moreover, the fake signal and force feature give us almost the same
> benefits and the user can decide to use them in problematic situations
> (while immediate needs to be set before the patch is applied). It is
> also more isolated in terms of code.
> 
> Thus, remove immediate feature completely and save us from the problems.

Just for record, the above paragraphs needs to be reworded because the
problem still will be there with the force feature.

> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 1c3c9b27c916..461c0b7dc913 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -367,10 +367,10 @@ static int __klp_enable_patch(struct klp_patch *patch)
>  	 * A reference is taken on the patch module to prevent it from being
>  	 * unloaded.
>  	 *
> -	 * Note: For immediate (no consistency model) patches we don't allow
> -	 * patch modules to unload since there is no safe/sane method to
> -	 * determine if a thread is still running in the patched code contained
> -	 * in the patch module once the ftrace registration is successful.
> +	 * Note: When klp_forced is set we don't allow patch modules to unload
> +	 * since there is no safe/sane method to determine if a thread is still
> +	 * running in the patched code contained in the patch module once the
> +	 * ftrace registration is successful.

I would remove this paragraph completely. You removed the
cross-reference klp_complete_transition() as well.

>  	 */
>  	if (!try_module_get(patch->mod))
>  		return -ENODEV;
> @@ -890,12 +890,7 @@ int klp_register_patch(struct klp_patch *patch)
>  	if (!klp_initialized())
>  		return -ENODEV;
>  
> -	/*
> -	 * Architectures without reliable stack traces have to set
> -	 * patch->immediate because there's currently no way to patch kthreads
> -	 * with the consistency model.
> -	 */
> -	if (!klp_have_reliable_stack() && !patch->immediate) {
> +	if (!klp_have_reliable_stack()) {
>  		pr_err("This architecture doesn't have support for the livepatch consistency model.\n");
>  		return -ENOSYS;
>  	}

> diff --git a/samples/livepatch/livepatch-callbacks-demo.c b/samples/livepatch/livepatch-callbacks-demo.c
> index 3d115bd68442..bda7f3841f3e 100644
> --- a/samples/livepatch/livepatch-callbacks-demo.c
> +++ b/samples/livepatch/livepatch-callbacks-demo.c
> @@ -197,20 +197,8 @@ static int livepatch_callbacks_demo_init(void)
>  {
>  	int ret;
>  
> -	if (!klp_have_reliable_stack() && !patch.immediate) {
> -		/*
> -		 * WARNING: Be very careful when using 'patch.immediate' in
> -		 * your patches.  It's ok to use it for simple patches like
> -		 * this, but for more complex patches which change function
> -		 * semantics, locking semantics, or data structures, it may not
> -		 * be safe.  Use of this option will also prevent removal of
> -		 * the patch.
> -		 *
> -		 * See Documentation/livepatch/livepatch.txt for more details.
> -		 */
> -		patch.immediate = true;
> -		pr_notice("The consistency model isn't supported for your architecture.  Bypassing safety mechanisms and applying the patch immediately.\n");
> -	}
> +	if (!klp_have_reliable_stack())
> +		pr_notice("The consistency model isn't supported for your architecture. The transition may not finish.\n");

The notice is redundant. The klp_registrer_patch() would printk
similar message and return -ENOSYS.

Same is true for the other sample modules.

In each case, I like this patch. It simplifies the code a lot.

Best Regards,
Petr