From 43347d56c8d9dd732cee2f8efd384ad21dd1f6c4 Mon Sep 17 00:00:00 2001 From: Miroslav Benes Date: Wed, 15 Nov 2017 14:50:13 +0100 Subject: [PATCH 1/4] livepatch: send a fake signal to all blocking tasks Live patching consistency model is of LEAVE_PATCHED_SET and SWITCH_THREAD. This means that all tasks in the system have to be marked one by one as safe to call a new patched function. Safe means when a task is not (sleeping) in a set of patched functions. That is, no patched function is on the task's stack. Another clearly safe place is the boundary between kernel and userspace. The patching waits for all tasks to get outside of the patched set or to cross the boundary. The transition is completed afterwards. The problem is that a task can block the transition for quite a long time, if not forever. It could sleep in a set of patched functions, for example. Luckily we can force the task to leave the set by sending it a fake signal, that is a signal with no data in signal pending structures (no handler, no sign of proper signal delivered). Suspend/freezer use this to freeze the tasks as well. The task gets TIF_SIGPENDING set and is woken up (if it has been sleeping in the kernel before) or kicked by rescheduling IPI (if it was running on other CPU). This causes the task to go to kernel/userspace boundary where the signal would be handled and the task would be marked as safe in terms of live patching. There are tasks which are not affected by this technique though. The fake signal is not sent to kthreads. They should be handled differently. They can be woken up so they leave the patched set and their TIF_PATCH_PENDING can be cleared thanks to stack checking. For the sake of completeness, if the task is in TASK_RUNNING state but not currently running on some CPU it doesn't get the IPI, but it would eventually handle the signal anyway. Second, if the task runs in the kernel (in TASK_RUNNING state) it gets the IPI, but the signal is not handled on return from the interrupt. It would be handled on return to the userspace in the future when the fake signal is sent again. Stack checking deals with these cases in a better way. If the task was sleeping in a syscall it would be woken by our fake signal, it would check if TIF_SIGPENDING is set (by calling signal_pending() predicate) and return ERESTART* or EINTR. Syscalls with ERESTART* return values are restarted in case of the fake signal (see do_signal()). EINTR is propagated back to the userspace program. This could disturb the program, but... * each process dealing with signals should react accordingly to EINTR return values. * syscalls returning EINTR happen to be quite common situation in the system even if no fake signal is sent. * freezer sends the fake signal and does not deal with EINTR anyhow. Thus EINTR values are returned when the system is resumed. The very safe marking is done in architectures' "entry" on syscall and interrupt/exception exit paths, and in a stack checking functions of livepatch. TIF_PATCH_PENDING is cleared and the next recalc_sigpending() drops TIF_SIGPENDING. In connection with this, also call klp_update_patch_state() before do_signal(), so that recalc_sigpending() in dequeue_signal() can clear TIF_PATCH_PENDING immediately and thus prevent a double call of do_signal(). Note that the fake signal is not sent to stopped/traced tasks. Such task prevents the patching to finish till it continues again (is not traced anymore). Last, sending the fake signal is not automatic. It is done only when admin requests it by writing 1 to signal sysfs attribute in livepatch sysfs directory. Signed-off-by: Miroslav Benes Cc: Oleg Nesterov Cc: Michael Ellerman Cc: Thomas Gleixner Cc: Ingo Molnar Cc: "H. Peter Anvin" Cc: Andy Lutomirski Cc: linuxppc-dev@lists.ozlabs.org Cc: x86@kernel.org Acked-by: Michael Ellerman (powerpc) Signed-off-by: Jiri Kosina --- .../ABI/testing/sysfs-kernel-livepatch | 12 ++++++ Documentation/livepatch/livepatch.txt | 11 ++++- arch/powerpc/kernel/signal.c | 6 +-- arch/x86/entry/common.c | 6 +-- kernel/livepatch/core.c | 30 ++++++++++++++ kernel/livepatch/transition.c | 41 +++++++++++++++++++ kernel/livepatch/transition.h | 1 + kernel/signal.c | 4 +- 8 files changed, 102 insertions(+), 9 deletions(-) diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch index d5d39748382f..3bb9d5bc1ce3 100644 --- a/Documentation/ABI/testing/sysfs-kernel-livepatch +++ b/Documentation/ABI/testing/sysfs-kernel-livepatch @@ -33,6 +33,18 @@ Description: An attribute which indicates whether the patch is currently in transition. +What: /sys/kernel/livepatch//signal +Date: Nov 2017 +KernelVersion: 4.15.0 +Contact: live-patching@vger.kernel.org +Description: + A writable attribute that allows administrator to affect the + course of an existing transition. Writing 1 sends a fake + signal to all remaining blocking tasks. The fake signal + means that no proper signal is delivered (there is no data in + signal pending structures). Tasks are interrupted or woken up, + and forced to change their patched state. + What: /sys/kernel/livepatch// Date: Nov 2014 KernelVersion: 3.19.0 diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt index ecdb18104ab0..9bcdef277a36 100644 --- a/Documentation/livepatch/livepatch.txt +++ b/Documentation/livepatch/livepatch.txt @@ -176,8 +176,12 @@ If a patch is in transition, this file shows 0 to indicate the task is unpatched and 1 to indicate it's patched. Otherwise, if no patch is in transition, it shows -1. Any tasks which are blocking the transition can be signaled with SIGSTOP and SIGCONT to force them to change their -patched state. - +patched state. This may be harmful to the system though. +/sys/kernel/livepatch//signal attribute provides a better alternative. +Writing 1 to the attribute sends a fake signal to all remaining blocking +tasks. No proper signal is actually delivered (there is no data in signal +pending structures). Tasks are interrupted or woken up, and forced to change +their patched state. 3.1 Adding consistency model support to new architectures --------------------------------------------------------- @@ -435,6 +439,9 @@ Information about the registered patches can be found under /sys/kernel/livepatch. The patches could be enabled and disabled by writing there. +/sys/kernel/livepatch//signal attribute allows administrator to affect a +patching operation. + See Documentation/ABI/testing/sysfs-kernel-livepatch for more details. diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c index e9436c5e1e09..bf9c4e7792d1 100644 --- a/arch/powerpc/kernel/signal.c +++ b/arch/powerpc/kernel/signal.c @@ -153,6 +153,9 @@ void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags) if (thread_info_flags & _TIF_UPROBE) uprobe_notify_resume(regs); + if (thread_info_flags & _TIF_PATCH_PENDING) + klp_update_patch_state(current); + if (thread_info_flags & _TIF_SIGPENDING) { BUG_ON(regs != current->thread.regs); do_signal(current); @@ -163,9 +166,6 @@ void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags) tracehook_notify_resume(regs); } - if (thread_info_flags & _TIF_PATCH_PENDING) - klp_update_patch_state(current); - user_enter(); } diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c index d7d3cc24baf4..1e3883e45687 100644 --- a/arch/x86/entry/common.c +++ b/arch/x86/entry/common.c @@ -153,6 +153,9 @@ static void exit_to_usermode_loop(struct pt_regs *regs, u32 cached_flags) if (cached_flags & _TIF_UPROBE) uprobe_notify_resume(regs); + if (cached_flags & _TIF_PATCH_PENDING) + klp_update_patch_state(current); + /* deal with pending signal delivery */ if (cached_flags & _TIF_SIGPENDING) do_signal(regs); @@ -165,9 +168,6 @@ static void exit_to_usermode_loop(struct pt_regs *regs, u32 cached_flags) if (cached_flags & _TIF_USER_RETURN_NOTIFY) fire_user_return_notifiers(); - if (cached_flags & _TIF_PATCH_PENDING) - klp_update_patch_state(current); - /* Disable IRQs and retry */ local_irq_disable(); diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index de9e45dca70f..88766bd91803 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -454,6 +454,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch); * /sys/kernel/livepatch/ * /sys/kernel/livepatch//enabled * /sys/kernel/livepatch//transition + * /sys/kernel/livepatch//signal * /sys/kernel/livepatch// * /sys/kernel/livepatch/// */ @@ -528,11 +529,40 @@ static ssize_t transition_show(struct kobject *kobj, patch == klp_transition_patch); } +static ssize_t signal_store(struct kobject *kobj, struct kobj_attribute *attr, + const char *buf, size_t count) +{ + struct klp_patch *patch; + int ret; + bool val; + + patch = container_of(kobj, struct klp_patch, kobj); + + /* + * klp_mutex lock is not grabbed here intentionally. It is not really + * needed. The race window is harmless and grabbing the lock would only + * hold the action back. + */ + if (patch != klp_transition_patch) + return -EINVAL; + + ret = kstrtobool(buf, &val); + if (ret) + return ret; + + if (val) + klp_send_signals(); + + return count; +} + static struct kobj_attribute enabled_kobj_attr = __ATTR_RW(enabled); static struct kobj_attribute transition_kobj_attr = __ATTR_RO(transition); +static struct kobj_attribute signal_kobj_attr = __ATTR_WO(signal); static struct attribute *klp_patch_attrs[] = { &enabled_kobj_attr.attr, &transition_kobj_attr.attr, + &signal_kobj_attr.attr, NULL }; diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c index 56add6327736..edcfcb8ebb2d 100644 --- a/kernel/livepatch/transition.c +++ b/kernel/livepatch/transition.c @@ -608,3 +608,44 @@ void klp_copy_process(struct task_struct *child) /* TIF_PATCH_PENDING gets copied in setup_thread_stack() */ } + +/* + * Sends a fake signal to all non-kthread tasks with TIF_PATCH_PENDING set. + * Kthreads with TIF_PATCH_PENDING set are woken up. Only admin can request this + * action currently. + */ +void klp_send_signals(void) +{ + struct task_struct *g, *task; + + pr_notice("signaling remaining tasks\n"); + + read_lock(&tasklist_lock); + for_each_process_thread(g, task) { + if (!klp_patch_pending(task)) + continue; + + /* + * There is a small race here. We could see TIF_PATCH_PENDING + * set and decide to wake up a kthread or send a fake signal. + * Meanwhile the task could migrate itself and the action + * would be meaningless. It is not serious though. + */ + if (task->flags & PF_KTHREAD) { + /* + * Wake up a kthread which sleeps interruptedly and + * still has not been migrated. + */ + wake_up_state(task, TASK_INTERRUPTIBLE); + } else { + /* + * Send fake signal to all non-kthread tasks which are + * still not migrated. + */ + spin_lock_irq(&task->sighand->siglock); + signal_wake_up(task, 0); + spin_unlock_irq(&task->sighand->siglock); + } + } + read_unlock(&tasklist_lock); +} diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h index 0f6e27c481f9..40522795a5f6 100644 --- a/kernel/livepatch/transition.h +++ b/kernel/livepatch/transition.h @@ -11,5 +11,6 @@ void klp_cancel_transition(void); void klp_start_transition(void); void klp_try_complete_transition(void); void klp_reverse_transition(void); +void klp_send_signals(void); #endif /* _LIVEPATCH_TRANSITION_H */ diff --git a/kernel/signal.c b/kernel/signal.c index 8dcd8825b2de..186143b06d00 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -40,6 +40,7 @@ #include #include #include +#include #define CREATE_TRACE_POINTS #include @@ -163,7 +164,8 @@ void recalc_sigpending_and_wake(struct task_struct *t) void recalc_sigpending(void) { - if (!recalc_sigpending_tsk(current) && !freezing(current)) + if (!recalc_sigpending_tsk(current) && !freezing(current) && + !klp_patch_pending(current)) clear_thread_flag(TIF_SIGPENDING); } From c99a2be790b07752d8cc694434d3450afd4c5a00 Mon Sep 17 00:00:00 2001 From: Miroslav Benes Date: Wed, 22 Nov 2017 11:29:21 +0100 Subject: [PATCH 2/4] livepatch: force transition to finish 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. Removal (rmmod) of patch modules is permanently disabled when the feature is used. It cannot be guaranteed there is no task sleeping in such module. Signed-off-by: Miroslav Benes Acked-by: Josh Poimboeuf Reviewed-by: Petr Mladek Signed-off-by: Jiri Kosina --- .../ABI/testing/sysfs-kernel-livepatch | 14 ++++++++ Documentation/livepatch/livepatch.txt | 18 ++++++++-- kernel/livepatch/core.c | 30 ++++++++++++++++ kernel/livepatch/transition.c | 36 +++++++++++++++++-- kernel/livepatch/transition.h | 1 + 5 files changed, 95 insertions(+), 4 deletions(-) diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch index 3bb9d5bc1ce3..dac7e1e62a8b 100644 --- a/Documentation/ABI/testing/sysfs-kernel-livepatch +++ b/Documentation/ABI/testing/sysfs-kernel-livepatch @@ -45,6 +45,20 @@ Description: signal pending structures). Tasks are interrupted or woken up, and forced to change their patched state. +What: /sys/kernel/livepatch//force +Date: Nov 2017 +KernelVersion: 4.15.0 +Contact: live-patching@vger.kernel.org +Description: + A writable attribute that allows administrator to affect the + course of an existing transition. Writing 1 clears + TIF_PATCH_PENDING flag of all tasks and thus forces the tasks to + the patched or unpatched state. Administrator should not + use this feature without a clearance from a patch + distributor. Removal (rmmod) of patch modules is permanently + disabled when the feature is used. See + Documentation/livepatch/livepatch.txt for more information. + What: /sys/kernel/livepatch// Date: Nov 2014 KernelVersion: 3.19.0 diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt index 9bcdef277a36..896ba8941702 100644 --- a/Documentation/livepatch/livepatch.txt +++ b/Documentation/livepatch/livepatch.txt @@ -183,6 +183,20 @@ tasks. No proper signal is actually delivered (there is no data in signal pending structures). Tasks are interrupted or woken up, and forced to change their patched state. +Administrator can also affect a transition through +/sys/kernel/livepatch//force attribute. Writing 1 there clears +TIF_PATCH_PENDING flag of all tasks and thus forces the tasks to the patched +state. Important note! The force attribute is intended for cases when the +transition gets stuck for a long time because of a blocking task. Administrator +is expected to collect all necessary data (namely stack traces of such blocking +tasks) and request a clearance from a patch distributor to force the transition. +Unauthorized usage may cause harm to the system. It depends on the nature of the +patch, which functions are (un)patched, and which functions the blocking tasks +are sleeping in (/proc//stack may help here). Removal (rmmod) of patch +modules is permanently disabled when the force feature is used. It cannot be +guaranteed there is no task sleeping in such module. It implies unbounded +reference count if a patch module is disabled and enabled in a loop. + 3.1 Adding consistency model support to new architectures --------------------------------------------------------- @@ -439,8 +453,8 @@ Information about the registered patches can be found under /sys/kernel/livepatch. The patches could be enabled and disabled by writing there. -/sys/kernel/livepatch//signal attribute allows administrator to affect a -patching operation. +/sys/kernel/livepatch//signal and /sys/kernel/livepatch//force +attributes allow administrator to affect a patching operation. See Documentation/ABI/testing/sysfs-kernel-livepatch for more details. diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index 88766bd91803..1c3c9b27c916 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -455,6 +455,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch); * /sys/kernel/livepatch//enabled * /sys/kernel/livepatch//transition * /sys/kernel/livepatch//signal + * /sys/kernel/livepatch//force * /sys/kernel/livepatch// * /sys/kernel/livepatch/// */ @@ -556,13 +557,42 @@ static ssize_t signal_store(struct kobject *kobj, struct kobj_attribute *attr, return count; } +static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr, + const char *buf, size_t count) +{ + struct klp_patch *patch; + int ret; + bool val; + + patch = container_of(kobj, struct klp_patch, kobj); + + /* + * klp_mutex lock is not grabbed here intentionally. It is not really + * needed. The race window is harmless and grabbing the lock would only + * hold the action back. + */ + if (patch != klp_transition_patch) + return -EINVAL; + + ret = kstrtobool(buf, &val); + if (ret) + return ret; + + if (val) + klp_force_transition(); + + return count; +} + static struct kobj_attribute enabled_kobj_attr = __ATTR_RW(enabled); static struct kobj_attribute transition_kobj_attr = __ATTR_RO(transition); static struct kobj_attribute signal_kobj_attr = __ATTR_WO(signal); +static struct kobj_attribute force_kobj_attr = __ATTR_WO(force); static struct attribute *klp_patch_attrs[] = { &enabled_kobj_attr.attr, &transition_kobj_attr.attr, &signal_kobj_attr.attr, + &force_kobj_attr.attr, NULL }; diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c index edcfcb8ebb2d..be5bfa533ee8 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. @@ -146,9 +148,12 @@ done: /* * See complementary comment in __klp_enable_patch() for why we * keep the module reference for immediate patches. + * + * klp_forced or immediate_func set implies unbounded increase of + * module's ref count if the module is disabled/enabled in a loop. */ - if (!klp_transition_patch->immediate && !immediate_func && - klp_target_state == KLP_UNPATCHED) { + if (!klp_forced && !klp_transition_patch->immediate && + !immediate_func && klp_target_state == KLP_UNPATCHED) { module_put(klp_transition_patch->mod); } @@ -649,3 +654,30 @@ void klp_send_signals(void) } read_unlock(&tasklist_lock); } + +/* + * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an + * existing transition to finish. + * + * NOTE: klp_update_patch_state(task) requires the task to be inactive or + * 'current'. This is not the case here and the consistency model could be + * broken. Administrator, who is the only one to execute the + * klp_force_transitions(), has to be aware of this. + */ +void klp_force_transition(void) +{ + struct task_struct *g, *task; + unsigned int cpu; + + pr_warn("forcing remaining tasks to the patched state\n"); + + read_lock(&tasklist_lock); + for_each_process_thread(g, task) + klp_update_patch_state(task); + read_unlock(&tasklist_lock); + + for_each_possible_cpu(cpu) + klp_update_patch_state(idle_task(cpu)); + + klp_forced = true; +} diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h index 40522795a5f6..f9d0bc016067 100644 --- a/kernel/livepatch/transition.h +++ b/kernel/livepatch/transition.h @@ -12,5 +12,6 @@ void klp_start_transition(void); void klp_try_complete_transition(void); void klp_reverse_transition(void); void klp_send_signals(void); +void klp_force_transition(void); #endif /* _LIVEPATCH_TRANSITION_H */ From d0807da78e11d46f18399cbf8c4028c731346766 Mon Sep 17 00:00:00 2001 From: Miroslav Benes Date: Wed, 10 Jan 2018 11:01:28 +0100 Subject: [PATCH 3/4] livepatch: Remove immediate feature Immediate flag has been used to disable per-task consistency and patch all tasks immediately. It could be useful if the patch doesn't change any function or data semantics. However, it causes problems on its own. The consistency problem is currently broken with respect to immediate patches. func a patches 1i 2i 3 When the patch 3 is applied, only 2i function is checked (by stack checking facility). There might be a task sleeping in 1i though. Such task is migrated to 3, because we do not check 1i in klp_check_stack_func() at all. Coming atomic replace feature would be easier to implement and more reliable without immediate. Thus, remove immediate feature completely and save us from the problems. Note that force feature has the similar problem. However it is considered as a last resort. If used, administrator should not apply any new live patches and should plan for reboot into an updated kernel. The architectures would now need to provide HAVE_RELIABLE_STACKTRACE to fully support livepatch. Signed-off-by: Miroslav Benes Acked-by: Josh Poimboeuf Signed-off-by: Jiri Kosina --- Documentation/livepatch/livepatch.txt | 89 ++++++-------------- include/linux/livepatch.h | 4 - kernel/livepatch/core.c | 12 +-- kernel/livepatch/transition.c | 49 ++--------- samples/livepatch/livepatch-callbacks-demo.c | 15 ---- samples/livepatch/livepatch-sample.c | 15 ---- samples/livepatch/livepatch-shadow-fix1.c | 15 ---- samples/livepatch/livepatch-shadow-fix2.c | 15 ---- 8 files changed, 33 insertions(+), 181 deletions(-) diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt index 896ba8941702..1ae2de758c08 100644 --- a/Documentation/livepatch/livepatch.txt +++ b/Documentation/livepatch/livepatch.txt @@ -72,8 +72,7 @@ example, they add a NULL pointer or a boundary check, fix a race by adding a missing memory barrier, or add some locking around a critical section. Most of these changes are self contained and the function presents itself the same way to the rest of the system. In this case, the functions might -be updated independently one by one. (This can be done by setting the -'immediate' flag in the klp_patch struct.) +be updated independently one by one. But there are more complex fixes. For example, a patch might change ordering of locking in multiple functions at the same time. Or a patch @@ -125,12 +124,6 @@ safe to patch tasks: b) Patching CPU-bound user tasks. If the task is highly CPU-bound then it will get patched the next time it gets interrupted by an IRQ. - c) In the future it could be useful for applying patches for - architectures which don't yet have HAVE_RELIABLE_STACKTRACE. In - this case you would have to signal most of the tasks on the - system. However this isn't supported yet because there's - currently no way to patch kthreads without - HAVE_RELIABLE_STACKTRACE. 3. For idle "swapper" tasks, since they don't ever exit the kernel, they instead have a klp_update_patch_state() call in the idle loop which @@ -138,27 +131,16 @@ safe to patch tasks: (Note there's not yet such an approach for kthreads.) -All the above approaches may be skipped by setting the 'immediate' flag -in the 'klp_patch' struct, which will disable per-task consistency and -patch all tasks immediately. This can be useful if the patch doesn't -change any function or data semantics. Note that, even with this flag -set, it's possible that some tasks may still be running with an old -version of the function, until that function returns. +Architectures which don't have HAVE_RELIABLE_STACKTRACE solely rely on +the second approach. It's highly likely that some tasks may still be +running with an old version of the function, until that function +returns. In this case you would have to signal the tasks. This +especially applies to kthreads. They may not be woken up and would need +to be forced. See below for more information. -There's also an 'immediate' flag in the 'klp_func' struct which allows -you to specify that certain functions in the patch can be applied -without per-task consistency. This might be useful if you want to patch -a common function like schedule(), and the function change doesn't need -consistency but the rest of the patch does. - -For architectures which don't have HAVE_RELIABLE_STACKTRACE, the user -must set patch->immediate which causes all tasks to be patched -immediately. This option should be used with care, only when the patch -doesn't change any function or data semantics. - -In the future, architectures which don't have HAVE_RELIABLE_STACKTRACE -may be allowed to use per-task consistency if we can come up with -another way to patch kthreads. +Unless we can come up with another way to patch kthreads, architectures +without HAVE_RELIABLE_STACKTRACE are not considered fully supported by +the kernel livepatching. The /sys/kernel/livepatch//transition file shows whether a patch is in transition. Only a single patch (the topmost patch on the stack) @@ -197,6 +179,11 @@ modules is permanently disabled when the force feature is used. It cannot be guaranteed there is no task sleeping in such module. It implies unbounded reference count if a patch module is disabled and enabled in a loop. +Moreover, the usage of force may also affect future applications of live +patches and cause even more harm to the system. Administrator should first +consider to simply cancel a transition (see above). If force is used, reboot +should be planned and no more live patches applied. + 3.1 Adding consistency model support to new architectures --------------------------------------------------------- @@ -234,13 +221,6 @@ few options: a good backup option for those architectures which don't have reliable stack traces yet. -In the meantime, patches for such architectures can bypass the -consistency model by setting klp_patch.immediate to true. This option -is perfectly fine for patches which don't change the semantics of the -patched functions. In practice, this is usable for ~90% of security -fixes. Use of this option also means the patch can't be unloaded after -it has been disabled. - 4. Livepatch module =================== @@ -296,9 +276,6 @@ into three levels: only for a particular object ( vmlinux or a kernel module ). Note that kallsyms allows for searching symbols according to the object name. - There's also an 'immediate' flag which, when set, patches the - function immediately, bypassing the consistency model safety checks. - + struct klp_object defines an array of patched functions (struct klp_func) in the same object. Where the object is either vmlinux (NULL) or a module name. @@ -317,9 +294,6 @@ into three levels: symbols are found. The only exception are symbols from objects (kernel modules) that have not been loaded yet. - Setting the 'immediate' flag applies the patch to all tasks - immediately, bypassing the consistency model safety checks. - For more details on how the patch is applied on a per-task basis, see the "Consistency model" section. @@ -334,14 +308,12 @@ section "Livepatch life-cycle" below for more details about these two operations. Module removal is only safe when there are no users of the underlying -functions. The immediate consistency model is not able to detect this. The -code just redirects the functions at the very beginning and it does not -check if the functions are in use. In other words, it knows when the -functions get called but it does not know when the functions return. -Therefore it cannot be decided when the livepatch module can be safely -removed. This is solved by a hybrid consistency model. When the system is -transitioned to a new patch state (patched/unpatched) it is guaranteed that -no task sleeps or runs in the old code. +functions. This is the reason why the force feature permanently disables +the removal. The forced tasks entered the functions but we cannot say +that they returned back. Therefore it cannot be decided when the +livepatch module can be safely removed. When the system is successfully +transitioned to a new patch state (patched/unpatched) without being +forced it is guaranteed that no task sleeps or runs in the old code. 5. Livepatch life-cycle @@ -355,19 +327,12 @@ First, the patch is applied only when all patched symbols for already loaded objects are found. The error handling is much easier if this check is done before particular functions get redirected. -Second, the immediate consistency model does not guarantee that anyone is not -sleeping in the new code after the patch is reverted. This means that the new -code needs to stay around "forever". If the code is there, one could apply it -again. Therefore it makes sense to separate the operations that might be done -once and those that need to be repeated when the patch is enabled (applied) -again. - -Third, it might take some time until the entire system is migrated -when a more complex consistency model is used. The patch revert might -block the livepatch module removal for too long. Therefore it is useful -to revert the patch using a separate operation that might be called -explicitly. But it does not make sense to remove all information -until the livepatch module is really removed. +Second, it might take some time until the entire system is migrated with +the hybrid consistency model being used. The patch revert might block +the livepatch module removal for too long. Therefore it is useful to +revert the patch using a separate operation that might be called +explicitly. But it does not make sense to remove all information until +the livepatch module is really removed. 5.1. Registration diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h index fc5c1be3f6f4..4754f01c1abb 100644 --- a/include/linux/livepatch.h +++ b/include/linux/livepatch.h @@ -40,7 +40,6 @@ * @new_func: pointer to the patched function code * @old_sympos: a hint indicating which symbol position the old function * can be found (optional) - * @immediate: patch the func immediately, bypassing safety mechanisms * @old_addr: the address of the function being patched * @kobj: kobject for sysfs resources * @stack_node: list node for klp_ops func_stack list @@ -76,7 +75,6 @@ struct klp_func { * in kallsyms for the given object is used. */ unsigned long old_sympos; - bool immediate; /* internal */ unsigned long old_addr; @@ -137,7 +135,6 @@ struct klp_object { * struct klp_patch - patch structure for live patching * @mod: reference to the live patch module * @objs: object entries for kernel objects to be patched - * @immediate: patch all funcs immediately, bypassing safety mechanisms * @list: list node for global list of registered patches * @kobj: kobject for sysfs resources * @enabled: the patch is enabled (but operation may be incomplete) @@ -147,7 +144,6 @@ struct klp_patch { /* external */ struct module *mod; struct klp_object *objs; - bool immediate; /* internal */ struct list_head list; diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index 1c3c9b27c916..41be6061b92f 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -366,11 +366,6 @@ static int __klp_enable_patch(struct klp_patch *patch) /* * A reference is taken on the patch module to prevent it from being * unloaded. - * - * Note: For immediate (no consistency model) patches we don't allow - * patch modules to unload since there is no safe/sane method to - * determine if a thread is still running in the patched code contained - * in the patch module once the ftrace registration is successful. */ if (!try_module_get(patch->mod)) return -ENODEV; @@ -890,12 +885,7 @@ int klp_register_patch(struct klp_patch *patch) if (!klp_initialized()) return -ENODEV; - /* - * Architectures without reliable stack traces have to set - * patch->immediate because there's currently no way to patch kthreads - * with the consistency model. - */ - if (!klp_have_reliable_stack() && !patch->immediate) { + if (!klp_have_reliable_stack()) { pr_err("This architecture doesn't have support for the livepatch consistency model.\n"); return -ENOSYS; } diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c index be5bfa533ee8..7c6631e693bc 100644 --- a/kernel/livepatch/transition.c +++ b/kernel/livepatch/transition.c @@ -82,7 +82,6 @@ static void klp_complete_transition(void) struct klp_func *func; struct task_struct *g, *task; unsigned int cpu; - bool immediate_func = false; pr_debug("'%s': completing %s transition\n", klp_transition_patch->mod->name, @@ -104,16 +103,9 @@ static void klp_complete_transition(void) klp_synchronize_transition(); } - if (klp_transition_patch->immediate) - goto done; - - klp_for_each_object(klp_transition_patch, obj) { - klp_for_each_func(obj, func) { + klp_for_each_object(klp_transition_patch, obj) + klp_for_each_func(obj, func) func->transition = false; - if (func->immediate) - immediate_func = true; - } - } /* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */ if (klp_target_state == KLP_PATCHED) @@ -132,7 +124,6 @@ static void klp_complete_transition(void) task->patch_state = KLP_UNDEFINED; } -done: klp_for_each_object(klp_transition_patch, obj) { if (!klp_is_object_loaded(obj)) continue; @@ -146,16 +137,11 @@ done: klp_target_state == KLP_PATCHED ? "patching" : "unpatching"); /* - * See complementary comment in __klp_enable_patch() for why we - * keep the module reference for immediate patches. - * - * klp_forced or immediate_func set implies unbounded increase of - * module's ref count if the module is disabled/enabled in a loop. + * klp_forced set implies unbounded increase of module's ref count if + * the module is disabled/enabled in a loop. */ - if (!klp_forced && !klp_transition_patch->immediate && - !immediate_func && klp_target_state == KLP_UNPATCHED) { + if (!klp_forced && klp_target_state == KLP_UNPATCHED) module_put(klp_transition_patch->mod); - } klp_target_state = KLP_UNDEFINED; klp_transition_patch = NULL; @@ -223,9 +209,6 @@ static int klp_check_stack_func(struct klp_func *func, struct klp_ops *ops; int i; - if (func->immediate) - return 0; - for (i = 0; i < trace->nr_entries; i++) { address = trace->entries[i]; @@ -387,13 +370,6 @@ void klp_try_complete_transition(void) WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED); - /* - * If the patch can be applied or reverted immediately, skip the - * per-task transitions. - */ - if (klp_transition_patch->immediate) - goto success; - /* * Try to switch the tasks to the target patch state by walking their * stacks and looking for any to-be-patched or to-be-unpatched @@ -437,7 +413,6 @@ void klp_try_complete_transition(void) return; } -success: /* we're done, now cleanup the data structures */ klp_complete_transition(); } @@ -457,13 +432,6 @@ void klp_start_transition(void) 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. They'll * switch either in klp_try_complete_transition() or as they exit the @@ -513,13 +481,6 @@ void klp_init_transition(struct klp_patch *patch, int state) pr_debug("'%s': initializing %s transition\n", 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 (patch->immediate) - return; - /* * Initialize all tasks to the initial patch state to prepare them for * switching to the target state. diff --git a/samples/livepatch/livepatch-callbacks-demo.c b/samples/livepatch/livepatch-callbacks-demo.c index 3d115bd68442..72f9e6d1387b 100644 --- a/samples/livepatch/livepatch-callbacks-demo.c +++ b/samples/livepatch/livepatch-callbacks-demo.c @@ -197,21 +197,6 @@ static int livepatch_callbacks_demo_init(void) { int ret; - if (!klp_have_reliable_stack() && !patch.immediate) { - /* - * WARNING: Be very careful when using 'patch.immediate' in - * your patches. It's ok to use it for simple patches like - * this, but for more complex patches which change function - * semantics, locking semantics, or data structures, it may not - * be safe. Use of this option will also prevent removal of - * the patch. - * - * See Documentation/livepatch/livepatch.txt for more details. - */ - patch.immediate = true; - pr_notice("The consistency model isn't supported for your architecture. Bypassing safety mechanisms and applying the patch immediately.\n"); - } - ret = klp_register_patch(&patch); if (ret) return ret; diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c index 84795223f15f..2d554dd930e2 100644 --- a/samples/livepatch/livepatch-sample.c +++ b/samples/livepatch/livepatch-sample.c @@ -71,21 +71,6 @@ static int livepatch_init(void) { int ret; - if (!klp_have_reliable_stack() && !patch.immediate) { - /* - * WARNING: Be very careful when using 'patch.immediate' in - * your patches. It's ok to use it for simple patches like - * this, but for more complex patches which change function - * semantics, locking semantics, or data structures, it may not - * be safe. Use of this option will also prevent removal of - * the patch. - * - * See Documentation/livepatch/livepatch.txt for more details. - */ - patch.immediate = true; - pr_notice("The consistency model isn't supported for your architecture. Bypassing safety mechanisms and applying the patch immediately.\n"); - } - ret = klp_register_patch(&patch); if (ret) return ret; diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c index fbe0a1f3d99b..830c55514f9f 100644 --- a/samples/livepatch/livepatch-shadow-fix1.c +++ b/samples/livepatch/livepatch-shadow-fix1.c @@ -133,21 +133,6 @@ static int livepatch_shadow_fix1_init(void) { int ret; - if (!klp_have_reliable_stack() && !patch.immediate) { - /* - * WARNING: Be very careful when using 'patch.immediate' in - * your patches. It's ok to use it for simple patches like - * this, but for more complex patches which change function - * semantics, locking semantics, or data structures, it may not - * be safe. Use of this option will also prevent removal of - * the patch. - * - * See Documentation/livepatch/livepatch.txt for more details. - */ - patch.immediate = true; - pr_notice("The consistency model isn't supported for your architecture. Bypassing safety mechanisms and applying the patch immediately.\n"); - } - ret = klp_register_patch(&patch); if (ret) return ret; diff --git a/samples/livepatch/livepatch-shadow-fix2.c b/samples/livepatch/livepatch-shadow-fix2.c index 53c1794bdc5f..ff9948f0ec00 100644 --- a/samples/livepatch/livepatch-shadow-fix2.c +++ b/samples/livepatch/livepatch-shadow-fix2.c @@ -128,21 +128,6 @@ static int livepatch_shadow_fix2_init(void) { int ret; - if (!klp_have_reliable_stack() && !patch.immediate) { - /* - * WARNING: Be very careful when using 'patch.immediate' in - * your patches. It's ok to use it for simple patches like - * this, but for more complex patches which change function - * semantics, locking semantics, or data structures, it may not - * be safe. Use of this option will also prevent removal of - * the patch. - * - * See Documentation/livepatch/livepatch.txt for more details. - */ - patch.immediate = true; - pr_notice("The consistency model isn't supported for your architecture. Bypassing safety mechanisms and applying the patch immediately.\n"); - } - ret = klp_register_patch(&patch); if (ret) return ret; From 8869016d3a58cbe7c31c70f4f008a92122b271c7 Mon Sep 17 00:00:00 2001 From: Miroslav Benes Date: Thu, 21 Dec 2017 14:40:43 +0100 Subject: [PATCH 4/4] livepatch: add locking to force and signal functions klp_send_signals() and klp_force_transition() do not acquire klp_mutex, because it seemed to be superfluous. A potential race in klp_send_signals() was harmless and there was nothing in klp_force_transition() which needed to be synchronized. That changed with the addition of klp_forced variable during the review process. There is a small window now, when klp_complete_transition() does not see klp_forced set to true while all tasks have been already transitioned to the target state. module_put() is called and the module can be removed. Acquire klp_mutex in sysfs callback to prevent it. Do the same for the signal sending just to be sure. There is no real downside to that. Fixes: c99a2be790b07 ("livepatch: force transition to finish") Fixes: 43347d56c8d9d ("livepatch: send a fake signal to all blocking tasks") Reported-by: Jason Baron Signed-off-by: Miroslav Benes Reviewed-by: Petr Mladek Acked-by: Josh Poimboeuf Signed-off-by: Jiri Kosina --- kernel/livepatch/core.c | 52 ++++++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index 1c3c9b27c916..8fd8e8f126da 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -537,22 +537,24 @@ static ssize_t signal_store(struct kobject *kobj, struct kobj_attribute *attr, int ret; bool val; - patch = container_of(kobj, struct klp_patch, kobj); - - /* - * klp_mutex lock is not grabbed here intentionally. It is not really - * needed. The race window is harmless and grabbing the lock would only - * hold the action back. - */ - if (patch != klp_transition_patch) - return -EINVAL; - ret = kstrtobool(buf, &val); if (ret) return ret; - if (val) - klp_send_signals(); + if (!val) + return count; + + mutex_lock(&klp_mutex); + + patch = container_of(kobj, struct klp_patch, kobj); + if (patch != klp_transition_patch) { + mutex_unlock(&klp_mutex); + return -EINVAL; + } + + klp_send_signals(); + + mutex_unlock(&klp_mutex); return count; } @@ -564,22 +566,24 @@ static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr, int ret; bool val; - patch = container_of(kobj, struct klp_patch, kobj); - - /* - * klp_mutex lock is not grabbed here intentionally. It is not really - * needed. The race window is harmless and grabbing the lock would only - * hold the action back. - */ - if (patch != klp_transition_patch) - return -EINVAL; - ret = kstrtobool(buf, &val); if (ret) return ret; - if (val) - klp_force_transition(); + if (!val) + return count; + + mutex_lock(&klp_mutex); + + patch = container_of(kobj, struct klp_patch, kobj); + if (patch != klp_transition_patch) { + mutex_unlock(&klp_mutex); + return -EINVAL; + } + + klp_force_transition(); + + mutex_unlock(&klp_mutex); return count; }