On Thu 2016-12-22 12:31:37, Josh Poimboeuf wrote: > On Thu, Dec 22, 2016 at 03:34:52PM +0100, Petr Mladek wrote: > > On Wed 2016-12-21 15:25:05, Josh Poimboeuf wrote: > > > On Tue, Dec 20, 2016 at 06:32:46PM +0100, Petr Mladek wrote: > > > > On Thu 2016-12-08 12:08:38, Josh Poimboeuf wrote: > > > > > Change livepatch to use a basic per-task consistency model. This is the > > > > > foundation which will eventually enable us to patch those ~10% of > > > > > security patches which change function or data semantics. This is the > > > > > biggest remaining piece needed to make livepatch more generally useful. > > > > > > > > > > [1] https://lkml.kernel.org/r/20141107140458.GA21774@xxxxxxx > > > > > > > > > > --- /dev/null > > > > > +++ b/kernel/livepatch/transition.c > > > > > + /* > > > > > + * Enforce the order of the task->patch_state initializations and the > > > > > + * func->transition updates to ensure that, in the enable path, > > > > > + * klp_ftrace_handler() doesn't see a func in transition with a > > > > > + * task->patch_state of KLP_UNDEFINED. > > > > > + */ > > > > > + smp_wmb(); > > > > > + > > > > > + /* > > > > > + * Set the func transition states so klp_ftrace_handler() will know to > > > > > + * switch to the transition logic. > > > > > + * > > > > > + * When patching, the funcs aren't yet in the func_stack and will be > > > > > + * made visible to the ftrace handler shortly by the calls to > > > > > + * klp_patch_object(). > > > > > + * > > > > > + * When unpatching, the funcs are already in the func_stack and so are > > > > > + * already visible to the ftrace handler. > > > > > + */ > > > > > + klp_for_each_object(patch, obj) > > > > > + klp_for_each_func(obj, func) > > > > > + func->transition = true; > > > > > +} > > > > > + > > > > > +/* > > > > > + * Start the transition to the specified target patch state so tasks can begin > > > > > + * switching to it. > > > > > + */ > > > > > +void klp_start_transition(void) > > > > > +{ > > > > > + struct task_struct *g, *task; > > > > > + unsigned int cpu; > > > > > + > > > > > + WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED); > > > > > + > > > > > + pr_notice("'%s': %s...\n", klp_transition_patch->mod->name, > > > > > + klp_target_state == KLP_PATCHED ? "patching" : "unpatching"); > > > > > + > > > > > + /* > > > > > + * If the patch can be applied or reverted immediately, skip the > > > > > + * per-task transitions. > > > > > + */ > > > > > + if (klp_transition_patch->immediate) > > > > > + return; > > > > > + > > > > > + /* > > > > > + * Mark all normal tasks as needing a patch state update. As they pass > > > > > + * through the syscall barrier they'll switch over to the target state > > > > > + * (unless we switch them in klp_try_complete_transition() first). > > > > > + */ > > > > > + read_lock(&tasklist_lock); > > > > > + for_each_process_thread(g, task) > > > > > + set_tsk_thread_flag(task, TIF_PATCH_PENDING); > > > > > > > > This is called also from klp_reverse_transition(). We should set it > > > > only when the task need migration. Also we should clear it when > > > > the task is in the right state already. > > > > > > > > It is not only optimization. It actually solves a race between > > > > klp_complete_transition() and klp_update_patch_state(), see below. > > > > > > I agree about the race, but if I did: > > > > > > for_each_process_thread(g, task) { > > > if (task->patch_state != klp_target_state) > > > set_tsk_thread_flag(task, TIF_PATCH_PENDING); > > > else > > > clear_tsk_thread_flag(task, TIF_PATCH_PENDING); > > > } > > > > > > It would still leave a small window where TIF_PATCH_PENDING gets set for > > > an already patched task, if klp_update_patch_state() is running at the > > > same time. > > > > I see your point. Well, it seems that it is more complicated: > > > > The race would be possible only when this was called from > > klp_reverse_transition(). But we need to call there > > rcu_synchronize() to prevent races with klp_update_patch_state() > > also to prevent prelimitary patch completion. > > > > The result is: > > > > if (task->patch_state != klp_target_state) { > > # it means that the task was already migrated before > > # we reverted klp_target_state. It means that > > # the TIF flag was already cleared, the related > > # klp_update_patch_state() already finished (thanks > > # to rcu_synchronize() and new one will be called > > # only when we set the flag again > > # => it is safe to set it > > > > # we should also check and warn when the TIF flag > > # was not clear before we set it here > > > > > > else > > > > # the task was not migrated before we reverted > > # klp_target_state. klp_update_patch_state() > > # could run in parallel but it will do the same > > # what we do, clear TIF flag and keep the patch_state > > # as is > > # => it is safe to clear it > > > > > > I agree that this is complex like hell. But it also allows use to > > check that things work as we expect. > > Ouch. I agree that it seems safe but it's way too hard to reason about. > And then it gets worse if you try to think about what happens when > adding another reverse operation. > > > > > If we always set the flag here and always clear it later, we might > > hide a bug. > > > > If we want to make it slightly more straightforward, we might > > clear TIF flags in klp_reverse_transaction() before we revert > > klp_target_state. The later rcu_synchronize() should make sure > > that all migrations are finished and non-will run in parallel. > > Then we could set the TIF flag only where needed here. > > I think this last paragraph is important. It would simplify things > greatly and ensure we won't have klp_update_patch_state() changing > things in the background. OK, let's clear all TIF_PATCH_PENDIG flags and call rcu_synchronize() at the beginning of klp_reverse_transition(). It might be slightly suboptimal but it greatly simplifies the situation. I vote for it. We need to prevent our heads from cracking ;-) Note that I would still set TIF_PATCH_PENDING flag only for tasks that are not in the requested state. > > > > > + read_unlock(&tasklist_lock); > > > > > + > > > > > + /* > > > > > + * Ditto for the idle "swapper" tasks, though they never cross the > > > > > + * syscall barrier. Instead they switch over in cpu_idle_loop(). > > > > > + */ > > > > > + get_online_cpus(); > > > > > + for_each_online_cpu(cpu) > > > > > + set_tsk_thread_flag(idle_task(cpu), TIF_PATCH_PENDING); > > > > > + put_online_cpus(); > > > > > > > > Also this stage need to be somehow handled by CPU coming/going > > > > handlers. > > > > > > Here I think we could automatically switch any offline CPUs' idle tasks. > > > And something similar in klp_try_complete_transition(). > > > > We still need to make sure to do not race with the cpu_up()/cpu_down() > > calls. > > Hm, maybe we'd need to call cpu_hotplug_disable() before switching the > offline idle tasks? > > > I would use here the trick with for_each_possible_cpu() and let > > the migration for the stack check. > > There are a few issues with that: > > 1) The idle task of a missing CPU doesn't *have* a stack, so it doesn't > make much sense to try to check it. > > 2) We can't rely *only* on the stack check, because not all arches have > it. The other way to migrate idle tasks is from the idle loop switch > point. But if the task's CPU is down, its idle loop isn't running so > it can't migrate. > > (Note this is currently a theoretical point: we currently don't allow > such arches to use the consistency model anyway because there's no > way for them to migrate kthreads.) Good points. My only concern is that the transaction might take a long or even forever. I am not sure if it is wise to disable cpu_hotplug for the entire transaction. A compromise might be to disable cpu hotplug only when the task state is manipulated a more complex way. Hmm, cpu_hotplug_disable() looks like a rather costly function. We should not call it in klp_try_complete_transition(). But we could do: 1. When the patch is being enabled, disable cpu hotplug, go through each_possible_cpu and setup the transaction only for CPUs that are online. Then we could enable the hotplug again. 2. Check only each_online_cpu in klp_try_complete_transition(). If all tasks are migrated, disable cpu hotplug and re-check idle tasks on online CPUs. If any is not migrated, enable hotplug and return failure. Othewise, continue with completion of the transaction. [*] 3. In klp_complete_transition, update all tasks including the offline CPUs and enable cpu hotplug again. If the re-check in the 2nd step looks ugly, we could add some hotlug notifiers to make sure that enabled/disabled CPUs are in a reasonable state. We still should disable the hotplug in the 1st and 3rd step. BTW: There is a new API for the cpu hotplug callbacks. I was involved in one conversion. You might take inspiration in drivers/thermal/intel_powerclamp.c. See cpuhp_setup_state_nocalls() there. [...] > > > > > diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c > > > > > index e34f871..bb61c65 100644 > > > > > --- a/samples/livepatch/livepatch-sample.c > > > > > +++ b/samples/livepatch/livepatch-sample.c > > > > > @@ -17,6 +17,8 @@ > > > > > * along with this program; if not, see <http://www.gnu.org/licenses/>. > > > > > */ > > > > > > > > > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > > > > + > > > > > #include <linux/module.h> > > > > > #include <linux/kernel.h> > > > > > #include <linux/livepatch.h> > > > > > @@ -69,6 +71,11 @@ static int livepatch_init(void) > > > > > { > > > > > int ret; > > > > > > > > > > + if (!klp_have_reliable_stack() && !patch.immediate) { > > > > > + pr_notice("disabling consistency model!\n"); > > > > > + patch.immediate = true; > > > > > + } > > > > > > > > I am scared to have this in the sample module. It makes sense > > > > to use the consistency model even for immediate patches because > > > > it allows to remove them. But this must not be used for patches > > > > that really require the consistency model. We should add > > > > a big fat warning at least. > > > > > > I did this so that the sample module would still work for non-x86_64 > > > arches, for which there's currently no way to patch kthreads. > > > > > > Notice I did add a warning: > > > > > > pr_notice("disabling consistency model!\n"); > > > > > > Is the warning not fat enough? > > > > The warning does not explain who did it, why, if it is safe, and when > > this could be used. I suggest a comment like: > > > > /* > > * WARNING: Use this check only when you know what you do! > > * > > * This sample patch does not change the semantic of the data structures, > > * locks, or return adresses. It is safe to be applied immediately. > > * But we want to test and use the consistency model on supported > > * architectures. It allows to remove the patch module. > > * > > * See Documentation/livepatch/livepatch.txt for more details, please. > > */ > > > > Also the message might be more explicit. > > > > pr_notice("livepatch-sample: The consistency model is not supported on > > this architecture. Using the immediate model that is safe enough.\n"); > > Ok, will try to do something like that. > > > Alternatively, we might allow more values for patch.immediate, e.g. > > > > enum klp_consistency_model { > > KLP_CM_IMMEDIATE, > > KLP_CM_TASK, > > KLP_CM_TASK_OR_IMMEDIATE, > > }; > > > > Then we could do the decision on the kernel side. > > But I am not sure if this would be widely used and it > > it is worth the complication. > > I'd rather avoid that :-) Same here :-) Best Regards, Petr -- To unsubscribe from this list: send the line "unsubscribe live-patching" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
