From: "Madhavan T. Venkataraman" <[email protected]>
Introduction
============
The livepatch feature requires an unwinder that can provide a reliable stack
trace. General requirements for a reliable unwinder are described in this
document from Mark Rutland:
Documentation/livepatch/reliable-stacktrace.rst
The requirements have two parts:
1. The unwinder must be enhanced with certain features. E.g.,
- Identifying successful termination of stack trace
- Identifying unwindable and non-unwindable code
- Identifying interrupts and exceptions occurring in the frame pointer
prolog and epilog
- Identifying features such as kretprobe and ftrace graph tracing
that can modify the return address stored on the stack
- Identifying corrupted/unreliable stack contents
- Architecture-specific items that can render a stack trace unreliable
at certain points in code
Some of these features are already in the arm64 unwinder. I am pursuing
the rest in another patch series. This is work in progress. The latest
submission as of this writing is here:
https://lore.kernel.org/linux-arm-kernel/[email protected]/T/#t
2. Validation of the frame pointer
This assumes that the unwinder is based on the frame pointer (FP).
The actual frame pointer that the unwinder uses cannot just be
assumed to be correct. It needs to be validated somehow.
This patch series is to address this requirement.
Validation of the FP (aka STACK_VALIDATION)
====================
The current approach in Linux is to use objtool, a build time tool, for this
purpose. When configured, objtool is invoked on every relocatable object file
during kernel build. It performs static analysis of the code in each file. It
walks the instructions in every function and notes the changes to the stack
pointer (SP) and the frame pointer (FP). It makes sure that the changes are in
accordance with the ABI rules. There are also a lot of other checks that
Objtool performs. Once objtool completes successfully, the kernel can then be
used for livepatch purposes.
Objtool can have uses other than just FP validation. For instance, it can check
control flow integrity during its analysis.
Problem
=======
Objtool is complex and highly architecture-dependent. There are a lot of
different checks in objtool that all of the code in the kernel must pass
before livepatch can be enabled. If a check fails, it must be corrected
before we can proceed. Sometimes, the kernel code needs to be fixed.
Sometimes, it is a compiler bug that needs to be fixed. The challenge is
also to prove that all the work is complete for an architecture.
As such, it presents a great challenge to enable livepatch for an
architecture.
A different approach
====================
I would like to propose a different approach for FP validation. I would
like to be able to enable livepatch for an architecture as is. That is,
without "fixing" the kernel or the compiler for it:
There are two steps in this:
1. Objtool walks all the functions as usual. It computes the stack and
frame pointer offsets at each instruction as usual. It generates ORC
records and stores them in special sections as usual. This is simple
enough to do.
What Objtool will not do is perform any checks for ABI compliance, etc.
2. The unwinder in the kernel retrieves the ORC record for each return
address in a stack trace and computes the frame pointer from it. It
compares the computed frame pointer with the actual frame pointer. If
there is a match, the frame is reliable. If not, it isn't. A stack trace
is reliable if every single frame in it is reliable.
To summarize, the frame pointer validation is done dynamically instead
of statically.
Using this scheme, the unwinder can always know what kernel code is reliable
for unwind and what is not. This is the requirement for livepatch.
Instruction decoder
===================
To do this, an instruction decoder needs to be implemented. I have implemented
a simple, table-driven decoder for ARM64. Only a subset of the instructions
needs to be fully decoded for this purpose:
- Load-Store instructions
- Add/Sub/Mov instructions
- Branch instructions
- Call instructions
- Return instructions
The rest of the instructions are either dont-care from an unwind perspective
or unexpected from the compiler. I have added checks for the unexpected ones
to catch them if the compiler ever generates them.
This decoder is simpler than a full-fledged one. But if a full-fledged one
is ever implemented, my decoder can be subsumed by it.
Code reorganization and reuse
=============================
Stack validation scheme
-----------------------
Currently, the stack validation scheme supported in Objtool is static stack
validation. Static stack validation is performed in check.c. There is a lot
of code in check.c that should be shared with other validation schemes such
as the dynamic FP validation scheme that I am proposing. Accordingly, I have
moved that code into separate files:
- Code that walks instructions and decodes them
- Code that manages instructions
- CFI related code
- Code that handles unwind hints
So, all of this is shared across all architectures and validation schemes.
Architecture-dependent code
---------------------------
Currently, the ORC definitions and code are X86-specific. I have separated
out the architecture-specific stuff from the generic stuff and placed
them in appropriate files so other architectures can share.
So, these are the architecture-specific parts that need to be supplied for
a new architecture for my proposal. Everything else is shared.
- Instruction decoder as mentioned above
- ORC register definitions
- ORC support functions
- Unwind hint support
- Invoke ORC init from kernel initialization code
- Invoke ORC init from module initialization code
- Add ORC_UNWIND_TABLE to kernel data in vmlinux.lds.S
- Modify the unwinder to use ORC data to validate the frame pointer
- Add kernel config definitions for reliable stack trace and livepatch
Other than the decoder, all of this is very simple to do. Just follow the
example in ARM64.
For ARM64, the decoder turned out to be fairly simple. I cannot speak to
other architectures.
sorttable
---------
At build time, the ORC tables in special sections are sorted so that the
kernel does not have to spend time sorting them. The tables need to be
sorted for binary search. The sorttable program works without any change
in my proposal as well.
FP prolog, epilog, leaf functions, generated code, etc
======================================================
If the unwinder is not able to find an ORC record for a given instruction
address, it considers the code to be unreliable from an unwind perspective.
This enables the unwinder to deal with:
- Sections of code that Objtool ignores. E.g., on ARM64, .head.text
is ignored.
- Generated code that will not have any ORC records.
If the unwinder finds an ORC record, but the computed frame pointer does not
match the actual one, it considers the code to be unreliable from an unwind
perspective. This enables the unwinder to deal with:
- Interrupts/exceptions in frame pointer prologs and epilogs.
- Interrupts/exceptions in leaf functions that don't have a frame
setup.
- Compiler not setting up the frame pointer properly before calling
a function. E.g., if inline assembly code occurs at the beginning
of a function and it contains a call.
Assembly functions
==================
Objtool does not walk SYM_CODE functions as they are low-level functions
that don't follow ABI rules or functions that manipulate register state
in such a way that unwind is unreliable. For these the ORC records will
show that the frame offset is 0. So, the unwinder will be able to tell that
they are unreliable for unwind.
As for SYM_FUNC functions, Objtool will walk them and compute ORC. However,
currently, most of the SYM_FUNC functions in ARM64 do not setup a frame.
So, these will look unreliable to the unwinder. While this will not impact
the ability to do livepatch, I plan to submit a separate patch series to add
a frame pointer prolog and epilog to many of these functions. This is to
reduce the number of retries during the livepatch process.
Unwind hints
============
Now, there are certain points in assembly code that we would like to unwind
through reliably. Like interrupt and exception handlers. This is mainly for
getting reliable stack traces in these cases and reducing the number of
retries during the livepatch process. For these, unwind hints can be placed
at strategic points in assembly code. Only a small number of these hints
should be needed.
In this work, I have defined the following unwind hints so stack traces that
contain these can be done reliably:
- Exception handlers
- Interrupt handlers
- FTrace tracer functions
- FTrace graph return prepare code
- FTrace callsites
- Kretprobe Trampoline
Unwind hints are collected in a special section. Objtool converts unwind hints
to ORC data just like the CFI based ones. The unwinder processes unwind hints
to handle special cases mentioned above.
Size of the memory consumed within the kernel for this feature
==============================================================
This depends on the amount of code in the kernel which, in turn, depends on
the number of configs turned on. E.g., on the kernel on my arm64 system, the
ORC data size for vmlinux is about 2MB.
GitHub repository
=================
My github repo for this version is here:
https://github.com/madvenka786/linux/tree/orc_v2
Please feel free to clone and check it out. And, please let me know if you
find any issues.
Testing
=======
- I have run all of the livepatch selftests successfully. I have written a
couple of extra selftests myself which I will be posting separately.
- I have run ftrace tests and taken stacktraces from tracer functions and
graph trace functions to make sure that unwinding through ftrace entry
code is reliable.
- I have run a kretprobe test to make sure that unwinding through the
kretprobe trampoline is reliable.
- I have a test driver to induce a NULL pointer exception to make sure
that unwinding through exception handlers is reliable.
- I use the test driver to create a timer to make sure that unwinding through
the timer IRQ is reliable.
- I call the unwinder from different places during boot to make sure that
the unwinding in each of those cases is reliable.
TBD
===
- I need to perform more rigorous testing with different scenarios. This
is work in progress. Any ideas or suggestions are welcome.
- I plan to add a return address check in the unwinder. The unwinder will
decode the instruction at the call site for each frame and make sure that
it is a valid call instruction. This is just a paranoid check to catch it
if Objtool generates an incorrect ORC entry or if the FP is corrupted.
---
Changelog:
v2:
From Josh Poimboeuf <[email protected]>:
==========================================
DWARF is not proven to be reliable. So, depending on it for livepatch
is a problem.
I have removed the DWARF part from the patch series. Instead,
I have implemented the minimum ARM64 instruction decoder
required for this work. I have implemented code to walk all
the instructions in an object file and generate ORC data.
The ORC data will be used by the unwinder to compute a
frame pointer and validate the actual frame pointer with it.
Unwind hints are a problem from a maintenance perspective.
This is true. But there are only a few unwind hints that I
have introduced in this work. Also, if an unwind hint becomes
outdated, the dynamic frame pointer check will catch it so
that the unwinder will know that it is unreliable.
Inline ASM code can cause problems that DWARF cannot catch.
Now that I walk all of the instructions, this problem is solved.
In version 2, the ORC data is generated based on the actual
machine code in the object file. So, the data reflects the
actual code. Unreliability in any part of the code will be
caught by the unwinder when it looks up the ORC data and
performs a frame pointer check.
Rename kernel code and data that currently contains the name dwarf
to avoid confusion.
This problem is solved as I have dropped DWARF altogether.
Try to reuse the existing ORC data format and code as much as possible.
I have reorganized the code in the following ways:
- I have placed code that was in check.c in separate files
so that different stack validation schemes can share the code.
- I have separated architecture-specific code and structures
from generic ones so that different architectures can share
common stuff.
- I am using the ORC structure as it is currently defined. The
only cosmetic change I have done is to rename the fields
bp_* to fp_* (FP for frame pointer).
- I completely reuse the ORC definitions and code. E.g., in
the kernel the ORC lookup code is shared across architectures.
Objtool contains other features which other architectures are looking
into. So, should we just implement static stack validation for other
architectures or use dynamic FP validation just for the livepatch
feature?
For one thing, it will take a long time before the static
validation scheme can even be proved to be complete on ARM64.
Livepatching is an immediate need for security fixes.
Also, since I am using the traditional approach in v2 of
walking the instructions, computing CFI, generating ORC, etc,
my current approach can be combined with the traditional
approach. Dynamic FP validation can be offered either as an
alternative to static stack validation or as something that
can be combined with static stack validation to make the
feature even more robust. Objtool can always have bugs and
there can be bad ORC data.
From Peter Zijlstra <[email protected]>:
===========================================
Please use/extend ORC.
Done. Please see my description above.
Why deviate from the traditional approach of static stack validation?
I have given the answer above.
Mandating DWARF sucks. Compile times are so much worse with DWARVES.
I have removed DWARF from the work. I use the traditional
approach of decoding the instructions and computing the
same data as DWARF but in ORC format.
DWARF does not cover assembly code.
In v2, I walk all of the functions including assembly functions.
SYM_CODE functions are not walked by Objtool anyway. So, I
don't do that. But I walk all the SYM_FUNC functions. Currently,
only a few SYM_FUNC functions in ARM64 have a proper FP prolog
and epilog. So, I plan to submit a separate patch series to
add an FP prolog and epilog for other SYM_FUNC functions.
But this is only to reduce retries during the livepatch process.
It is not absolutely required for livepatch to work. But I
plan to address this separately.
Compilers don't consider DWARF generation to be a correctness issue.
I totally agree. I have myself found 4 bugs that I have had
to compensate for. So, I have dropped DWARF.
From Chen Zhongjin <[email protected]>:
=============================================
One cannot depend on compilers to generate correct DWARF info.
Agreed. I have dropped DWARF.
DWARF does not cover assembly. So, what if too many assembly
functions exist so that the livepatch process can encounter too
many retries?
DWARF has been dropped. The code in version 2 walks all the
functions including assembly functions.
There is a corner case where an interrupt or an exception can happen
in FP prologs/epilogs. The stack trace would be unreliable.
Yes. This will be caught by the reliable unwinder in the kernel
in my scheme when it retrieves the ORC data and validates
the actual frame pointer. The validation will fail and the
stack trace will be considered unreliable.
v1:
- Introduced the livepatch feature based on DWARF Call Frame
Information generated by the compilers.
Previous versions and discussion
================================
v1: https://lore.kernel.org/linux-arm-kernel/[email protected]/T/#t
Madhavan T. Venkataraman (20):
objtool: Reorganize CFI code
objtool: Reorganize instruction-related code
objtool: Move decode_instructions() to a separate file
objtool: Reorganize Unwind hint code
objtool: Reorganize ORC types
objtool: Reorganize ORC code
objtool: Reorganize ORC kernel code
objtool: arm64: Implement decoder for FP validation
objtool: arm64: Implement command to invoke the decoder
objtool: arm64: Compute destinations for call and jump instructions
objtool: arm64: Walk instructions and compute CFI for each instruction
objtool: arm64: Generate ORC data from CFI for object files
objtool: arm64: Dump ORC data present in object files
objtool: arm64: Add unwind hint support
arm64: Add unwind hints to specific points in code
arm64: Add kernel and module support for ORC
arm64: Build the kernel with ORC information
arm64: unwinder: Add a reliability check in the unwinder based on ORC
arm64: Miscellaneous changes required for enabling livepatch
arm64: Enable livepatch for ARM64
arch/Kconfig | 4 +-
arch/arm64/Kconfig | 7 +
arch/arm64/Kconfig.debug | 21 +
arch/arm64/include/asm/livepatch.h | 42 ++
arch/arm64/include/asm/module.h | 10 +-
arch/arm64/include/asm/orc_types.h | 35 ++
arch/arm64/include/asm/stacktrace.h | 9 +
arch/arm64/include/asm/thread_info.h | 4 +-
arch/arm64/include/asm/unwind_hints.h | 104 ++++
arch/arm64/kernel/entry-ftrace.S | 23 +
arch/arm64/kernel/entry.S | 3 +
arch/arm64/kernel/ftrace.c | 16 +
arch/arm64/kernel/module.c | 13 +-
arch/arm64/kernel/probes/kprobes_trampoline.S | 3 +
arch/arm64/kernel/setup.c | 2 +
arch/arm64/kernel/signal.c | 4 +
arch/arm64/kernel/stacktrace.c | 153 ++++++
arch/arm64/kernel/vmlinux.lds.S | 3 +
arch/x86/include/asm/orc_types.h | 37 +-
arch/x86/include/asm/unwind.h | 5 -
arch/x86/include/asm/unwind_hints.h | 85 ++++
arch/x86/kernel/module.c | 7 +-
arch/x86/kernel/unwind_orc.c | 258 +---------
arch/x86/kernel/vmlinux.lds.S | 2 +-
.../asm => include/asm-generic}/orc_lookup.h | 42 ++
include/linux/ftrace.h | 4 +
include/linux/objtool.h | 73 +--
include/linux/orc_entry.h | 39 ++
kernel/Makefile | 2 +
kernel/orc_lookup.c | 261 ++++++++++
scripts/Makefile | 4 +-
scripts/Makefile.build | 4 +
scripts/link-vmlinux.sh | 7 +
tools/arch/arm64/include/asm/orc_types.h | 35 ++
tools/arch/arm64/include/asm/unwind_hints.h | 104 ++++
tools/arch/x86/include/asm/orc_types.h | 37 +-
tools/arch/x86/include/asm/unwind_hints.h | 147 ++++++
tools/include/linux/objtool.h | 73 +--
tools/include/linux/orc_entry.h | 39 ++
tools/objtool/Build | 16 +
tools/objtool/Makefile | 6 +-
tools/objtool/arch/arm64/Build | 2 +
tools/objtool/arch/arm64/decode.c | 389 +++++++++++++++
.../arch/arm64/include/arch/cfi_regs.h | 12 +
tools/objtool/arch/arm64/include/arch/elf.h | 9 +
.../arch/arm64/include/arch/endianness.h | 9 +
tools/objtool/arch/arm64/orc.c | 92 ++++
tools/objtool/arch/x86/Build | 1 +
tools/objtool/arch/x86/include/arch/elf.h | 1 +
tools/objtool/arch/x86/orc.c | 150 ++++++
tools/objtool/builtin-fpv.c | 79 +++
tools/objtool/cfi.c | 108 ++++
tools/objtool/check.c | 460 ------------------
tools/objtool/decode.c | 106 ++++
tools/objtool/fpv.c | 272 +++++++++++
tools/objtool/include/objtool/arch.h | 1 +
tools/objtool/include/objtool/builtin.h | 1 +
tools/objtool/include/objtool/cfi.h | 12 +
tools/objtool/include/objtool/check.h | 74 +--
tools/objtool/include/objtool/endianness.h | 1 +
tools/objtool/include/objtool/insn.h | 125 +++++
tools/objtool/include/objtool/objtool.h | 2 +
tools/objtool/include/objtool/orc.h | 18 +
tools/objtool/insn.c | 197 ++++++++
tools/objtool/objtool.c | 12 +-
tools/objtool/orc_dump.c | 63 +--
tools/objtool/orc_gen.c | 89 +---
tools/objtool/sync-check.sh | 10 +
tools/objtool/unwind_hints.c | 91 ++++
tools/objtool/weak.c | 5 +
70 files changed, 3005 insertions(+), 1129 deletions(-)
create mode 100644 arch/arm64/include/asm/livepatch.h
create mode 100644 arch/arm64/include/asm/orc_types.h
create mode 100644 arch/arm64/include/asm/unwind_hints.h
rename {arch/x86/include/asm => include/asm-generic}/orc_lookup.h (51%)
create mode 100644 include/linux/orc_entry.h
create mode 100644 kernel/orc_lookup.c
create mode 100644 tools/arch/arm64/include/asm/orc_types.h
create mode 100644 tools/arch/arm64/include/asm/unwind_hints.h
create mode 100644 tools/arch/x86/include/asm/unwind_hints.h
create mode 100644 tools/include/linux/orc_entry.h
create mode 100644 tools/objtool/arch/arm64/Build
create mode 100644 tools/objtool/arch/arm64/decode.c
create mode 100644 tools/objtool/arch/arm64/include/arch/cfi_regs.h
create mode 100644 tools/objtool/arch/arm64/include/arch/elf.h
create mode 100644 tools/objtool/arch/arm64/include/arch/endianness.h
create mode 100644 tools/objtool/arch/arm64/orc.c
create mode 100644 tools/objtool/arch/x86/orc.c
create mode 100644 tools/objtool/builtin-fpv.c
create mode 100644 tools/objtool/cfi.c
create mode 100644 tools/objtool/decode.c
create mode 100644 tools/objtool/fpv.c
create mode 100644 tools/objtool/include/objtool/insn.h
create mode 100644 tools/objtool/include/objtool/orc.h
create mode 100644 tools/objtool/insn.c
create mode 100644 tools/objtool/unwind_hints.c
base-commit: 3123109284176b1532874591f7c81f3837bbdc17
--
2.25.1
From: "Madhavan T. Venkataraman" <[email protected]>
check.c implements static stack validation. But decode_instructions() which
resides in it can be shared with other types of validation. E.g., dynamic
FP validation. Move the function to its own file - decode.c.
Signed-off-by: Madhavan T. Venkataraman <[email protected]>
---
tools/objtool/Build | 2 +
tools/objtool/check.c | 96 ------------------------
tools/objtool/decode.c | 106 +++++++++++++++++++++++++++
tools/objtool/include/objtool/insn.h | 1 +
4 files changed, 109 insertions(+), 96 deletions(-)
create mode 100644 tools/objtool/decode.c
diff --git a/tools/objtool/Build b/tools/objtool/Build
index 52ed2f710d2a..199561f86c1e 100644
--- a/tools/objtool/Build
+++ b/tools/objtool/Build
@@ -5,10 +5,12 @@ objtool-y += weak.o
objtool-$(SUBCMD_CHECK) += check.o
objtool-$(SUBCMD_CHECK) += cfi.o
objtool-$(SUBCMD_CHECK) += insn.o
+objtool-$(SUBCMD_CHECK) += decode.o
objtool-$(SUBCMD_CHECK) += special.o
objtool-$(SUBCMD_ORC) += check.o
objtool-$(SUBCMD_ORC) += cfi.o
objtool-$(SUBCMD_ORC) += insn.o
+objtool-$(SUBCMD_ORC) += decode.o
objtool-$(SUBCMD_ORC) += orc_gen.o
objtool-$(SUBCMD_ORC) += orc_dump.o
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 78168e0ad2bf..334ddc737bf9 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -163,104 +163,8 @@ static bool dead_end_function(struct objtool_file *file, struct symbol *func)
return __dead_end_function(file, func, 0);
}
-static unsigned long nr_insns;
static unsigned long nr_insns_visited;
-/*
- * Call the arch-specific instruction decoder for all the instructions and add
- * them to the global instruction list.
- */
-static int decode_instructions(struct objtool_file *file)
-{
- struct section *sec;
- struct symbol *func;
- unsigned long offset;
- struct instruction *insn;
- int ret;
-
- for_each_sec(file, sec) {
-
- if (!(sec->sh.sh_flags & SHF_EXECINSTR))
- continue;
-
- if (strcmp(sec->name, ".altinstr_replacement") &&
- strcmp(sec->name, ".altinstr_aux") &&
- strncmp(sec->name, ".discard.", 9))
- sec->text = true;
-
- if (!strcmp(sec->name, ".noinstr.text") ||
- !strcmp(sec->name, ".entry.text"))
- sec->noinstr = true;
-
- for (offset = 0; offset < sec->sh.sh_size; offset += insn->len) {
- insn = malloc(sizeof(*insn));
- if (!insn) {
- WARN("malloc failed");
- return -1;
- }
- memset(insn, 0, sizeof(*insn));
- INIT_LIST_HEAD(&insn->alts);
- INIT_LIST_HEAD(&insn->stack_ops);
- INIT_LIST_HEAD(&insn->call_node);
-
- insn->sec = sec;
- insn->offset = offset;
-
- ret = arch_decode_instruction(file, sec, offset,
- sec->sh.sh_size - offset,
- &insn->len, &insn->type,
- &insn->immediate,
- &insn->stack_ops);
- if (ret)
- goto err;
-
- /*
- * By default, "ud2" is a dead end unless otherwise
- * annotated, because GCC 7 inserts it for certain
- * divide-by-zero cases.
- */
- if (insn->type == INSN_BUG)
- insn->dead_end = true;
-
- hash_add(file->insn_hash, &insn->hash, sec_offset_hash(sec, insn->offset));
- list_add_tail(&insn->list, &file->insn_list);
- nr_insns++;
- }
-
- list_for_each_entry(func, &sec->symbol_list, list) {
- if (func->type != STT_FUNC || func->alias != func)
- continue;
-
- if (!find_insn(file, sec, func->offset)) {
- WARN("%s(): can't find starting instruction",
- func->name);
- return -1;
- }
-
- sym_for_each_insn(file, func, insn) {
- insn->func = func;
- if (insn->type == INSN_ENDBR && list_empty(&insn->call_node)) {
- if (insn->offset == insn->func->offset) {
- list_add_tail(&insn->call_node, &file->endbr_list);
- file->nr_endbr++;
- } else {
- file->nr_endbr_int++;
- }
- }
- }
- }
- }
-
- if (stats)
- printf("nr_insns: %lu\n", nr_insns);
-
- return 0;
-
-err:
- free(insn);
- return ret;
-}
-
/*
* Read the pv_ops[] .data table to find the static initialized values.
*/
diff --git a/tools/objtool/decode.c b/tools/objtool/decode.c
new file mode 100644
index 000000000000..4ed438ccc07f
--- /dev/null
+++ b/tools/objtool/decode.c
@@ -0,0 +1,106 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2015-2017 Josh Poimboeuf <[email protected]>
+ */
+#include <linux/objtool.h>
+
+#include <objtool/builtin.h>
+#include <objtool/insn.h>
+#include <objtool/warn.h>
+
+static unsigned long nr_insns;
+
+/*
+ * Call the arch-specific instruction decoder for all the instructions and add
+ * them to the global instruction list.
+ */
+int decode_instructions(struct objtool_file *file)
+{
+ struct section *sec;
+ struct symbol *func;
+ unsigned long offset;
+ struct instruction *insn;
+ int ret;
+
+ for_each_sec(file, sec) {
+
+ if (!(sec->sh.sh_flags & SHF_EXECINSTR))
+ continue;
+
+ if (strcmp(sec->name, ".altinstr_replacement") &&
+ strcmp(sec->name, ".altinstr_aux") &&
+ strncmp(sec->name, ".discard.", 9))
+ sec->text = true;
+
+ if (!strcmp(sec->name, ".noinstr.text") ||
+ !strcmp(sec->name, ".entry.text"))
+ sec->noinstr = true;
+
+ for (offset = 0; offset < sec->sh.sh_size; offset += insn->len) {
+ insn = malloc(sizeof(*insn));
+ if (!insn) {
+ WARN("malloc failed");
+ return -1;
+ }
+ memset(insn, 0, sizeof(*insn));
+ INIT_LIST_HEAD(&insn->alts);
+ INIT_LIST_HEAD(&insn->stack_ops);
+ INIT_LIST_HEAD(&insn->call_node);
+
+ insn->sec = sec;
+ insn->offset = offset;
+
+ ret = arch_decode_instruction(file, sec, offset,
+ sec->sh.sh_size - offset,
+ &insn->len, &insn->type,
+ &insn->immediate,
+ &insn->stack_ops);
+ if (ret)
+ goto err;
+
+ /*
+ * By default, "ud2" is a dead end unless otherwise
+ * annotated, because GCC 7 inserts it for certain
+ * divide-by-zero cases.
+ */
+ if (insn->type == INSN_BUG)
+ insn->dead_end = true;
+
+ hash_add(file->insn_hash, &insn->hash, sec_offset_hash(sec, insn->offset));
+ list_add_tail(&insn->list, &file->insn_list);
+ nr_insns++;
+ }
+
+ list_for_each_entry(func, &sec->symbol_list, list) {
+ if (func->type != STT_FUNC || func->alias != func)
+ continue;
+
+ if (!find_insn(file, sec, func->offset)) {
+ WARN("%s(): can't find starting instruction",
+ func->name);
+ return -1;
+ }
+
+ sym_for_each_insn(file, func, insn) {
+ insn->func = func;
+ if (insn->type == INSN_ENDBR && list_empty(&insn->call_node)) {
+ if (insn->offset == insn->func->offset) {
+ list_add_tail(&insn->call_node, &file->endbr_list);
+ file->nr_endbr++;
+ } else {
+ file->nr_endbr_int++;
+ }
+ }
+ }
+ }
+ }
+
+ if (stats)
+ printf("nr_insns: %lu\n", nr_insns);
+
+ return 0;
+
+err:
+ free(insn);
+ return ret;
+}
diff --git a/tools/objtool/include/objtool/insn.h b/tools/objtool/include/objtool/insn.h
index 1b9fce586679..5f01fd0ce8ed 100644
--- a/tools/objtool/include/objtool/insn.h
+++ b/tools/objtool/include/objtool/insn.h
@@ -83,6 +83,7 @@ struct reloc *insn_reloc(struct objtool_file *file, struct instruction *insn);
bool insn_cfi_match(struct instruction *insn, struct cfi_state *cfi2);
bool same_function(struct instruction *insn1, struct instruction *insn2);
bool is_first_func_insn(struct instruction *insn);
+int decode_instructions(struct objtool_file *file);
#define for_each_insn(file, insn) \
list_for_each_entry(insn, &file->insn_list, list)
--
2.25.1
From: "Madhavan T. Venkataraman" <[email protected]>
Implement a built-in command called cmd_fpv() that can be invoked as
follows:
objtool fpv generate file.o
The built-in command invokes decode_instructions() to walk each function
and decode the instructions of the function.
Signed-off-by: Madhavan T. Venkataraman <[email protected]>
---
tools/objtool/Build | 5 ++
tools/objtool/Makefile | 6 ++-
tools/objtool/builtin-fpv.c | 71 +++++++++++++++++++++++++
tools/objtool/fpv.c | 19 +++++++
tools/objtool/include/objtool/builtin.h | 1 +
tools/objtool/include/objtool/objtool.h | 1 +
tools/objtool/objtool.c | 12 ++++-
tools/objtool/weak.c | 5 ++
8 files changed, 117 insertions(+), 3 deletions(-)
create mode 100644 tools/objtool/builtin-fpv.c
create mode 100644 tools/objtool/fpv.c
diff --git a/tools/objtool/Build b/tools/objtool/Build
index 9c2a332f61f3..a491f51c40b4 100644
--- a/tools/objtool/Build
+++ b/tools/objtool/Build
@@ -15,9 +15,14 @@ objtool-$(SUBCMD_ORC) += decode.o
objtool-$(SUBCMD_ORC) += unwind_hints.o
objtool-$(SUBCMD_ORC) += orc_gen.o
objtool-$(SUBCMD_ORC) += orc_dump.o
+objtool-$(SUBCMD_FPV) += fpv.o
+objtool-$(SUBCMD_FPV) += cfi.o
+objtool-$(SUBCMD_FPV) += insn.o
+objtool-$(SUBCMD_FPV) += decode.o
objtool-y += builtin-check.o
objtool-y += builtin-orc.o
+objtool-y += builtin-fpv.o
objtool-y += elf.o
objtool-y += objtool.o
diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
index 0dbd397f319d..2511053245bc 100644
--- a/tools/objtool/Makefile
+++ b/tools/objtool/Makefile
@@ -47,7 +47,11 @@ ifeq ($(SRCARCH),x86)
SUBCMD_ORC := y
endif
-export SUBCMD_CHECK SUBCMD_ORC
+ifeq ($(SRCARCH),arm64)
+ SUBCMD_FPV := y
+endif
+
+export SUBCMD_CHECK SUBCMD_ORC SUBCMD_FPV
export srctree OUTPUT CFLAGS SRCARCH AWK
include $(srctree)/tools/build/Makefile.include
diff --git a/tools/objtool/builtin-fpv.c b/tools/objtool/builtin-fpv.c
new file mode 100644
index 000000000000..ff57dde39587
--- /dev/null
+++ b/tools/objtool/builtin-fpv.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Author: Madhavan T. Venkataraman ([email protected])
+ *
+ * Copyright (C) 2022 Microsoft Corporation
+ */
+
+/*
+ * objtool fp validation:
+ *
+ * This command analyzes a .o file and adds .orc_unwind and .orc_unwind_ip
+ * sections to it. The sections are used by the frame pointer-based in-kernel
+ * unwinder to validate the frame pointer.
+ */
+
+#include <string.h>
+#include <objtool/builtin.h>
+#include <objtool/objtool.h>
+
+static const char * const fpv_usage[] = {
+ "objtool fpv generate file.o",
+ NULL,
+};
+
+const struct option fpv_options[] = {
+ OPT_END(),
+};
+
+int cmd_fpv(int argc, const char **argv)
+{
+ const char *objname;
+ struct objtool_file *file;
+ int ret;
+
+ argc--; argv++;
+ if (argc <= 0)
+ usage_with_options(fpv_usage, fpv_options);
+
+ objname = argv[1];
+
+ file = objtool_open_read(objname);
+ if (!file)
+ return 1;
+
+ /* Supported architectures. */
+ switch (file->elf->ehdr.e_machine) {
+ case EM_AARCH64:
+ break;
+
+ default:
+ return 1;
+ }
+
+ if (!strncmp(argv[0], "gen", 3)) {
+ ret = fpv_decode(file);
+ if (ret)
+ return ret;
+
+ if (list_empty(&file->insn_list))
+ return 0;
+
+ if (!file->elf->changed)
+ return 0;
+
+ return elf_write(file->elf);
+ }
+
+ usage_with_options(fpv_usage, fpv_options);
+
+ return 0;
+}
diff --git a/tools/objtool/fpv.c b/tools/objtool/fpv.c
new file mode 100644
index 000000000000..76f0f2e611a8
--- /dev/null
+++ b/tools/objtool/fpv.c
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Author: Madhavan T. Venkataraman ([email protected])
+ *
+ * Copyright (C) 2022 Microsoft Corporation
+ */
+
+#include <string.h>
+#include <stdlib.h>
+#include <sys/mman.h>
+
+#include <objtool/builtin.h>
+#include <objtool/insn.h>
+#include <objtool/warn.h>
+
+int fpv_decode(struct objtool_file *file)
+{
+ return decode_instructions(file);
+}
diff --git a/tools/objtool/include/objtool/builtin.h b/tools/objtool/include/objtool/builtin.h
index c39dbfaef6dc..bfd99e08c33b 100644
--- a/tools/objtool/include/objtool/builtin.h
+++ b/tools/objtool/include/objtool/builtin.h
@@ -16,5 +16,6 @@ extern int cmd_parse_options(int argc, const char **argv, const char * const usa
extern int cmd_check(int argc, const char **argv);
extern int cmd_orc(int argc, const char **argv);
+extern int cmd_fpv(int argc, const char **argv);
#endif /* _BUILTIN_H */
diff --git a/tools/objtool/include/objtool/objtool.h b/tools/objtool/include/objtool/objtool.h
index 7a5c13a78f87..e00c8dcc6885 100644
--- a/tools/objtool/include/objtool/objtool.h
+++ b/tools/objtool/include/objtool/objtool.h
@@ -45,5 +45,6 @@ void objtool_pv_add(struct objtool_file *file, int idx, struct symbol *func);
int check(struct objtool_file *file);
int orc_dump(const char *objname);
int orc_create(struct objtool_file *file);
+int fpv_decode(struct objtool_file *file);
#endif /* _OBJTOOL_H */
diff --git a/tools/objtool/objtool.c b/tools/objtool/objtool.c
index b09946f4e1d6..974a9bc384e8 100644
--- a/tools/objtool/objtool.c
+++ b/tools/objtool/objtool.c
@@ -35,9 +35,17 @@ struct cmd_struct {
static const char objtool_usage_string[] =
"objtool COMMAND [ARGS]";
+static const char check_help[] =
+ "Perform stack metadata validation on an object file";
+static const char orc_help[] =
+ "Generate in-place ORC unwind tables for an object file";
+static const char fpv_help[] =
+ "Generate in-place FP validation tables for an object file";
+
static struct cmd_struct objtool_cmds[] = {
- {"check", cmd_check, "Perform stack metadata validation on an object file" },
- {"orc", cmd_orc, "Generate in-place ORC unwind tables for an object file" },
+ {"check", cmd_check, check_help },
+ {"orc", cmd_orc, orc_help },
+ {"fpv", cmd_fpv, fpv_help },
};
bool help;
diff --git a/tools/objtool/weak.c b/tools/objtool/weak.c
index 8314e824db4a..5e56ad5fe0fe 100644
--- a/tools/objtool/weak.c
+++ b/tools/objtool/weak.c
@@ -29,3 +29,8 @@ int __weak orc_create(struct objtool_file *file)
{
UNSUPPORTED("orc");
}
+
+int __weak fpv_decode(struct objtool_file *file)
+{
+ UNSUPPORTED("fpv");
+}
--
2.25.1
From: "Madhavan T. Venkataraman" <[email protected]>
The ORC code needs to be reorganized into arch-specific and generic parts
so that architectures other than X86 can use the generic parts.
orc_types.h contains the following ORC definitions shared between objtool
and the kernel:
- ORC register definitions which are arch-specific.
- orc_entry structure which is generic.
Move orc_entry into a new file include/linux/orc_entry.h. Also, the field
names bp_reg and bp_offset in struct orc_entry are x86-specific. Change
them to fp_reg and fp_offset. FP stands for frame pointer.
Currently, the type field in orc_entry is only 2 bits. For other
architectures, we will need more. So, expand this to 3 bits.
Signed-off-by: Madhavan T. Venkataraman <[email protected]>
---
arch/x86/include/asm/orc_types.h | 37 +++++-------------------
include/linux/orc_entry.h | 39 ++++++++++++++++++++++++++
tools/arch/x86/include/asm/orc_types.h | 37 +++++-------------------
tools/include/linux/orc_entry.h | 39 ++++++++++++++++++++++++++
tools/objtool/orc_gen.c | 4 +--
tools/objtool/sync-check.sh | 1 +
6 files changed, 95 insertions(+), 62 deletions(-)
create mode 100644 include/linux/orc_entry.h
create mode 100644 tools/include/linux/orc_entry.h
diff --git a/arch/x86/include/asm/orc_types.h b/arch/x86/include/asm/orc_types.h
index 5a2baf28a1dc..851c9fb9f695 100644
--- a/arch/x86/include/asm/orc_types.h
+++ b/arch/x86/include/asm/orc_types.h
@@ -8,6 +8,13 @@
#include <linux/types.h>
#include <linux/compiler.h>
+#include <linux/orc_entry.h>
+
+/*
+ * For x86, use the appripriate name for the frame pointer in orc_entry.
+ */
+#define bp_offset fp_offset
+#define bp_reg fp_reg
/*
* The ORC_REG_* registers are base registers which are used to find other
@@ -39,34 +46,4 @@
#define ORC_REG_SP_INDIRECT 9
#define ORC_REG_MAX 15
-#ifndef __ASSEMBLY__
-#include <asm/byteorder.h>
-
-/*
- * This struct is more or less a vastly simplified version of the DWARF Call
- * Frame Information standard. It contains only the necessary parts of DWARF
- * CFI, simplified for ease of access by the in-kernel unwinder. It tells the
- * unwinder how to find the previous SP and BP (and sometimes entry regs) on
- * the stack for a given code address. Each instance of the struct corresponds
- * to one or more code locations.
- */
-struct orc_entry {
- s16 sp_offset;
- s16 bp_offset;
-#if defined(__LITTLE_ENDIAN_BITFIELD)
- unsigned sp_reg:4;
- unsigned bp_reg:4;
- unsigned type:2;
- unsigned end:1;
-#elif defined(__BIG_ENDIAN_BITFIELD)
- unsigned bp_reg:4;
- unsigned sp_reg:4;
- unsigned unused:5;
- unsigned end:1;
- unsigned type:2;
-#endif
-} __packed;
-
-#endif /* __ASSEMBLY__ */
-
#endif /* _ORC_TYPES_H */
diff --git a/include/linux/orc_entry.h b/include/linux/orc_entry.h
new file mode 100644
index 000000000000..3d49e3b9dabe
--- /dev/null
+++ b/include/linux/orc_entry.h
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2017 Josh Poimboeuf <[email protected]>
+ */
+
+#ifndef _ORC_ENTRY_H
+#define _ORC_ENTRY_H
+
+#ifndef __ASSEMBLY__
+#include <asm/byteorder.h>
+
+/*
+ * This struct is more or less a vastly simplified version of the DWARF Call
+ * Frame Information standard. It contains only the necessary parts of DWARF
+ * CFI, simplified for ease of access by the in-kernel unwinder. It tells the
+ * unwinder how to find the previous SP and BP (and sometimes entry regs) on
+ * the stack for a given code address. Each instance of the struct corresponds
+ * to one or more code locations.
+ */
+struct orc_entry {
+ s16 sp_offset;
+ s16 fp_offset;
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+ unsigned sp_reg:4;
+ unsigned fp_reg:4;
+ unsigned type:3;
+ unsigned end:1;
+#elif defined(__BIG_ENDIAN_BITFIELD)
+ unsigned fp_reg:4;
+ unsigned sp_reg:4;
+ unsigned unused:4;
+ unsigned end:1;
+ unsigned type:3;
+#endif
+} __packed;
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* _ORC_ENTRY_H */
diff --git a/tools/arch/x86/include/asm/orc_types.h b/tools/arch/x86/include/asm/orc_types.h
index 5a2baf28a1dc..851c9fb9f695 100644
--- a/tools/arch/x86/include/asm/orc_types.h
+++ b/tools/arch/x86/include/asm/orc_types.h
@@ -8,6 +8,13 @@
#include <linux/types.h>
#include <linux/compiler.h>
+#include <linux/orc_entry.h>
+
+/*
+ * For x86, use the appripriate name for the frame pointer in orc_entry.
+ */
+#define bp_offset fp_offset
+#define bp_reg fp_reg
/*
* The ORC_REG_* registers are base registers which are used to find other
@@ -39,34 +46,4 @@
#define ORC_REG_SP_INDIRECT 9
#define ORC_REG_MAX 15
-#ifndef __ASSEMBLY__
-#include <asm/byteorder.h>
-
-/*
- * This struct is more or less a vastly simplified version of the DWARF Call
- * Frame Information standard. It contains only the necessary parts of DWARF
- * CFI, simplified for ease of access by the in-kernel unwinder. It tells the
- * unwinder how to find the previous SP and BP (and sometimes entry regs) on
- * the stack for a given code address. Each instance of the struct corresponds
- * to one or more code locations.
- */
-struct orc_entry {
- s16 sp_offset;
- s16 bp_offset;
-#if defined(__LITTLE_ENDIAN_BITFIELD)
- unsigned sp_reg:4;
- unsigned bp_reg:4;
- unsigned type:2;
- unsigned end:1;
-#elif defined(__BIG_ENDIAN_BITFIELD)
- unsigned bp_reg:4;
- unsigned sp_reg:4;
- unsigned unused:5;
- unsigned end:1;
- unsigned type:2;
-#endif
-} __packed;
-
-#endif /* __ASSEMBLY__ */
-
#endif /* _ORC_TYPES_H */
diff --git a/tools/include/linux/orc_entry.h b/tools/include/linux/orc_entry.h
new file mode 100644
index 000000000000..3d49e3b9dabe
--- /dev/null
+++ b/tools/include/linux/orc_entry.h
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2017 Josh Poimboeuf <[email protected]>
+ */
+
+#ifndef _ORC_ENTRY_H
+#define _ORC_ENTRY_H
+
+#ifndef __ASSEMBLY__
+#include <asm/byteorder.h>
+
+/*
+ * This struct is more or less a vastly simplified version of the DWARF Call
+ * Frame Information standard. It contains only the necessary parts of DWARF
+ * CFI, simplified for ease of access by the in-kernel unwinder. It tells the
+ * unwinder how to find the previous SP and BP (and sometimes entry regs) on
+ * the stack for a given code address. Each instance of the struct corresponds
+ * to one or more code locations.
+ */
+struct orc_entry {
+ s16 sp_offset;
+ s16 fp_offset;
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+ unsigned sp_reg:4;
+ unsigned fp_reg:4;
+ unsigned type:3;
+ unsigned end:1;
+#elif defined(__BIG_ENDIAN_BITFIELD)
+ unsigned fp_reg:4;
+ unsigned sp_reg:4;
+ unsigned unused:4;
+ unsigned end:1;
+ unsigned type:3;
+#endif
+} __packed;
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* _ORC_ENTRY_H */
diff --git a/tools/objtool/orc_gen.c b/tools/objtool/orc_gen.c
index dd3c64af9db2..68c317daadbf 100644
--- a/tools/objtool/orc_gen.c
+++ b/tools/objtool/orc_gen.c
@@ -98,7 +98,7 @@ static int write_orc_entry(struct elf *elf, struct section *orc_sec,
orc = (struct orc_entry *)orc_sec->data->d_buf + idx;
memcpy(orc, o, sizeof(*orc));
orc->sp_offset = bswap_if_needed(orc->sp_offset);
- orc->bp_offset = bswap_if_needed(orc->bp_offset);
+ orc->fp_offset = bswap_if_needed(orc->fp_offset);
/* populate reloc for ip */
if (elf_add_reloc_to_insn(elf, ip_sec, idx * sizeof(int), R_X86_64_PC32,
@@ -149,7 +149,7 @@ int orc_create(struct objtool_file *file)
struct orc_entry null = {
.sp_reg = ORC_REG_UNDEFINED,
- .bp_reg = ORC_REG_UNDEFINED,
+ .fp_reg = ORC_REG_UNDEFINED,
.type = UNWIND_HINT_TYPE_CALL,
};
diff --git a/tools/objtool/sync-check.sh b/tools/objtool/sync-check.sh
index ee49b4e9e72c..ef1acb064605 100755
--- a/tools/objtool/sync-check.sh
+++ b/tools/objtool/sync-check.sh
@@ -18,6 +18,7 @@ arch/x86/include/asm/unwind_hints.h
arch/x86/lib/x86-opcode-map.txt
arch/x86/tools/gen-insn-attr-x86.awk
include/linux/static_call_types.h
+include/linux/orc_entry.h
"
SYNC_CHECK_FILES='
--
2.25.1
On Mon, May 23, 2022 at 07:16:26PM -0500, [email protected] wrote:
> From: "Madhavan T. Venkataraman" <[email protected]>
>
> Implement a built-in command called cmd_fpv() that can be invoked as
> follows:
>
> objtool fpv generate file.o
>
> The built-in command invokes decode_instructions() to walk each function
> and decode the instructions of the function.
In commit b51277eb9775ce91 ("objtool: Ditch subcommands") Josh removed
subcommands so this interface is going to need a rethink.
See my responses inline...
On 5/24/22 09:27, Chen Zhongjin wrote:
>
>
> On 2022/5/24 8:16, [email protected] wrote:
>> From: "Madhavan T. Venkataraman" <[email protected]>
>>
>> The ORC code needs to be reorganized into arch-specific and generic parts
>> so that architectures other than X86 can use the generic parts.
>>
>> orc_types.h contains the following ORC definitions shared between objtool
>> and the kernel:
>>
>> - ORC register definitions which are arch-specific.
>> - orc_entry structure which is generic.
> ...
>> diff --git a/include/linux/orc_entry.h b/include/linux/orc_entry.h
>> new file mode 100644
>> index 000000000000..3d49e3b9dabe
>> --- /dev/null
>> +++ b/include/linux/orc_entry.h
>> @@ -0,0 +1,39 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2017 Josh Poimboeuf <[email protected]>
>> + */
>> +
>> +#ifndef _ORC_ENTRY_H
>> +#define _ORC_ENTRY_H
>> +
>> +#ifndef __ASSEMBLY__
>> +#include <asm/byteorder.h>
>> +
>> +/*
>> + * This struct is more or less a vastly simplified version of the DWARF Call
>> + * Frame Information standard. It contains only the necessary parts of DWARF
>> + * CFI, simplified for ease of access by the in-kernel unwinder. It tells the
>> + * unwinder how to find the previous SP and BP (and sometimes entry regs) on
>> + * the stack for a given code address. Each instance of the struct corresponds
>> + * to one or more code locations.
>> + */
>> +struct orc_entry {
>> + s16 sp_offset;
>> + s16 fp_offset;
>> +#if defined(__LITTLE_ENDIAN_BITFIELD)
>> + unsigned sp_reg:4;
>> + unsigned fp_reg:4;
> Are sp_reg & fp_reg & end needed? I noticed that they are not used in
> reliability checking.
Yeah. The ORC entry structure is a superset of what I need for ARM64. But X86 requires all of it.
(Although I think "end" is not needed anymore).
And, other architectures might require all of it. So, I am keeping it as is.
>
>> + unsigned type:3;
>> + unsigned end:1;
>> +#elif defined(__BIG_ENDIAN_BITFIELD)
>> + unsigned fp_reg:4;
>> + unsigned sp_reg:4;
>> + unsigned unused:4;
>> + unsigned end:1;
>> + unsigned type:3;
>> +#endif
>> +} __packed;
>> +
>> +#endif /* __ASSEMBLY__ */
>> +
>> +#endif /* _ORC_ENTRY_H */
Thanks!
Madhavan
Thanks for taking the time to review my patches.
On 5/24/22 09:24, Chen Zhongjin wrote:
> Hi Madvenka,
>
> I have a brief look at your patch and the idea that using CFA metadata to
> validate FP is reasonable to me. And I found a problem when I used 'pv dump' to
> check the orc value and I replied your commit 11/20 for that.
>
I have responded to that comment in another email. Please take a look.
> I think it's not necessary that you rewrite the arm64 decoder(there is already a
> decoder in my patch) and insn check(objtool check can just make it) by yourself.
>
This is a fair point. I will think about this a little bit and respond to this in a separate email.
> For me it's also a trouble that objtool runs too much unnecessary work. I advise
> that we should move some check for x86 as arch specific and refactor the cmdline
> options, they doesn't turn off everything perfectly now.
>
So, Josh has done what you have mentioned. He has reorganized all of that code.
I am working on syncing up to his changes. I will send out version 3.
> Other than that I have an advise: We only use orc for reliable stacktrace and
> normal FP unwind doesn't depends on it. Should we only load these data for
> livepatch (or other scenario needs reliable stacktrace)? It can save the memory
> and time consuming for kernel.
>
Yes. For ARM64, that is what I am trying to do. STACK_VALIDATION is optional and it
is off by default. It needs to be turned on only if reliable stack trace is required.
> That's all. And if you don't mind, can I incorporate some commit into my set?
> Appreciate for it.
>
Please feel free to use any and all of my code. I am also looking at merging our two
decoders so that there is only one decoder. I need to think about this a little bit.
So, stay tuned.
Thanks!
Madhavan
On 5/24/22 09:09, Mark Brown wrote:
> On Mon, May 23, 2022 at 07:16:26PM -0500, [email protected] wrote:
>> From: "Madhavan T. Venkataraman" <[email protected]>
>>
>> Implement a built-in command called cmd_fpv() that can be invoked as
>> follows:
>>
>> objtool fpv generate file.o
>>
>> The built-in command invokes decode_instructions() to walk each function
>> and decode the instructions of the function.
>
> In commit b51277eb9775ce91 ("objtool: Ditch subcommands") Josh removed
> subcommands so this interface is going to need a rethink.
Thanks for mentioning this. I will sync my patchset to the latest and send out version 3.
Thanks!
Madhavan
On Sun, May 29, 2022 at 09:49:44AM -0500, Madhavan T. Venkataraman wrote:
>
>
> On 5/24/22 09:09, Mark Brown wrote:
> > On Mon, May 23, 2022 at 07:16:26PM -0500, [email protected] wrote:
> >> From: "Madhavan T. Venkataraman" <[email protected]>
> >>
> >> Implement a built-in command called cmd_fpv() that can be invoked as
> >> follows:
> >>
> >> objtool fpv generate file.o
> >>
> >> The built-in command invokes decode_instructions() to walk each function
> >> and decode the instructions of the function.
> >
> > In commit b51277eb9775ce91 ("objtool: Ditch subcommands") Josh removed
> > subcommands so this interface is going to need a rethink.
>
> Thanks for mentioning this. I will sync my patchset to the latest and send out version 3.
Before you do; why are you duplicating lots of validate_branch() ? Why
can't you use the regular code to generate ORC data?
I'm really not happy about all this.
On 5/30/22 02:51, Peter Zijlstra wrote:
> On Sun, May 29, 2022 at 09:49:44AM -0500, Madhavan T. Venkataraman wrote:
>>
>>
>> On 5/24/22 09:09, Mark Brown wrote:
>>> On Mon, May 23, 2022 at 07:16:26PM -0500, [email protected] wrote:
>>>> From: "Madhavan T. Venkataraman" <[email protected]>
>>>>
>>>> Implement a built-in command called cmd_fpv() that can be invoked as
>>>> follows:
>>>>
>>>> objtool fpv generate file.o
>>>>
>>>> The built-in command invokes decode_instructions() to walk each function
>>>> and decode the instructions of the function.
>>>
>>> In commit b51277eb9775ce91 ("objtool: Ditch subcommands") Josh removed
>>> subcommands so this interface is going to need a rethink.
>>
>> Thanks for mentioning this. I will sync my patchset to the latest and send out version 3.
>
> Before you do; why are you duplicating lots of validate_branch() ? Why
> can't you use the regular code to generate ORC data?
>
> I'm really not happy about all this.
Hi Peter,
I am preparing a detailed response to this explaining why I have not used validate_branch().
The short answer is that no validation is required for my approach. But I will send my detailed
response shortly.
Thanks.
Madhavan
On 6/1/22 17:45, Madhavan T. Venkataraman wrote:
>
>
> On 5/30/22 02:51, Peter Zijlstra wrote:
>> On Sun, May 29, 2022 at 09:49:44AM -0500, Madhavan T. Venkataraman wrote:
>>>
>>>
>>> On 5/24/22 09:09, Mark Brown wrote:
>>>> On Mon, May 23, 2022 at 07:16:26PM -0500, [email protected] wrote:
>>>>> From: "Madhavan T. Venkataraman" <[email protected]>
>>>>>
>>>>> Implement a built-in command called cmd_fpv() that can be invoked as
>>>>> follows:
>>>>>
>>>>> objtool fpv generate file.o
>>>>>
>>>>> The built-in command invokes decode_instructions() to walk each function
>>>>> and decode the instructions of the function.
>>>>
>>>> In commit b51277eb9775ce91 ("objtool: Ditch subcommands") Josh removed
>>>> subcommands so this interface is going to need a rethink.
>>>
>>> Thanks for mentioning this. I will sync my patchset to the latest and send out version 3.
>>
>> Before you do; why are you duplicating lots of validate_branch() ? Why
>> can't you use the regular code to generate ORC data?
>>
>> I'm really not happy about all this.
>
> Hi Peter,
>
> I am preparing a detailed response to this explaining why I have not used validate_branch().
> The short answer is that no validation is required for my approach. But I will send my detailed
> response shortly.
>
> Thanks.
>
> Madhavan
Sorry for the delay in responding to your comment. I had to make changes
to address it and complete testing.
I will address your comment in version 3. There are two parts to your comment:
- Why is validate_branch() not being used in Dynamic FP Validation?
I will use it in version 3. See below.
- Why is the current code not being used to compute ORC?
There are some differences in the CFI code between X86 and ARM64.
So, I have defined handle_insn_ops() separately for the two in v3.
What I am doing in v3
=====================
I have discarded my own function (walk_code()) to walk instructions. Instead,
I have used validate_branch() per your comment.
However, my approach requires no validation. More on this below.
So, what I have done is to move all of the validation checks and actions into
their own functions. I define the functions separately for Static Stack
Validation (in check.c) and Dynamic Frame Pointer Validation (in fpv.c). The
two files are mutually exclusive.
The functions in fpv.c contain their own checks and actions.
validate_insn_initial() Initial checks.
validate_insn_cfi() CFI-related checks.
validate_insn_alt() Alternate instructions checks.
validate_insn_return() Return instruction checks.
validate_insn_call() Call/Call dynamic instruction checks.
validate_insn_jump() Jump instruction checks.
validate_insn_jump_dynamic() Jump Dynamic instruction checks.
validate_insn_rest() Checks for miscellaneous instructions.
validate_ibt_insn() IBT instruction checks.
handle_insn_ops() Update CFI from stack ops generated by
the decoder.
validate_branch() looks like this now:
int validate_branch(struct objtool_file *file, struct symbol *func,
struct instruction *insn, struct insn_state state)
{
struct instruction *next_insn, *prev_insn = NULL;
struct section *sec;
u8 visited;
int ret;
sec = insn->sec;
while (1) {
next_insn = next_insn_to_validate(file, insn);
if (validate_insn_initial(file, func, insn, &ret))
return ret;
if (validate_insn_cfi(prev_insn, insn, &state, &visited, &ret))
return ret;
if (state.noinstr)
state.instr += insn->instr;
if (validate_insn_alt(file, func, insn, &state, &ret))
return ret;
if (handle_insn_ops(insn, next_insn, &state))
return 1;
switch (insn->type) {
case INSN_RETURN:
validate_insn_return(func, insn,
next_insn, &state, &ret);
return ret;
case INSN_CALL:
case INSN_CALL_DYNAMIC:
if (validate_insn_call(file, func, insn, &state, &ret))
return ret;
break;
case INSN_JUMP_CONDITIONAL:
case INSN_JUMP_UNCONDITIONAL:
if (validate_insn_jump(file, func, insn, &state, &ret))
return ret;
break;
case INSN_JUMP_DYNAMIC:
case INSN_JUMP_DYNAMIC_CONDITIONAL:
if (validate_insn_jump_dynamic(file, insn, next_insn,
&state, &ret)) {
return ret;
}
break;
default:
if (validate_insn_rest(func, insn, next_insn,
&state, &ret)) {
return ret;
}
break;
}
if (ibt)
validate_ibt_insn(file, insn);
if (insn->dead_end)
return 0;
if (!next_insn) {
if (state.cfi.cfa.base == CFI_UNDEFINED)
return 0;
WARN("%s: unexpected end of section", sec->name);
return 1;
}
prev_insn = insn;
insn = next_insn;
}
return 0;
}
Why no validation?
==================
There are two approaches for reliable stacktrace.
1. Static Stack Validation (the current approach).
Analyze the code statically and perform checks for ABI compliance and valid
stack operations. If any warnings or errors are encountered, "fix" the
kernel and/or the toolchain so the generated code conforms to Objtool's
expectations.
2. Dynamic Frame Pointer Validation.
Don't perform any validation of kernel code. Simply compute the SP and FP
offsets at each instruction address based on the actual code. During unwind,
compute a frame pointer from the offsets at each frame and validate the
actual frame pointer with it. If an FP cannot be computed or the computed
FP does not match the actual FP, consider the frame unreliable for unwind.
Since the unwinder can clearly tell whether a frame is reliable or not,
reliable stacktrace can be provided.
I am doing (2) in my patch series.
Different cases
===============
C Functions
-----------
I find that the compiler generates proper FP prolog and epilog for C functions.
The only exceptions I found are functions that have multiple code paths sharing
some instructions with differing CFIs. See CFI mismatch below. This mismatch
happens only for a very small percentage of the functions.
Buggy code generated by compiler
--------------------------------
Even assuming that the compiler can sometimes generate code that does
not follow ABI rules, it is still not a problem as the unwinder can
do an FP match and tell whether some code is reliable for unwind or not.
Assembly Functions
------------------
There are two cases:
SYM_CODE functions
------------------
Functions defined using the SYM_CODE_*() macros.
AFAICT, Objtool does not process these. These are low-level functions
that don't follow any ABI rules. The ORC entries for these would be
undefined. So, the unwinder will rightly consider them unreliable
for unwind.
SYM_FUNC functions
------------------
Functions defined using the SYM_FUNC_*() macros.
These are supposed to have proper FP prologs and epilogs.
At the moment, they don't for ARM64. The unwinder will consider these
unreliable for unwind at the moment.
That said, I am working on a separate patch series to add the prologs
and epilogs to these functions (except in cases where functionality
or performance would be affected). This is not required to support
reliable stack trace. This is only to reduce potential retries during
the livepatch process.
Functions without a proper FP prolog/epilog
-------------------------------------------
For leaf functions, the compiler may not generate FP prologs/epilogs for
performance reasons. In Dynamic FP Validation, the unwinder will recognize
these to be unreliable for unwind.
Assembly functions that don't have a proper FP prolog/epilog are treated like
leaf functions.
CFI mismatch
------------
This is based on actual code on ARM64.
Let us say there are two code paths in a function. The two code paths share
some instructions. If the SP and FP offsets are different in the two code paths,
the shared instructions will have a CFI mismatch. But this is not invalid or
buggy code. It is just a case that Objtool cannot handle because only one CFI
is associated with an instruction in Objtool.
In my approach, one CFI will be recorded. The other will be ignored. The
computed FP will match the actual FP in one code path. It will not match
in the other one. The unwinder will consider the former reliable and the
latter unreliable.
This happens only for a very small number of functions in the entire kernel.
That said, I am investigating the possibility of storing both in ORC entries
in a manner similar to alternate instructions. If this is feasible, then the
unwinder can do an FP match using any of these entries.
Thanks!
Madhavan
Hi Madhavan,
And thank you for your work on this!
> My github repo for this version is here:
>
> https://github.com/madvenka786/linux/tree/orc_v2
>
> Please feel free to clone and check it out. And, please let me know if you
> find any issues.
>
<snip>
>
> TBD
> ===
>
> - I need to perform more rigorous testing with different scenarios. This
> is work in progress. Any ideas or suggestions are welcome.
>
I have run following [1] livepatch tests on kernel build from your repository.
Overall results looks good, but when I run klp_tc_13.shI there is something which
I still can not understand completely. It is because of kaslr.
[ 36.817617] livepatch: enabling patch 'klp_tc_13_livepatch'
[ 36.819602] branch_imm_common: offset out of range
[ 36.820113] branch_imm_common: offset out of range
[ 36.820643] ------------[ ftrace bug ]------------
[ 36.821172] ftrace failed to modify
[ 36.821173] [<ffffdde931176804>] orig_do_read_active_livepatch_id+0x4/0xa8 [klp_test_support_mod]
[ 36.822465] actual: e4:01:00:94
[ 36.822821] Updating ftrace call site to call a different ftrace function
[ 36.823537] ftrace record flags: e4000002
[ 36.823953] (2) R
[ 36.823953] expected tramp: ffffdde96882e224
[ 36.824619] ------------[ cut here ]------------
[ 36.825125] WARNING: CPU: 0 PID: 950 at kernel/trace/ftrace.c:2085 ftrace_bug+0x98/0x280
[ 36.826027] Modules linked in: klp_tc_13_livepatch(OK+) klp_test_support_mod(O) crct10dif_ce
[ 36.826943] CPU: 0 PID: 950 Comm: insmod Tainted: G O K 5.18.0-rc1-00020-g1ffee6fdcfda #39
[ 36.827987] Hardware name: linux,dummy-virt (DT)
[ 36.828546] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 36.829348] pc : ftrace_bug+0x98/0x280
[ 36.829790] lr : ftrace_bug+0x228/0x280
[ 36.830224] sp : ffff8000084038e0
[ 36.830620] x29: ffff8000084038e0 x28: ffff00000485a920 x27: ffffdde931176804
[ 36.831419] x26: ffffdde93117d1a0 x25: ffff00000485a900 x24: ffffdde96aea1000
[ 36.832226] x23: 0000000000000000 x22: 0000000000000001 x21: ffffdde96a124da0
[ 36.833024] x20: ffff0000045620f0 x19: ffffdde96b54a358 x18: ffffffffffffffff
[ 36.833818] x17: 5b20386178302f34 x16: 78302b64695f6863 x15: ffffdde96a3078f8
[ 36.834621] x14: 0000000000000000 x13: 3432326532383836 x12: ffffdde96ae9b3d8
[ 36.835425] x11: 0000000000000001 x10: 0000000000000001 x9 : ffffdde96891305c
[ 36.836221] x8 : c0000000ffffefff x7 : 0000000000017fe8 x6 : ffffdde96ae83398
[ 36.837023] x5 : 0000000000057fa8 x4 : 0000000000000000 x3 : 00000000ffffefff
[ 36.837836] x2 : e32439832ffb9700 x1 : 0000000000000000 x0 : 0000000000000022
[ 36.838635] Call trace:
[ 36.838923] ftrace_bug+0x98/0x280
[ 36.839319] ftrace_replace_code+0xa0/0xb8
[ 36.839768] ftrace_modify_all_code+0xc0/0x160
[ 36.840273] arch_ftrace_update_code+0x14/0x20
[ 36.840780] ftrace_run_update_code+0x24/0x78
[ 36.841283] ftrace_startup_enable+0x50/0x60
[ 36.841781] ftrace_startup+0xb4/0x178
[ 36.842214] register_ftrace_function+0x68/0x88
[ 36.842738] klp_patch_object+0x1c8/0x330
[ 36.843196] klp_enable_patch+0x468/0x828
Regards,
Ivan
[1] https://github.com/lpechacek/qa_test_klp
For this to work on aarch64 you will need fixes from PR#13
On Wed, Jun 15, 2022 at 03:18:44PM +0300, Ivan T. Ivanov wrote:
> I have run following [1] livepatch tests on kernel build from your repository.
> Overall results looks good, but when I run klp_tc_13.shI there is something which
> I still can not understand completely. It is because of kaslr.
>
> [ 36.817617] livepatch: enabling patch 'klp_tc_13_livepatch'
> [ 36.819602] branch_imm_common: offset out of range
> [ 36.820113] branch_imm_common: offset out of range
> [ 36.820643] ------------[ ftrace bug ]------------
> [ 36.821172] ftrace failed to modify
> [ 36.821173] [<ffffdde931176804>] orig_do_read_active_livepatch_id+0x4/0xa8 [klp_test_support_mod]
> [ 36.822465] actual: e4:01:00:94
> [ 36.822821] Updating ftrace call site to call a different ftrace function
> [ 36.823537] ftrace record flags: e4000002
> [ 36.823953] (2) R
> [ 36.823953] expected tramp: ffffdde96882e224
> [ 36.824619] ------------[ cut here ]------------
> [ 36.825125] WARNING: CPU: 0 PID: 950 at kernel/trace/ftrace.c:2085 ftrace_bug+0x98/0x280
> [ 36.826027] Modules linked in: klp_tc_13_livepatch(OK+) klp_test_support_mod(O) crct10dif_ce
> [ 36.826943] CPU: 0 PID: 950 Comm: insmod Tainted: G O K 5.18.0-rc1-00020-g1ffee6fdcfda #39
> [ 36.827987] Hardware name: linux,dummy-virt (DT)
> [ 36.828546] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 36.829348] pc : ftrace_bug+0x98/0x280
> [ 36.829790] lr : ftrace_bug+0x228/0x280
> [ 36.830224] sp : ffff8000084038e0
> [ 36.830620] x29: ffff8000084038e0 x28: ffff00000485a920 x27: ffffdde931176804
> [ 36.831419] x26: ffffdde93117d1a0 x25: ffff00000485a900 x24: ffffdde96aea1000
> [ 36.832226] x23: 0000000000000000 x22: 0000000000000001 x21: ffffdde96a124da0
> [ 36.833024] x20: ffff0000045620f0 x19: ffffdde96b54a358 x18: ffffffffffffffff
> [ 36.833818] x17: 5b20386178302f34 x16: 78302b64695f6863 x15: ffffdde96a3078f8
> [ 36.834621] x14: 0000000000000000 x13: 3432326532383836 x12: ffffdde96ae9b3d8
> [ 36.835425] x11: 0000000000000001 x10: 0000000000000001 x9 : ffffdde96891305c
> [ 36.836221] x8 : c0000000ffffefff x7 : 0000000000017fe8 x6 : ffffdde96ae83398
> [ 36.837023] x5 : 0000000000057fa8 x4 : 0000000000000000 x3 : 00000000ffffefff
> [ 36.837836] x2 : e32439832ffb9700 x1 : 0000000000000000 x0 : 0000000000000022
> [ 36.838635] Call trace:
> [ 36.838923] ftrace_bug+0x98/0x280
> [ 36.839319] ftrace_replace_code+0xa0/0xb8
> [ 36.839768] ftrace_modify_all_code+0xc0/0x160
> [ 36.840273] arch_ftrace_update_code+0x14/0x20
> [ 36.840780] ftrace_run_update_code+0x24/0x78
> [ 36.841283] ftrace_startup_enable+0x50/0x60
> [ 36.841781] ftrace_startup+0xb4/0x178
> [ 36.842214] register_ftrace_function+0x68/0x88
> [ 36.842738] klp_patch_object+0x1c8/0x330
> [ 36.843196] klp_enable_patch+0x468/0x828
IIUC that splat specifically is due to ftrace_modify_call() missing module PLT lookups.
That should be fixed by:
https://lore.kernel.org/all/[email protected]/
I have not looked at the rest of this series (yet).
Thanks,
Mark.
Hi,
On 06-15 14:37, Mark Rutland wrote:
>
> On Wed, Jun 15, 2022 at 03:18:44PM +0300, Ivan T. Ivanov wrote:
> > I have run following [1] livepatch tests on kernel build from your repository.
> > Overall results looks good, but when I run klp_tc_13.shI there is something which
> > I still can not understand completely. It is because of kaslr.
> >
> > [ 36.817617] livepatch: enabling patch 'klp_tc_13_livepatch'
> > [ 36.819602] branch_imm_common: offset out of range
> > [ 36.820113] branch_imm_common: offset out of range
> > [ 36.820643] ------------[ ftrace bug ]------------
> > [ 36.821172] ftrace failed to modify
> > [ 36.821173] [<ffffdde931176804>] orig_do_read_active_livepatch_id+0x4/0xa8 [klp_test_support_mod]
> > [ 36.822465] actual: e4:01:00:94
> > [ 36.822821] Updating ftrace call site to call a different ftrace function
> > [ 36.823537] ftrace record flags: e4000002
> > [ 36.823953] (2) R
> > [ 36.823953] expected tramp: ffffdde96882e224
> > [ 36.824619] ------------[ cut here ]------------
> > [ 36.825125] WARNING: CPU: 0 PID: 950 at kernel/trace/ftrace.c:2085 ftrace_bug+0x98/0x280
> > [ 36.826027] Modules linked in: klp_tc_13_livepatch(OK+) klp_test_support_mod(O) crct10dif_ce
> > [ 36.826943] CPU: 0 PID: 950 Comm: insmod Tainted: G O K 5.18.0-rc1-00020-g1ffee6fdcfda #39
> > [ 36.827987] Hardware name: linux,dummy-virt (DT)
> > [ 36.828546] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > [ 36.829348] pc : ftrace_bug+0x98/0x280
> > [ 36.829790] lr : ftrace_bug+0x228/0x280
> > [ 36.830224] sp : ffff8000084038e0
> > [ 36.830620] x29: ffff8000084038e0 x28: ffff00000485a920 x27: ffffdde931176804
> > [ 36.831419] x26: ffffdde93117d1a0 x25: ffff00000485a900 x24: ffffdde96aea1000
> > [ 36.832226] x23: 0000000000000000 x22: 0000000000000001 x21: ffffdde96a124da0
> > [ 36.833024] x20: ffff0000045620f0 x19: ffffdde96b54a358 x18: ffffffffffffffff
> > [ 36.833818] x17: 5b20386178302f34 x16: 78302b64695f6863 x15: ffffdde96a3078f8
> > [ 36.834621] x14: 0000000000000000 x13: 3432326532383836 x12: ffffdde96ae9b3d8
> > [ 36.835425] x11: 0000000000000001 x10: 0000000000000001 x9 : ffffdde96891305c
> > [ 36.836221] x8 : c0000000ffffefff x7 : 0000000000017fe8 x6 : ffffdde96ae83398
> > [ 36.837023] x5 : 0000000000057fa8 x4 : 0000000000000000 x3 : 00000000ffffefff
> > [ 36.837836] x2 : e32439832ffb9700 x1 : 0000000000000000 x0 : 0000000000000022
> > [ 36.838635] Call trace:
> > [ 36.838923] ftrace_bug+0x98/0x280
> > [ 36.839319] ftrace_replace_code+0xa0/0xb8
> > [ 36.839768] ftrace_modify_all_code+0xc0/0x160
> > [ 36.840273] arch_ftrace_update_code+0x14/0x20
> > [ 36.840780] ftrace_run_update_code+0x24/0x78
> > [ 36.841283] ftrace_startup_enable+0x50/0x60
> > [ 36.841781] ftrace_startup+0xb4/0x178
> > [ 36.842214] register_ftrace_function+0x68/0x88
> > [ 36.842738] klp_patch_object+0x1c8/0x330
> > [ 36.843196] klp_enable_patch+0x468/0x828
>
> IIUC that splat specifically is due to ftrace_modify_call() missing module PLT lookups.
>
> That should be fixed by:
>
> https://lore.kernel.org/all/[email protected]/
>
Yes, this is it. Thanks!
Regards,
Ivan
On 6/15/22 07:18, Ivan T. Ivanov wrote:
> Hi Madhavan,
>
> And thank you for your work on this!
>
>> My github repo for this version is here:
>>
>> https://github.com/madvenka786/linux/tree/orc_v2
>>
>> Please feel free to clone and check it out. And, please let me know if you
>> find any issues.
>>
>
> <snip>
>
>>
>> TBD
>> ===
>>
>> - I need to perform more rigorous testing with different scenarios. This
>> is work in progress. Any ideas or suggestions are welcome.
>>
>
> I have run following [1] livepatch tests on kernel build from your repository.
> Overall results looks good, but when I run klp_tc_13.shI there is something which
> I still can not understand completely. It is because of kaslr.
>
> [ 36.817617] livepatch: enabling patch 'klp_tc_13_livepatch'
> [ 36.819602] branch_imm_common: offset out of range
> [ 36.820113] branch_imm_common: offset out of range
> [ 36.820643] ------------[ ftrace bug ]------------
> [ 36.821172] ftrace failed to modify
> [ 36.821173] [<ffffdde931176804>] orig_do_read_active_livepatch_id+0x4/0xa8 [klp_test_support_mod]
> [ 36.822465] actual: e4:01:00:94
> [ 36.822821] Updating ftrace call site to call a different ftrace function
> [ 36.823537] ftrace record flags: e4000002
> [ 36.823953] (2) R
> [ 36.823953] expected tramp: ffffdde96882e224
> [ 36.824619] ------------[ cut here ]------------
> [ 36.825125] WARNING: CPU: 0 PID: 950 at kernel/trace/ftrace.c:2085 ftrace_bug+0x98/0x280
> [ 36.826027] Modules linked in: klp_tc_13_livepatch(OK+) klp_test_support_mod(O) crct10dif_ce
> [ 36.826943] CPU: 0 PID: 950 Comm: insmod Tainted: G O K 5.18.0-rc1-00020-g1ffee6fdcfda #39
> [ 36.827987] Hardware name: linux,dummy-virt (DT)
> [ 36.828546] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 36.829348] pc : ftrace_bug+0x98/0x280
> [ 36.829790] lr : ftrace_bug+0x228/0x280
> [ 36.830224] sp : ffff8000084038e0
> [ 36.830620] x29: ffff8000084038e0 x28: ffff00000485a920 x27: ffffdde931176804
> [ 36.831419] x26: ffffdde93117d1a0 x25: ffff00000485a900 x24: ffffdde96aea1000
> [ 36.832226] x23: 0000000000000000 x22: 0000000000000001 x21: ffffdde96a124da0
> [ 36.833024] x20: ffff0000045620f0 x19: ffffdde96b54a358 x18: ffffffffffffffff
> [ 36.833818] x17: 5b20386178302f34 x16: 78302b64695f6863 x15: ffffdde96a3078f8
> [ 36.834621] x14: 0000000000000000 x13: 3432326532383836 x12: ffffdde96ae9b3d8
> [ 36.835425] x11: 0000000000000001 x10: 0000000000000001 x9 : ffffdde96891305c
> [ 36.836221] x8 : c0000000ffffefff x7 : 0000000000017fe8 x6 : ffffdde96ae83398
> [ 36.837023] x5 : 0000000000057fa8 x4 : 0000000000000000 x3 : 00000000ffffefff
> [ 36.837836] x2 : e32439832ffb9700 x1 : 0000000000000000 x0 : 0000000000000022
> [ 36.838635] Call trace:
> [ 36.838923] ftrace_bug+0x98/0x280
> [ 36.839319] ftrace_replace_code+0xa0/0xb8
> [ 36.839768] ftrace_modify_all_code+0xc0/0x160
> [ 36.840273] arch_ftrace_update_code+0x14/0x20
> [ 36.840780] ftrace_run_update_code+0x24/0x78
> [ 36.841283] ftrace_startup_enable+0x50/0x60
> [ 36.841781] ftrace_startup+0xb4/0x178
> [ 36.842214] register_ftrace_function+0x68/0x88
> [ 36.842738] klp_patch_object+0x1c8/0x330
> [ 36.843196] klp_enable_patch+0x468/0x828
>
>
> Regards,
> Ivan
>
> [1] https://github.com/lpechacek/qa_test_klp
> For this to work on aarch64 you will need fixes from PR#13
Thank you so much for taking the time to try it out!
I saw Mark's reply to this.
Appreciate it!
Madhavan
On 6/15/22 08:37, Mark Rutland wrote:
> On Wed, Jun 15, 2022 at 03:18:44PM +0300, Ivan T. Ivanov wrote:
>> I have run following [1] livepatch tests on kernel build from your repository.
>> Overall results looks good, but when I run klp_tc_13.shI there is something which
>> I still can not understand completely. It is because of kaslr.
>>
>> [ 36.817617] livepatch: enabling patch 'klp_tc_13_livepatch'
>> [ 36.819602] branch_imm_common: offset out of range
>> [ 36.820113] branch_imm_common: offset out of range
>> [ 36.820643] ------------[ ftrace bug ]------------
>> [ 36.821172] ftrace failed to modify
>> [ 36.821173] [<ffffdde931176804>] orig_do_read_active_livepatch_id+0x4/0xa8 [klp_test_support_mod]
>> [ 36.822465] actual: e4:01:00:94
>> [ 36.822821] Updating ftrace call site to call a different ftrace function
>> [ 36.823537] ftrace record flags: e4000002
>> [ 36.823953] (2) R
>> [ 36.823953] expected tramp: ffffdde96882e224
>> [ 36.824619] ------------[ cut here ]------------
>> [ 36.825125] WARNING: CPU: 0 PID: 950 at kernel/trace/ftrace.c:2085 ftrace_bug+0x98/0x280
>> [ 36.826027] Modules linked in: klp_tc_13_livepatch(OK+) klp_test_support_mod(O) crct10dif_ce
>> [ 36.826943] CPU: 0 PID: 950 Comm: insmod Tainted: G O K 5.18.0-rc1-00020-g1ffee6fdcfda #39
>> [ 36.827987] Hardware name: linux,dummy-virt (DT)
>> [ 36.828546] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>> [ 36.829348] pc : ftrace_bug+0x98/0x280
>> [ 36.829790] lr : ftrace_bug+0x228/0x280
>> [ 36.830224] sp : ffff8000084038e0
>> [ 36.830620] x29: ffff8000084038e0 x28: ffff00000485a920 x27: ffffdde931176804
>> [ 36.831419] x26: ffffdde93117d1a0 x25: ffff00000485a900 x24: ffffdde96aea1000
>> [ 36.832226] x23: 0000000000000000 x22: 0000000000000001 x21: ffffdde96a124da0
>> [ 36.833024] x20: ffff0000045620f0 x19: ffffdde96b54a358 x18: ffffffffffffffff
>> [ 36.833818] x17: 5b20386178302f34 x16: 78302b64695f6863 x15: ffffdde96a3078f8
>> [ 36.834621] x14: 0000000000000000 x13: 3432326532383836 x12: ffffdde96ae9b3d8
>> [ 36.835425] x11: 0000000000000001 x10: 0000000000000001 x9 : ffffdde96891305c
>> [ 36.836221] x8 : c0000000ffffefff x7 : 0000000000017fe8 x6 : ffffdde96ae83398
>> [ 36.837023] x5 : 0000000000057fa8 x4 : 0000000000000000 x3 : 00000000ffffefff
>> [ 36.837836] x2 : e32439832ffb9700 x1 : 0000000000000000 x0 : 0000000000000022
>> [ 36.838635] Call trace:
>> [ 36.838923] ftrace_bug+0x98/0x280
>> [ 36.839319] ftrace_replace_code+0xa0/0xb8
>> [ 36.839768] ftrace_modify_all_code+0xc0/0x160
>> [ 36.840273] arch_ftrace_update_code+0x14/0x20
>> [ 36.840780] ftrace_run_update_code+0x24/0x78
>> [ 36.841283] ftrace_startup_enable+0x50/0x60
>> [ 36.841781] ftrace_startup+0xb4/0x178
>> [ 36.842214] register_ftrace_function+0x68/0x88
>> [ 36.842738] klp_patch_object+0x1c8/0x330
>> [ 36.843196] klp_enable_patch+0x468/0x828
>
> IIUC that splat specifically is due to ftrace_modify_call() missing module PLT lookups.
>
> That should be fixed by:
>
> https://lore.kernel.org/all/[email protected]/
>
> I have not looked at the rest of this series (yet).
>
> Thanks,
> Mark.
Thank you so much, Mark!
I will include this in my patch series until it is upstream.
Madhavan