From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S937327AbdEXNGY (ORCPT <rfc822;w@1wt.eu>);
        Wed, 24 May 2017 09:06:24 -0400
Received: from mx2.suse.de ([195.135.220.15]:53432 "EHLO mx1.suse.de"
        rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP
        id S1035434AbdEXNGM (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Wed, 24 May 2017 09:06:12 -0400
Date: Wed, 24 May 2017 15:06:10 +0200
From: Petr Mladek <pmladek@suse.com>
To: Miroslav Benes <mbenes@suse.cz>
Cc: jpoimboe@redhat.com, jeyu@redhat.com, jikos@kernel.org,
        live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/3] livepatch: force transition process to finish
Message-ID: <20170524130610.GK7297@pathway.suse.cz>
References: <20170518120043.7205-1-mbenes@suse.cz>
 <20170518120043.7205-4-mbenes@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170518120043.7205-4-mbenes@suse.cz>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu 2017-05-18 14:00:43, Miroslav Benes wrote:
> If a task sleeps in a set of patched functions uninterruptibly, it could
> block the whole transition process 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 2 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! Use wisely. Admin must be sure that it is safe to
> execute such action. This means that it must be checked that by doing so
> the consistency model guarantees are not violated.
> 
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index bb61aaa196d3..d057a34510e6 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -591,3 +591,19 @@ void klp_send_fake_signal(void)
>  	}
>  	read_unlock(&tasklist_lock);
>  }
> +
> +/*
> + * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an
> + * existing transition to finish.
> + */
> +void klp_unmark_tasks(void)
> +{
> +	struct task_struct *g, *task;
> +
> +	pr_warn("all tasks marked as migrated on admin's request\n");
> +
> +	read_lock(&tasklist_lock);
> +	for_each_process_thread(g, task)
> +		klp_update_patch_state(task);
> +	read_unlock(&tasklist_lock);

This should get called under klp_mutex. The following race comes to my mind:

CPU0:					CPU1:

klp_transition_work_fn()
  klp_try_complete_transition()
    for_each_process()
	if (!klp_try_switch_task(task))

	# success

   klp_complete_transition()

     for_each_process()
	task->patch_state = KLP_UNDEFINED;


					klp_unmark_tasks()
					  for_each_process()
					    klp_update_patch_state()
					      task->patch_state =
						klp_target_state;

	klp_target_state = KLP_UNDEFINED;

=> CPU1 might happily set an obsolete state and create a mess.

It would be possible to solve this by reodering, barriers.
But much better solution seems to serialize both actions
using klp_mutex.

In fact, I would suggest to take klp_mutex in force_store()
and do all actions synchronously, including the check
of klp_transition_patch.

Best Regards,
Petr

PS: I know that I talked about this with Mirek and suggested
doing the check for klp_transition_patch without the lock.
It made perfect sense. But I have changed my mind when
seeing the final code.