kpatch/0052-patch-author-guide-update-jump-label-static-call-des.patch

From 41128c0987ea569dd623d2de70391c5be739d38e Mon Sep 17 00:00:00 2001
From: Josh Poimboeuf <jpoimboe@redhat.com>
Date: Wed, 30 Nov 2022 18:48:34 -0800
Subject: [PATCH] patch-author-guide: update jump label / static call
 descriptions

Now that we have KPATCH_STATIC_CALL(), document its usage.  While at it,
give a more thorough description for why jump labels and static calls
aren't supported in some scenarios.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 doc/patch-author-guide.md | 96 +++++++++++++++++++++++++++------------
 1 file changed, 68 insertions(+), 28 deletions(-)

diff --git a/doc/patch-author-guide.md b/doc/patch-author-guide.md
index 0133cec..26daee3 100644
--- a/doc/patch-author-guide.md
+++ b/doc/patch-author-guide.md
@@ -24,7 +24,7 @@ Table of contents
 - [Code removal](#code-removal)
 - [Once macros](#once-macros)
 - [inline implies notrace](#inline-implies-notrace)
-- [Jump labels](#jump-labels)
+- [Jump labels and static calls](#jump-labels-and-static-calls)
 - [Sibling calls](#sibling-calls)
 - [Exported symbol versioning](#exported-symbol-versioning)
 - [System calls](#system-calls)
@@ -747,41 +747,81 @@ changes to all of `__tcp_mtu_to_mss()` callers (ie, it was inlined as
 requested).  In this case, a simple workaround is to specify
 `__tcp_mtu_to_mss()` as `__always_inline` to force the compiler to do so.
 
-Jump labels
------------
+Jump labels and static calls
+----------------------------
+
+### Late module patching vs special section relocations
+
+Jump labels and static calls can be problematic due to "late module patching",
+which is a feature (design flaw?) in upstream livepatch.  When a livepatch
+module patches another module, unfortunately the livepatch module doesn't have
+an official module dependency on the patched module.  That means the patched
+module doesn't even have to be loaded when the livepatch module gets loaded.
+In that case the patched module gets patched on demand whenever it might get
+loaded in the future.  It also gets unpatched on demand whenever it gets
+unloaded.
+
+Loading (and patching) the module at some point after loading the livepatch
+module is called "late module patching".  In order to support this
+(mis?)feature, all relocations in the livepatch module which reference module
+symbols must be converted to "klp relocations", which get resolved at patching
+time.
+
+In all modules (livepatch and otherwise), jump labels and static calls rely on
+special sections which trigger jump-label/static-call code patching when a
+module gets loaded.  But unfortunately those special sections have relocations
+which need to get resolved, so there's an ordering issue.
+
+When a (livepatch) module gets loaded, first its relocations are resolved, then
+its special section handling (and code patching) is done.  The problem is, for
+klp relocations, if they reference another module's symbols, and that module
+isn't loaded, they're not yet defined.  So if a `.static_call_sites` entry
+tries to reference its corresponding `struct static_call_key`, but that key
+lives in another module which is not yet loaded, the key reference won't be
+resolved, and so `mod->static_call_sites` will be corrupted when
+`static_call_module_notify()` runs when the livepatch module first loads.
+
+### Jump labels
+
+With pre-5.8 kernels, kpatch-build will error out if it encounters any jump
+labels:
+```
+oom_kill.o: Found a jump label at out_of_memory()+0x10a, using key cpusets_enabled_key.  Jump labels aren't supported with this kernel.  Use static_key_enabled() instead.
+```
 
-When modifying a function that contains a jump label, kpatch-build may
-return an error like: `ERROR: oom_kill.o: kpatch_regenerate_special_section: 2109: Found a jump label at out_of_memory()+0x10a, using key cpusets_enabled_key.  Jump labels aren't currently supported.  Use static_key_enabled() instead.`
+With Linux 5.8+, klp relocation handling is integrated with the module relocation
+code, so jump labels in patched functions are supported when the static key was
+originally defined in the kernel proper (vmlinux).
 
-This is due to a limitation in the kernel to process static key
-livepatch relocations (resolved by late-module patching).  Older
-versions of kpatch-build may have reported successfully building
-kpatch module, but issue
-[#931](https://github.com/dynup/kpatch/issues/931) revealed potentially
-dangerous behavior if the static key value had been modified from its
-compiled default.
+However, if the static key lives in a module, jump labels are _not_ supported
+in patched code, due to the ordering issue described above.  If the jump label
+is a tracepoint, kpatch-build will silently remove the tracepoint.  Otherwise,
+there will be an error:
+```
+vmx.o: Found a jump label at vmx_hardware_enable.cold()+0x23, using key enable_evmcs, which is defined in a module.  Use static_key_enabled() instead.
+```
 
-The current workaround is to remove the jump label by explictly checking
-the static key:
+When you get one of the above errors, the fix is to remove the jump label usage
+in the patched function, replacing it with a regular C conditional.
 
-```c
-DEFINE_STATIC_KEY_TRUE(true_key);
-DEFINE_STATIC_KEY_FALSE(false_key);
+This can be done by replacing any usages of `static_branch_likely()`,
+`static_branch_unlikely()`, `static_key_true()`, and `static_key_false()` with
+`static_key_enabled()` in the patch file.
 
-/* unsupported */
-if (static_key_true(&true_key))
-if (static_key_false(&false_key))
-if (static_branch_likely(&key))
+### Static calls
 
-/* supported */
-if (static_key_enabled(&true_key))
-if (static_key_enabled(&false_key))
-if (likely(static_key_enabled(&key)))
+Similarly, static calls are not supported when the corresponding static call
+key was originally defined in a module.  If such a static call is part of a
+tracepoint, kpatch-build will silently remove it.  Otherwise, there will be an
+error:
+```
+cpuid.o: Found a static call at kvm_set_cpuid.cold()+0x32c, using key __SCK__kvm_x86_vcpu_after_set_cpuid, which is defined in a module.  Use KPATCH_STATIC_CALL() instead.
 ```
 
-Note that with Linux 5.8+, jump labels in patched functions are now supported
-when the static key was originally defined in the kernel proper (vmlinux).  The
-above error will not be seen unless the static key lives in a module.
+To fix this error, simply replace such static calls with regular indirect
+branches (or retpolines, if applicable) by adding `#include "kpatch-macros.h"`
+to the patch source and replacing usages of `static_call()` with
+`KPATCH_STATIC_CALL()`.
 
 Sibling calls
 -------------
-- 
2.27.0