Re: [RFC kgr on klp 9/9] livepatch: send a fake signal to all tasks

[Miroslav Benes <mbenes@suse.cz>]

Hi,

On Mon, 4 May 2015, Oleg Nesterov wrote:

> Well, I can't really comment this change because I didn't see other
> changes, and I do not know what klp_kgraft_task_in_progress() means...
> 
> On 05/04, Jiri Slaby wrote:
> >
> > Luckily we can force the task to do that by sending it a fake signal,
> 
> But note that signal_wake_up(0) won't wake the stopped/traced tasks up.

Yes, this could happen. Such process would prevent the patching to 
finish, but that should not be an issue for patching as such. The 
process's flag would be eventually cleared.

> > +static void klp_kgraft_send_fake_signal(void)
> > +{
> > +	struct task_struct *p;
> > +	unsigned long flags;
> > +
> > +	read_lock(&tasklist_lock);
> > +	for_each_process(p) {
> 
> Only the group leader can be klp_kgraft_task_in_progress?
> 
> Looks like you need for_each_process_thread()...

Thanks for spotting. This is consistent with other places in the code and 
needs to be fixed.

> > +		/*
> > +		 * send fake signal to all non-kthread processes which are still
> > +		 * not migrated
> > +		 */
> > +		if (!(p->flags & PF_KTHREAD) &&
> 
> So this can miss the execing kernel thread, I do not know if this is
> correct or not. PF_KTHREAD is cleared in flush_old_exec().

Correct, we do not deal with kthreads in this RFC yet. There is more work 
to do it correctly. See changelogs and comments in other patches.

> > +		    klp_kgraft_task_in_progress(p) &&
> > +		    lock_task_sighand(p, &flags)) {
> 
> No need for lock_task_sighand(). Just spin_lock_irq(p->sighand->siglock).
> tasklist_lock + for_each_process guarantees that "p" has a valid ->sighand.

Ah, thank you.

> 
> > +			signal_wake_up(p, 0);
> 
> To remind, this won't wakeup a TASK_STOPPED/TRACED thread.
> 
> >  void recalc_sigpending(void)
> >  {
> > -	if (!recalc_sigpending_tsk(current) && !freezing(current))
> > +	if (!recalc_sigpending_tsk(current) && !freezing(current) &&
> > +	    !klp_kgraft_task_in_progress(current))
> >  		clear_thread_flag(TIF_SIGPENDING);
> 
> It is not clear from this patch when TIF_SIGPENDING will be cleared.
> 
> I assume other changes add some hooks into do_notify_resume/get_signal
> paths, otherwise a klp_kgraft_task_in_progress() will spin until
> klp_kgraft_task_in_progress(current) becomes "false".

That is correct. The flag is cleared in do_notify_resume path and also in 
syscall_trace_enter_phase1. See patch number 4 of this RFC.

Thanks a lot for the feedback
Miroslav