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

[Miroslav Benes]

On Tue, 23 May 2017, Josh Poimboeuf wrote:

> On Thu, May 18, 2017 at 02:00:43PM +0200, 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.
> > 
> > Signed-off-by: Miroslav Benes <mbenes@xxxxxxx>
> 
> These patches look good to me.  Just some minor comments.
> 
> > ---
> >  include/linux/livepatch.h     |  1 +
> >  kernel/livepatch/core.c       |  3 +++
> >  kernel/livepatch/transition.c | 16 ++++++++++++++++
> >  kernel/livepatch/transition.h |  1 +
> >  4 files changed, 21 insertions(+)
> > 
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > index 43cfeebeb42b..b567208a1c6e 100644
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -31,6 +31,7 @@
> >  
> >  /* values for sysfs force attribute */
> >  #define KLP_FORCE_FAKE		1
> > +#define KLP_FORCE_UNMARK	2
> >  
> >  /* task patch states */
> >  #define KLP_UNDEFINED	-1
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index bb3b78fa7d2b..9bc1103348c9 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -469,6 +469,9 @@ static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
> >  	case KLP_FORCE_FAKE:
> >  		klp_send_fake_signal();
> >  		break;
> > +	case KLP_FORCE_UNMARK:
> > +		klp_unmark_tasks();
> > +		break;
> 
> I think the naming could be a little clearer, and more consistent.  What
> do you think about:
> 
> KLP_FORCE_SIGNALS -> klp_force_signals()
> KLP_FORCE_TRANSITIONS -> klp_force_transitions()

Yes, that is better.
 
> >  	default:
> >  		return -EINVAL;
> >  	}
> > 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");
> 
> The user might not know what migrated means.  How about "forcing
> remaining tasks to the patched state" or something similar?

Ok.

> > +
> > +	read_lock(&tasklist_lock);
> > +	for_each_process_thread(g, task)
> > +		klp_update_patch_state(task);
> > +	read_unlock(&tasklist_lock);
> 
> So klp_update_patch_state() has the following comment:
> 
>  * NOTE: If task is not 'current', the caller must ensure the task is inactive.
>  * Otherwise klp_ftrace_handler() might read the wrong 'patch_state' value.
> 
> This code doesn't ensure the task is inactive.  But I think that's ok as
> long as we document the fact that this could break the consistency
> model, right?

Correct. I'll add a comment to klp_unmark_tasks()/klp_force_transitions().

> On a related note, I think the new sysfs entry should also be documented
> in Documentation/livepatch/livepatch.txt somewhere.

No problem.

Thanks,
Miroslav