linux-kernel - Re: [PATCH 3/3] livepatch: force transition process to finish

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LSU.2.20.1705241610460.26925@pobox.suse.cz>
Date:   Wed, 24 May 2017 16:15:49 +0200 (CEST)
From:   Miroslav Benes <mbenes@...e.cz>
To:     Petr Mladek <pmladek@...e.com>
cc:     jpoimboe@...hat.com, jeyu@...hat.com, jikos@...nel.org,
        live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] livepatch: force transition process to finish

On Wed, 24 May 2017, Petr Mladek wrote:

> 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.

This should not happen. klp_update_patch_state() use 
test_and_clear_tsk_thread_flag(task, TIF_PATCH_PENDING) and only if true, 
task->patch_state is set.

And all TIF_PATCH_PENDING are cleared when you get 
klp_complete_transition().
 
> 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.

I still think it is better not do it. klp_unmark_tasks() does nothing else 
than tasks already do. They call klp_update_patch_state() by themselves 
and they do not grab klp_mutex lock for doing that. klp_unmark_tasks() 
only forces this action.

On the other hand, I do not see a problem in doing that. We already have a 
relationship between klp_mutex and tasklist_lock defined elsewhere, so it 
is safe. It would only serialize things needlessly.

Thanks,
Miroslav