Re: [PATCH v4 2/2] livepatch: force transition to finish

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Mon, Nov 20, 2017 at 04:57:19PM +0100, Miroslav Benes wrote:
> On Wed, 15 Nov 2017, Miroslav Benes wrote:
> 
> > If a task sleeps in a set of patched functions uninterruptedly, it could
> > block the whole transition 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 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! Administrator should not use this feature without a
> > clearance from a patch distributor. It must be checked that by doing so
> > the consistency model guarantees are not violated.
> > 
> > Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> 
> While working on "immediate" removal, I realized we had the similar 
> problem here with modules removal. There is no way out of the rabbit hole.
> 
> If a patch is forced, we obviously cannot say there is no task sleeping in 
> the old code. This could be disastrous if such old module is then removed 
> (either we disabled it and we want to rmmod it, or there is a new "atomic 
> replace" patch and we want to remove the old one).
> 
> We need something like the following (at least as a starting point)
> 
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index 566ab210853f..df4f2bbd9731 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -33,6 +33,8 @@ struct klp_patch *klp_transition_patch;
>  
>  static int klp_target_state = KLP_UNDEFINED;
>  
> +static bool klp_forced = false;
> +
>  /*
>   * This work can be performed periodically to finish patching or unpatching any
>   * "straggler" tasks which failed to transition in the first attempt.
> @@ -109,7 +111,7 @@ static void klp_complete_transition(void)
>                 }
>         }
>  
> -       if (klp_target_state == KLP_UNPATCHED && !immediate_func)
> +       if (klp_target_state == KLP_UNPATCHED && !klp_forced && !immediate_func)
>                 module_put(klp_transition_patch->mod);
>  
>         /* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */
> @@ -642,4 +644,6 @@ void klp_force_transition(void)
>  
>         for_each_possible_cpu(cpu)
>                 klp_update_patch_state(idle_task(cpu));
> +
> +       klp_forced = true;
>  }
> 
> 
> It is still better than immediate, because it is a "ex post" action.

Looks good to me.

> We can also try to improve later. We could remember all forced tasks
> and reenable rmmod once those tasks are really migrated ("shadow
> migration"). 

NACK :-)  Forcing should hopefully be a rare event, not worth the
trouble to try to keep track of that IMO.

-- 
Josh