Re: [PATCH v10 06/10] livepatch: Add atomic replace

[Petr Mladek]

On Tue 2018-03-20 16:26:19, Josh Poimboeuf wrote:
> On Tue, Mar 20, 2018 at 03:35:01PM +0100, Petr Mladek wrote:
> > On Tue 2018-03-13 17:48:04, Josh Poimboeuf wrote:
> > > On Wed, Mar 07, 2018 at 09:20:35AM +0100, Petr Mladek wrote:
> > > > This patch adds a new "replace" flag to struct klp_patch. When it is
> > > > enabled, a set of 'nop' klp_func will be dynamically created for all
> > > > functions that are already being patched but that will no longer be
> > > > modified by the new patch. They are temporarily used as a new target
> > > > during the patch transition.
> > > > 
> > > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > > > index fd0296859ff4..ad508a86b2f9 100644
> > > > --- a/kernel/livepatch/core.c
> > > > +++ b/kernel/livepatch/core.c
> > > > +static int klp_add_nops(struct klp_patch *patch)
> > > > +{
> > > > +	struct klp_patch *old_patch;
> > > > +	struct klp_object *old_obj;
> > > > +	int err = 0;
> > > > +
> > > > +	if (WARN_ON(!patch->replace))
> > > > +		return -EINVAL;
> > > 
> > > IMO, this is another one of those overly paranoid warnings that isn't
> > > really needed.  Why would we call klp_add_nops() for a non-replace
> > > patch?
> > 
> > Just to be sure. What is the difference, for example, against the following
> > checks in __klp_enable_patch() from your point of view, please?
> > 
> > 	if (klp_transition_patch)
> > 		return -EBUSY;
> > 
> > 	if (WARN_ON(patch->enabled))
> > 		return -EINVAL;
> > 
> > One difference is that klp_enable_patch() is exported symbol. One the
> > other hand, livepatch code developers could do mistakes as well.
> > Adding nops sounds like an innoncent operation after all ;-)
> 
> But klp_enable_patch() being an exported symbol is an important
> difference.  It catches a patch author abusing the interface.  Which is
> much more likely than one of us accidentally calling klp_add_nops().
> Have you not noticed how thorough our code reviews are? ;-)
> 
> Anyway, I suppose it's a harmless check and I don't feel very strongly
> about it, it just seems unnecessary.

I have removed the check.


> > > > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> > > > index 6917100fbe79..d6af190865d2 100644
> > > > --- a/kernel/livepatch/transition.c
> > > > +++ b/kernel/livepatch/transition.c
> > > > @@ -87,6 +87,36 @@ static void klp_complete_transition(void)
> > > >  		 klp_transition_patch->mod->name,
> > > >  		 klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
> > > >  
> > > > +	/*
> > > > +	 * For replace patches, we disable all previous patches, and replace
> > > > +	 * the dynamic no-op functions by removing the ftrace hook.
> > > > +	 */
> > > > +	if (klp_transition_patch->replace && klp_target_state == KLP_PATCHED) {
> > > > +		/*
> > > > +		 * Make sure that no ftrace handler accesses any older patch
> > > > +		 * on the stack.  This might happen when the user forced the
> > > > +		 * transaction while some running tasks were still falling
> > > > +		 * back to the old code.  There might even still be ftrace
> > > > +		 * handlers that have not seen the last patch on the stack yet.
> > > > +		 *
> > > > +		 * It probably is not necessary because of the rcu-safe access.
> > > > +		 * But better be safe than sorry.
> > > > +		 */
> > > > +		if (klp_forced)
> > > > +			klp_synchronize_transition();
> > > 
> > > I don't like this.  Hopefully we can get just rid of it, if we also get
> > > rid of the concept of "throwing away" patches like I proposed.
> > 
> > What exactly you do not like about it, please?
> > 
> > It is not needed if all processes were migrated using the consistency
> > model, definitely.
> > 
> > If the transition has been forced then the barrier should be needed from
> > similar reasons as the barrier after klp_unpatch_objects() below.
> > We basically want to be sure what ftrace handlers see on the stack.
> > 
> > Will it help, when I remove the last paragraph where the formulation
> > is quite uncertain?
> 
> Well, the last paragraph doesn't inspire a lot of confidence ;-)  It
> sounds like voodoo.

Races are never easy area. You are right that I was not completely
confident and wanted to be on the safe side. Your questions helped
me to realize that the synchronization is not neeeded.


> Also the comment just seems very confusing to me:
> 
> - What specifically is it protecting against, e.g., _why_ should no
>   ftrace handler access any old patch on the stack, and when shouldn't
>   it do so?  Is the barrier needed before func->transition is cleared,
>   or what?

I was afraid of invalid memory accessed in klp_ftrace_handler().
I simply underestimated the power of RCU. I am not that familiar
with it. Also the following is a bit non-standard:

	func = list_entry_rcu(func->stack_node.next,
			      struct klp_func, stack_node);

Anyway, you made me to check it. All looks safe after all.

> - Does RCU make it safe, or doesn't it?  If yes, why is this needed?  If
>   no, why not?

Yes. I removed the synchronization. Instead, I explained the situation
in a comment above klp_discard_replaced_patches().


 
> > > > +
> > > > +		klp_throw_away_replaced_patches(klp_transition_patch,
> > > > +						klp_forced);
> > > > +
> > > > +		/*
> > > > +		 * There is no need to synchronize the transition after removing
> > > > +		 * nops. They must be the last on the func_stack. Ftrace
> > > > +		 * gurantees that nobody will stay in the trampoline after
> > > 
> > > "guarantees"
> > > 
> > > > +		 * the ftrace handler is unregistered.
> > > > +		 */
> > > > +		klp_unpatch_objects(klp_transition_patch, KLP_FUNC_NOP);
> > > > +	}
> > > > +
> > > >  	if (klp_target_state == KLP_UNPATCHED) {
> > > >  		/*
> > > >  		 * All tasks have transitioned to KLP_UNPATCHED so we can now
> > > > @@ -143,6 +173,15 @@ static void klp_complete_transition(void)
> > > >  	if (!klp_forced && klp_target_state == KLP_UNPATCHED)
> > > >  		module_put(klp_transition_patch->mod);
> > > >  
> > > > +	/*
> > > > +	 * We do not need to wait until the objects are really freed.
> > > > +	 * The patch must be on the bottom of the stack. Therefore it
> > > > +	 * will never replace anything else. The only important thing
> > > > +	 * is that we wait when the patch is being unregistered.
> > > > +	 */
> > > > +	if (klp_transition_patch->replace && klp_target_state == KLP_PATCHED)
> > > > +		klp_free_objects(klp_transition_patch, KLP_FUNC_NOP);
> > > > +
> > > 
> > > This makes me a bit nervous.  What happens if the patch is enabled, then
> > > disabled, then enabled again?  Then klp_free_objects() wouldn't do
> > > anything, because the ops would already be freed.
> > 
> > They are not necessary when all replaced patches are removed from
> > the stack. There will be no livepatch if this one gets disabled.
> 
> My point was that if you enable, then disable, then enable again,
> klp_free_objects() will get called again, and it will do nothing the
> second time around.

> Maybe that's safe in this instance, but in general, it's easy to forget
> the re-enable case when adding special cases for 'patch->replace'.

Yes, it is safe.


> I get the feeling that it would be safer to just clear 'patch->replace'
> after this step to avoid such scenarios.  After all, when re-enabling a
> 'replace' patch, it's no longer replacing anything (assuming here that a
> replace patch will permanently disable all previous patches).

I am not completely comfortable with touching item that is set by the
author of the patch. On the other hand, I do not see how it could
harm. I have just added the line to disable the flag.

Best Regards,
Petr