Skip to content

Commit

Permalink
Merge pull request dynup#1318 from jpoimboe/static-call-patch-author-…
Browse files Browse the repository at this point in the history 
…guide

patch-author-guide: update jump label / static call descriptions
  • Loading branch information
@jpoimboe
jpoimboe authored Dec 2, 2022
2 parents a6aa2d8 + 48854d5 commit 059467d
Show file tree
Hide file tree
Showing 4 changed files with 126 additions and 85 deletions.
96 changes: 68 additions & 28 deletions doc/patch-author-guide.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
-------------
Expand Down
95 changes: 48 additions & 47 deletions kpatch-build/create-diff-object.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1759,14 +1759,14 @@ static void kpatch_include_symbol(struct symbol *sym)
/*
* The symbol gets included even if its section isn't needed, as it
* might be needed: either permanently for a rela, or temporarily for
* the later creation of a dynrela.
* the later creation of a klp relocation.
*/
sym->include = 1;

/*
* For a function/object symbol, if it has a section, we only need to
* include the section if it has changed. Otherwise the symbol will be
* used by relas/dynrelas to link to the real symbol externally.
* used by relas/klp_relocs to link to the real symbol externally.
*
* For section symbols, we always include the section because
* references to them can't otherwise be resolved externally.
Expand Down Expand Up @@ -1800,8 +1800,8 @@ static void kpatch_include_standard_elements(struct kpatch_elf *kelf)
*
* Note that if any of these sections have rela sections, they
* will also be included in their entirety. That may result in
* some extra (unused) dynrelas getting created, which should
* be harmless.
* some extra (unused) klp relocations getting created, which
* should be harmless.
*/
if (!strcmp(sec->name, ".shstrtab") ||
!strcmp(sec->name, ".strtab") ||
Expand Down Expand Up @@ -2284,8 +2284,8 @@ static bool jump_table_group_filter(struct lookup_table *lookup,
* (in the klp module load) as normal relas, before jump label init.
* On the other hand, jump labels based on static keys which are
* defined in modules aren't supported, because late module patching
* can result in the klp relas getting applied *after* the klp module's
* jump label init.
* can result in the klp relocations getting applied *after* the klp
* module's jump label init.
*/

if (lookup_symbol(lookup, key->sym, &symbol) &&
Expand Down Expand Up @@ -3204,8 +3204,8 @@ static int function_ptr_rela(const struct rela *rela)
rela->type == R_PPC64_TOC16_LO_DS));
}

static bool need_dynrela(struct kpatch_elf *kelf, struct lookup_table *table,
struct section *relasec, const struct rela *rela)
static bool need_klp_reloc(struct kpatch_elf *kelf, struct lookup_table *table,
struct section *relasec, const struct rela *rela)
{
struct lookup_result symbol;

Expand All @@ -3214,7 +3214,7 @@ static bool need_dynrela(struct kpatch_elf *kelf, struct lookup_table *table,

/*
* These references are treated specially by the module loader and
* should never be converted to dynrelas.
* should never be converted to klp relocations.
*/
if (rela->type == R_PPC64_REL16_HA || rela->type == R_PPC64_REL16_LO ||
rela->type == R_PPC64_ENTRY)
Expand Down Expand Up @@ -3259,16 +3259,16 @@ static bool need_dynrela(struct kpatch_elf *kelf, struct lookup_table *table,

if (rela->sym->sec) {
/*
* Internal symbols usually don't need dynrelas, because they
* live in the patch module and can be relocated normally.
* Internal symbols usually don't need klp relocations, because
* they live in the patch module and can be relocated normally.
*
* There's one exception: function pointers.
*
* If the rela references a function pointer, we convert it to
* a dynrela, so that the function pointer will refer to the
* original function rather than the patched function. This
* can prevent crashes in cases where the function pointer is
* called asynchronously after the patch module has been
* a klp relocation, so that the function pointer will refer to
* the original function rather than the patched function.
* This can prevent crashes in cases where the function pointer
* is called asynchronously after the patch module has been
* unloaded.
*/
if (!function_ptr_rela(rela))
Expand All @@ -3279,13 +3279,13 @@ static bool need_dynrela(struct kpatch_elf *kelf, struct lookup_table *table,
* special case. They are not supposed to be visible outside
* of the function that defines them. Their names may differ
* in the original and the patched kernels which makes it
* difficult to use dynrelas. Fortunately, nested functions
* are rare and are unlikely to be used as asynchronous
* callbacks, so the patched code can refer to them directly.
* It seems, one can only distinguish such functions by their
* names containing a dot. Other kinds of functions with such
* names (e.g. optimized copies of functions) are unlikely to
* be used as callbacks.
* difficult to use klp relocations. Fortunately, nested
* functions are rare and are unlikely to be used as
* asynchronous callbacks, so the patched code can refer to
* them directly. It seems, one can only distinguish such
* functions by their names containing a dot. Other kinds of
* functions with such names (e.g. optimized copies of
* functions) are unlikely to be used as callbacks.
*
* Function pointers to *new* functions don't have this issue,
* just use a normal rela for them.
Expand All @@ -3309,8 +3309,9 @@ static bool need_dynrela(struct kpatch_elf *kelf, struct lookup_table *table,
rela->sym->name);

/*
* The symbol is (formerly) local. Use a dynrela to access the
* original version of the symbol in the patched object.
* The symbol is (formerly) local. Use a klp relocation to
* access the original version of the symbol in the patched
* object.
*/
return true;
}
Expand All @@ -3322,9 +3323,9 @@ static bool need_dynrela(struct kpatch_elf *kelf, struct lookup_table *table,
* On powerpc, the symbol is global and exported, but
* it was also in the changed object file. In this
* case the rela refers to the 'localentry' point, so a
* normal rela wouldn't work. Force a dynrela so it
* can be handled correctly by the livepatch relocation
* code.
* normal rela wouldn't work. Force a klp relocation
* so it can be handled correctly by the livepatch
* relocation code.
*/
return true;
}
Expand All @@ -3340,17 +3341,18 @@ static bool need_dynrela(struct kpatch_elf *kelf, struct lookup_table *table,
/*
* The symbol is exported by the to-be-patched module, or by
* another module which the patched module depends on. Use a
* dynrela because of late module loading: the patch module may
* be loaded before the to-be-patched (or other) module.
* klp relocation because of late module loading: the patch
* module may be loaded before the to-be-patched (or other)
* module.
*/
return true;
}

if (symbol.global) {
/*
* The symbol is global in the to-be-patched object, but not
* exported. Use a dynrela to work around the fact that it's
* an unexported sybmbol.
* exported. Use a klp relocation to work around the fact that
* it's an unexported sybmbol.
*/
return true;
}
Expand All @@ -3366,9 +3368,8 @@ static bool need_dynrela(struct kpatch_elf *kelf, struct lookup_table *table,
/*
* kpatch_create_intermediate_sections()
*
* The primary purpose of this function is to convert some relas (also known as
* relocations) to dynrelas (also known as dynamic relocations or livepatch
* relocations or klp relas).
* The primary purpose of this function is to convert some relocations to klp
* relocations.
*
* If the patched code refers to a symbol, for example, if it calls a function
* or stores a pointer to a function somewhere or accesses some global data,
Expand All @@ -3377,7 +3378,7 @@ static bool need_dynrela(struct kpatch_elf *kelf, struct lookup_table *table,
*
* If the symbol lives outside the patch module, and if it's not exported by
* vmlinux (e.g., with EXPORT_SYMBOL) then the rela needs to be converted to a
* dynrela so the livepatch code can resolve it at runtime.
* klp relocation so the livepatch code can resolve it at runtime.
*/
static void kpatch_create_intermediate_sections(struct kpatch_elf *kelf,
struct lookup_table *table,
Expand Down Expand Up @@ -3408,18 +3409,18 @@ static void kpatch_create_intermediate_sections(struct kpatch_elf *kelf,
nr++;

/*
* We set 'need_dynrela' here in the first pass because
* the .toc section's 'need_dynrela' values are
* dependent on all the other sections. Otherwise, if
* we did this analysis in the second pass, we'd have
* to convert .toc dynrelas at the very end.
* We set 'need_klp_reloc' here in the first pass
* because the .toc section's 'need_klp_reloc' values
* are dependent on all the other sections. Otherwise,
* if we did this analysis in the second pass, we'd
* have to convert .toc klp relocations at the very end.
*
* Specifically, this is needed for the powerpc
* internal symbol function pointer check which is done
* via .toc indirection in need_dynrela().
* via .toc indirection in need_klp_reloc().
*/
if (need_dynrela(kelf, table, relasec, rela))
toc_rela(rela)->need_dynrela = 1;
if (need_klp_reloc(kelf, table, relasec, rela))
toc_rela(rela)->need_klp_reloc = true;
}
}

Expand Down Expand Up @@ -3464,7 +3465,7 @@ static void kpatch_create_intermediate_sections(struct kpatch_elf *kelf,
}

list_for_each_entry_safe(rela, safe, &relasec->relas, list) {
if (!rela->need_dynrela) {
if (!rela->need_klp_reloc) {
rela->sym->strip = SYMBOL_USED;
continue;
}
Expand All @@ -3483,7 +3484,7 @@ static void kpatch_create_intermediate_sections(struct kpatch_elf *kelf,
* runs due to late module patching.
*/
if (!KLP_ARCH && !vmlinux && special)
ERROR("unsupported dynrela reference to symbol '%s' in module-specific special section '%s'",
ERROR("unsupported klp relocation reference to symbol '%s' in module-specific special section '%s'",
rela->sym->name, relasec->base->name);

if (!lookup_symbol(table, rela->sym, &symbol))
Expand Down Expand Up @@ -3764,7 +3765,7 @@ static void kpatch_create_mcount_sections(struct kpatch_elf *kelf)
/*
* This function strips out symbols that were referenced by changed rela
* sections, but the rela entries that referenced them were converted to
* dynrelas and are no longer needed.
* klp relocations and are no longer needed.
*/
static void kpatch_strip_unneeded_syms(struct kpatch_elf *kelf,
struct lookup_table *table)
Expand Down Expand Up @@ -4090,7 +4091,7 @@ int main(int argc, char *argv[])

kpatch_no_sibling_calls_ppc64le(kelf_out);

/* create strings, patches, and dynrelas sections */
/* create strings, patches, and klp relocation sections */
kpatch_create_strings_elements(kelf_out);
kpatch_create_patches_sections(kelf_out, lookup, parent_name);
kpatch_create_intermediate_sections(kelf_out, lookup, parent_name, patch_name);
Expand Down
Loading

0 comments on commit 059467d

Please sign in to comment.