From: "Madhavan T. Venkataraman" <redacted>
There are a number of places in kernel code where the stack trace is not
reliable. Enhance the unwinder to check for those cases and mark the
stack trace as unreliable. Once all of the checks are in place, the unwinder
can provide a reliable stack trace. But before this can be used for livepatch,
some other entity needs to guarantee that the frame pointers are all set up
correctly in kernel functions. objtool is currently being worked on to
address that need.
Return address check
====================
Check the return PC of every stack frame to make sure that it is a valid
kernel text address (and not some generated code, for example). If it is
not a valid kernel text address, mark the stack trace as unreliable.
Assembly functions
==================
There are a number of assembly functions in arm64. Except for a couple of
them, these functions do not have a frame pointer prolog or epilog. Also,
many of them manipulate low-level state such as registers. These functions
are, by definition, unreliable from a stack unwinding perspective. That is,
when these functions occur in a stack trace, the unwinder would not be able
to unwind through them reliably.
Assembly functions are defined as SYM_FUNC_*() functions or SYM_CODE_*()
functions. objtool peforms static analysis of SYM_FUNC functions. It ignores
SYM_CODE functions because they have low level code that is difficult to
analyze. When objtool becomes ready eventually, SYM_FUNC functions will
be analyzed and "fixed" as necessary. So, they are not "interesting" for
the reliable unwinder.
That leaves SYM_CODE functions. It is for the unwinder to deal with these
for reliable stack trace. The unwinder needs to do the following:
- Recognize SYM_CODE functions in a stack trace.
- If a particular SYM_CODE function can be unwinded through using
some special logic, then do it. E.g., the return trampoline for
Function Graph Tracing.
- Otherwise, mark the stack trace unreliable.
Using text sections to recognize SYM_CODE functions
===================================================
SYM_CODE functions are present in the following text sections:
(1) .entry.text
(2) .idmap.text
(3) .hyp.idmap.text
(4) .hyp.text
(5) .hibernate_exit.text
(6) .entry.tramp.text
(7) .text
(8) .init.text
For each of these sections, there are global variables that contain the
starting and ending addresses generated by the linker. So, they can be
recognized easily. Create an array called sym_code_ranges[] to contain
the ranges for sections (1) thru (6).
Check if the return PC falls in any of these sections. If it does, mark
the stack trace unreliable.
Sections (7) and (8)
====================
Sections (7) and (8) are generic sections which also contain tons of other
functions which are actually reliable. The SYM_CODE functions in these
sections must be dealt with differently.
Some of these are "don't care" functions so they can be left in their
current sections. E.g.,
efi_enter_kernel().
arm64_relocate_new_kernel()
These functions enter the kernel. So, they will not occur
in a stack trace examined by livepatch.
__kernel_rt_sigreturn()
This only gets invoked in user context.
hypervisor vectors
I don't believe these are interesting to livepatch.
Others need to be moved to a special section. I have created a "catch-all"
section called ".code.text" for this purpose. Add this section to
vmlinux.lds.S and sym_code_ranges[].
Reliable SYM_CODE functions
===========================
The unwinder currently has logic to recognize the return trampoline of the
Function Graph Tracer (return_to_handler()), retrieve the original return
address and use that in the stack trace. So, this is a SYM_CODE function
that the unwinder can actually unwind through.
However, the current logic only addreses stack traces taken while still in
the traced function. When the traced function returns, control is transferred
to return_to_handler(). Any stack trace taken while in the return trampoline
is not handled. This messes up the stack trace as the unwinder has to keep
track of the current index within the return address stack.
There are two options:
- Either remove this logic altogether. This would work since the
unwinder would recognize the section of the trampoline and
treat the stack trace as unreliable. So, from a live patch
perspective, this is sufficient.
- Or, fix the logic. I have taken this approach. See the patch
for more details. That said, I am open to option 1.
Other cases like kretprobe_trampoline() and ftrace entry code can be
addressed in a similar fashion. But this is outside the scope of this
work. The only reason I fixed the logic for return_to_handler() in the
unwinder is because the logic is already there.
Last stack frame
================
If a SYM_CODE function occurs in the very last frame in the stack trace,
then the stack trace is not considered unreliable. This is because there
is no more unwinding to do. Examples:
- EL0 exception stack traces end in the top level EL0 exception
handlers.
- All kernel thread stack traces end in ret_from_fork().
Special cases
=============
Some special cases need to be mentioned.
EL1 interrupt and exception handlers are present in .entry.text. So, all
EL1 interrupt and exception stack traces will be considered unreliable.
This the correct behavior as interrupts and exceptions can happen on any
instruction including ones in the frame pointer prolog and epilog. Unless
objtool generates metadata so the unwinder can unwind through these
special cases, such stack traces will be considered unreliable.
A task can get preempted at the end of an interrupt. Stack traces of
preempted tasks will show the interrupt frame in the stack trace and
will be considered unreliable.
Breakpoints are exceptions. So, all stack traces in the break point
handler (including probes) will be considered unreliable.
All of the ftrace trampoline code that gets executed at the beginning
of a traced function falls in ".code.text". All stack traces taken from
tracer functions will be considered unreliable.
The same is true for kretprobe trampolines.
---
Changelog:
v3:
- Implemented a sym_code_ranges[] array to contains sections bounds
for text sections that contain SYM_CODE_*() functions. The unwinder
checks each return PC against the sections. If it falls in any of
the sections, the stack trace is marked unreliable.
- Moved SYM_CODE functions from .text and .init.text into a new
text section called ".code.text". Added this section to
vmlinux.lds.S and sym_code_ranges[].
- Fixed the logic in the unwinder that handles Function Graph
Tracer return trampoline.
- Removed all the previous code that handles:
- ftrace entry code for traced function
- special_functions[] array that lists individual functions
- kretprobe_trampoline() special case
- Synced with mainline v5.12-rc8.
v2
- Removed the terminating entry { 0, 0 } in special_functions[]
and replaced it with the idiom { /* sentinel */ }.
- Change the ftrace trampoline entry ftrace_graph_call in
special_functions[] to ftrace_call + 4 and added explanatory
comments.
- Unnested #ifdefs in special_functions[] for FTRACE.
v1
- Define a bool field in struct stackframe. This will indicate if
a stack trace is reliable.
- Implement a special_functions[] array that will be populated
with special functions in which the stack trace is considered
unreliable.
- Using kallsyms_lookup(), get the address ranges for the special
functions and record them.
- Implement an is_reliable_function(pc). This function will check
if a given return PC falls in any of the special functions. If
it does, the stack trace is unreliable.
- Implement check_reliability() function that will check if a
stack frame is reliable. Call is_reliable_function() from
check_reliability().
- Before a return PC is checked against special_funtions[], it
must be validates as a proper kernel text address. Call
__kernel_text_address() from check_reliability().
- Finally, call check_reliability() from unwind_frame() for
each stack frame.
- Add EL1 exception handlers to special_functions[].
el1_sync();
el1_irq();
el1_error();
el1_sync_invalid();
el1_irq_invalid();
el1_fiq_invalid();
el1_error_invalid();
- The above functions are currently defined as LOCAL symbols.
Make them global so that they can be referenced from the
unwinder code.
- Add FTRACE trampolines to special_functions[]:
ftrace_graph_call()
ftrace_graph_caller()
return_to_handler()
- Add the kretprobe trampoline to special functions[]:
kretprobe_trampoline()
Previous versions and discussion
================================
v2: https://lore.kernel.org/linux-arm-kernel/20210405204313.21346-1-madvenka@linux.microsoft.com/
v1: https://lore.kernel.org/linux-arm-kernel/20210330190955.13707-1-madvenka@linux.microsoft.com/
Madhavan T. Venkataraman (4):
arm64: Introduce stack trace reliability checks in the unwinder
arm64: Check the return PC against unreliable code sections
arm64: Handle miscellaneous functions in .text and .init.text
arm64: Handle funtion graph tracer better in the unwinder
arch/arm64/include/asm/sections.h | 1 +
arch/arm64/include/asm/stacktrace.h | 7 +
arch/arm64/kernel/entry-ftrace.S | 5 +
arch/arm64/kernel/entry.S | 6 +
arch/arm64/kernel/head.S | 3 +-
arch/arm64/kernel/probes/kprobes_trampoline.S | 2 +
arch/arm64/kernel/stacktrace.c | 129 ++++++++++++++++--
arch/arm64/kernel/vmlinux.lds.S | 7 +
8 files changed, 149 insertions(+), 11 deletions(-)
base-commit: bf05bf16c76bb44ab5156223e1e58e26dfe30a88
--
2.25.1
From: "Madhavan T. Venkataraman" <redacted>
The unwinder should check for the presence of various features and
conditions that can render the stack trace unreliable and mark the
the stack trace as unreliable for the benefit of the caller.
Introduce the first reliability check - If a return PC encountered in a
stack trace is not a valid kernel text address, the stack trace is
considered unreliable. It could be some generated code. Mark the stack
trace unreliable.
Other reliability checks will be added in the future.
Signed-off-by: Madhavan T. Venkataraman <redacted>
---
arch/arm64/include/asm/stacktrace.h | 4 ++++
arch/arm64/kernel/stacktrace.c | 19 ++++++++++++++++---
2 files changed, 20 insertions(+), 3 deletions(-)
From: "Madhavan T. Venkataraman" <redacted>
Create a sym_code_ranges[] array to cover the following text sections that
contain functions defined as SYM_CODE_*(). These functions are low-level
functions (and do not have a proper frame pointer prolog and epilog). So,
they are inherently unreliable from a stack unwinding perspective.
.entry.text
.idmap.text
.hyp.idmap.text
.hyp.text
.hibernate_exit.text
.entry.tramp.text
If a return PC falls in any of these, mark the stack trace unreliable.
The only exception to this is - if the unwinder has reached the last
frame already, it will not mark the stack trace unreliable since there
is no more unwinding to do. E.g.,
- ret_from_fork() occurs at the end of the stack trace of
kernel tasks.
- el0_*() functions occur at the end of EL0 exception stack
traces. This covers all user task entries into the kernel.
NOTE:
- EL1 exception handlers are in .entry.text. So, stack traces that
contain those functions will be marked not reliable. This covers
interrupts, exceptions and breakpoints encountered while executing
in the kernel.
- At the end of an interrupt, the kernel can preempt the current
task if required. So, the stack traces of all preempted tasks will
show the interrupt frame and will be considered unreliable.
Signed-off-by: Madhavan T. Venkataraman <redacted>
---
arch/arm64/kernel/stacktrace.c | 54 ++++++++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
From: "Madhavan T. Venkataraman" <redacted>
There are some SYM_CODE functions that are currently in ".text" or
".init.text" sections. Some of these are functions that the unwinder
does not care about as they are not "interesting" to livepatch. These
will remain in their current sections. The rest I have moved into a
new section called ".code.text".
Include .code.text in sym_code_ranges[] so the unwinder can check it.
I have listed the names of the functions along with the name of their
existing section.
Don't care functions
====================
efi-entry.S:
efi_enter_kernel .init.text
relocate_kernel.S:
arm64_relocate_new_kernel .text
sigreturn.S:
__kernel_rt_sigreturn .text
arch/arm64/kvm/hyp/hyp-entry.S:
el2t_sync_invalid .text
el2t_irq_invalid .text
el2t_fiq_invalid .text
el2t_error_invalid .text
el2h_irq_invalid .text
el2h_fiq_invalid .text
el1_fiq_invalid .text
__kvm_hyp_vector .text
__bp_harden_hyp_vecs .text
arch/arm64/kvm/hyp/nvhe/host.S:
__kvm_hyp_host_vector .text
__kvm_hyp_host_forward_smc .text
Rest of the functions (moved to .code.text)
=====================
entry.S:
__swpan_entry_el1 .text
__swpan_exit_el1 .text
__swpan_entry_el0 .text
__swpan_exit_el0 .text
ret_from_fork .text
__sdei_asm_handler .text
head.S:
primary_entry .init.text
preserve_boot_args .init.text
entry-ftrace.S:
ftrace_regs_caller .text
ftrace_caller .text
ftrace_common .text
ftrace_graph_caller .text
return_to_handler .text
kprobes_trampoline.S:
kretprobe_trampoline .text
Signed-off-by: Madhavan T. Venkataraman <redacted>
---
arch/arm64/include/asm/sections.h | 1 +
arch/arm64/kernel/entry-ftrace.S | 5 +++++
arch/arm64/kernel/entry.S | 6 ++++++
arch/arm64/kernel/head.S | 3 ++-
arch/arm64/kernel/probes/kprobes_trampoline.S | 2 ++
arch/arm64/kernel/stacktrace.c | 2 ++
arch/arm64/kernel/vmlinux.lds.S | 7 +++++++
7 files changed, 25 insertions(+), 1 deletion(-)
From: "Madhavan T. Venkataraman" <redacted>
The Function Graph Tracer modifies the return address of a traced function
to a return trampoline (return_to_handler()) to gather tracing data on
function return. When the unwinder encounters return_to_handler(), it calls
ftrace_graph_get_ret_stack() to lookup the original return address in the
return address stack.
This lookup will succeed as long as the unwinder is invoked when the traced
function is executing. However, when the traced function returns and control
goes to return_to_handler(), this lookup will not succeed because:
- the return address on the stack would not be return_to_handler. It would
be return_to_handler+someoffset. To solve this, get the address range for
return_to_handler() by looking up its symbol table entry and check if
frame->pc falls in the range. This is also required for the unwinder to
maintain the index into the return address stack correctly as it unwinds
through Function Graph trace return trampolines.
- the original return address will be popped off the return address stack
at some point. From this point till the end of return_to_handler(),
the lookup will not succeed. The stack trace is unreliable in that
window.
On arm64, each return address stack entry also stores the FP of the
caller of the traced function. Compare the FP in the current frame
with the entry that is looked up. If the FP matches, then, all is
well. Else, it is in the window. mark the stack trace unreliable.
Although it is possible to close the window mentioned above, it is
not worth it. It is a tiny window.
Signed-off-by: Madhavan T. Venkataraman <redacted>
---
arch/arm64/include/asm/stacktrace.h | 3 ++
arch/arm64/kernel/stacktrace.c | 60 ++++++++++++++++++++++++-----
2 files changed, 53 insertions(+), 10 deletions(-)
@@ -48,6 +51,33 @@ struct code_range sym_code_ranges[] ={/* sentinel */}};+voidinit_ranges(void)+{+staticcharsym[KSYM_NAME_LEN];+staticboolinited=false;+structcode_range*range;+unsignedlongpc,size,offset;++if(inited)+return;++for(range=sym_code_ranges;range->start;range++){+if(range->end)+continue;++pc=(unsignedlong)range->start;+if(kallsyms_lookup(pc,&size,&offset,NULL,sym)){+range->start=pc-offset;+range->end=range->start+size;+}else{+/* Range will only include one instruction */+range->start=pc;+range->end=pc+4;+}+}+inited=true;+}+staticstructcode_range*lookup_range(unsignedlongpc){structcode_range*range;
From: Mark Brown <broonie@kernel.org> Date: 2021-05-04 15:51:37
On Mon, May 03, 2021 at 12:36:12PM -0500, madvenka@linux.microsoft.com wrote:
+ /*
+ * First, make sure that the return address is a proper kernel text
+ * address. A NULL or invalid return address probably means there's
+ * some generated code which __kernel_text_address() doesn't know
+ * about. Mark the stack trace as not reliable.
+ */
+ if (!__kernel_text_address(frame->pc)) {
+ frame->reliable = false;
+ return 0;
+ }
Do we want the return here? It means that...
quoted hunk
+
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
if (tsk->ret_stack &&
- (ptrauth_strip_insn_pac(frame->pc) == (unsigned long)return_to_handler)) {
+ frame->pc == (unsigned long)return_to_handler) {
struct ftrace_ret_stack *ret_stack;
/*
* This is a case where function graph tracer has
...we skip this handling in the case where we're not in kernel code. I
don't know off hand if that's a case that can happen right now but it
seems more robust to run through this and anything else we add later,
even if it's not relevant now changes either in the unwinder itself or
resulting from some future work elsewhere may mean it later becomes
important. Skipping futher reliability checks is obviously fine if
we've already decided things aren't reliable but this is more than just
a reliability check.
From: Mark Brown <broonie@kernel.org> Date: 2021-05-04 16:05:46
On Mon, May 03, 2021 at 12:36:13PM -0500, madvenka@linux.microsoft.com wrote:
From: "Madhavan T. Venkataraman" <redacted>
Create a sym_code_ranges[] array to cover the following text sections that
contain functions defined as SYM_CODE_*(). These functions are low-level
This makes sense to me - a few of bikesheddy comments below but nothing
really substantive.
+static struct code_range *lookup_range(unsigned long pc)
This feels like it should have a prefix on the name (eg, unwinder_)
since it looks collision prone. Or lookup_code_range() rather than just
plain lookup_range().
+{
+ struct code_range *range;
+
+ for (range = sym_code_ranges; range->start; range++) {
It seems more idiomatic to use ARRAY_SIZE() rather than a sentinel here,
the array can't be empty.
quoted hunk
+ range = lookup_range(frame->pc);
+
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
if (tsk->ret_stack &&
frame->pc == (unsigned long)return_to_handler) {
Do we not need to look up the range of the restored pc and validate
what's being pointed to here? It's not immediately obvious why we do
the lookup before handling the function graph tracer, especially given
that we never look at the result and there's now a return added skipping
further reliability checks. At the very least I think this needs some
additional comments so the code is more obvious.
From: Madhavan T. Venkataraman <hidden> Date: 2021-05-04 19:03:21
On 5/4/21 11:05 AM, Mark Brown wrote:
On Mon, May 03, 2021 at 12:36:13PM -0500, madvenka@linux.microsoft.com wrote:
quoted
From: "Madhavan T. Venkataraman" <redacted>
Create a sym_code_ranges[] array to cover the following text sections that
contain functions defined as SYM_CODE_*(). These functions are low-level
This makes sense to me - a few of bikesheddy comments below but nothing
really substantive.
OK.
quoted
+static struct code_range *lookup_range(unsigned long pc)
This feels like it should have a prefix on the name (eg, unwinder_)
since it looks collision prone. Or lookup_code_range() rather than just
plain lookup_range().
I will add the prefix.
quoted
+{
+ struct code_range *range;
+
+ for (range = sym_code_ranges; range->start; range++) {
It seems more idiomatic to use ARRAY_SIZE() rather than a sentinel here,
the array can't be empty.
If there is a match, I return the matched range. Else, I return the sentinel.
This is just so I don't have to check for range == NULL after calling
lookup_range().
I will change it to what you have suggested and check for NULL explicitly.
It is not a problem.
quoted
+ range = lookup_range(frame->pc);
+
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
if (tsk->ret_stack &&
frame->pc == (unsigned long)return_to_handler) {
Do we not need to look up the range of the restored pc and validate
what's being pointed to here? It's not immediately obvious why we do
the lookup before handling the function graph tracer, especially given
that we never look at the result and there's now a return added skipping
further reliability checks. At the very least I think this needs some
additional comments so the code is more obvious.
I want sym_code_ranges[] to contain both unwindable and non-unwindable ranges.
Unwindable ranges will be special ranges such as the return_to_handler() and
kretprobe_trampoline() functions for which the unwinder has (or will have)
special code to unwind. So, the lookup_range() has to happen before the
function graph code. Please look at the last patch in the series for
the fix for the above function graph code.
On the question of "should the original return address be checked against
sym_code_ranges[]?" - I assumed that if there is a function graph trace on a
function, it had to be an ftraceable function. It would not be a part
of sym_code_ranges[]. Is that a wrong assumption on my part?
Madhavan
From: Madhavan T. Venkataraman <hidden> Date: 2021-05-04 19:14:22
On 5/4/21 10:50 AM, Mark Brown wrote:
On Mon, May 03, 2021 at 12:36:12PM -0500, madvenka@linux.microsoft.com wrote:
quoted
+ /*
+ * First, make sure that the return address is a proper kernel text
+ * address. A NULL or invalid return address probably means there's
+ * some generated code which __kernel_text_address() doesn't know
+ * about. Mark the stack trace as not reliable.
+ */
+ if (!__kernel_text_address(frame->pc)) {
+ frame->reliable = false;
+ return 0;
+ }
Do we want the return here? It means that...
quoted
+
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
if (tsk->ret_stack &&
- (ptrauth_strip_insn_pac(frame->pc) == (unsigned long)return_to_handler)) {
+ frame->pc == (unsigned long)return_to_handler) {
struct ftrace_ret_stack *ret_stack;
/*
* This is a case where function graph tracer has
...we skip this handling in the case where we're not in kernel code. I
don't know off hand if that's a case that can happen right now but it
seems more robust to run through this and anything else we add later,
even if it's not relevant now changes either in the unwinder itself or
resulting from some future work elsewhere may mean it later becomes
important. Skipping futher reliability checks is obviously fine if
we've already decided things aren't reliable but this is more than just
a reliability check.
AFAICT, currently, all the functions that the unwinder checks do have
valid kernel text addresses. However, I don't think there is any harm
in letting it fall through and make all the checks. So, I will remove the
return statement.
Thanks!
Madhavan
From: Madhavan T. Venkataraman <hidden> Date: 2021-05-04 19:32:40
On 5/4/21 2:03 PM, Madhavan T. Venkataraman wrote:
On 5/4/21 11:05 AM, Mark Brown wrote:
quoted
On Mon, May 03, 2021 at 12:36:13PM -0500, madvenka@linux.microsoft.com wrote:
quoted
From: "Madhavan T. Venkataraman" <redacted>
Create a sym_code_ranges[] array to cover the following text sections that
contain functions defined as SYM_CODE_*(). These functions are low-level
This makes sense to me - a few of bikesheddy comments below but nothing
really substantive.
OK.
quoted
quoted
+static struct code_range *lookup_range(unsigned long pc)
This feels like it should have a prefix on the name (eg, unwinder_)
since it looks collision prone. Or lookup_code_range() rather than just
plain lookup_range().
I will add the prefix.
quoted
quoted
+{
+ struct code_range *range;
+
+ for (range = sym_code_ranges; range->start; range++) {
It seems more idiomatic to use ARRAY_SIZE() rather than a sentinel here,
the array can't be empty.
If there is a match, I return the matched range. Else, I return the sentinel.
This is just so I don't have to check for range == NULL after calling
lookup_range().
I will change it to what you have suggested and check for NULL explicitly.
It is not a problem.
quoted
quoted
+ range = lookup_range(frame->pc);
+
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
if (tsk->ret_stack &&
frame->pc == (unsigned long)return_to_handler) {
Do we not need to look up the range of the restored pc and validate
what's being pointed to here? It's not immediately obvious why we do
the lookup before handling the function graph tracer, especially given
that we never look at the result and there's now a return added skipping
further reliability checks. At the very least I think this needs some
additional comments so the code is more obvious.
I want sym_code_ranges[] to contain both unwindable and non-unwindable ranges.
Unwindable ranges will be special ranges such as the return_to_handler() and
kretprobe_trampoline() functions for which the unwinder has (or will have)
special code to unwind. So, the lookup_range() has to happen before the
function graph code. Please look at the last patch in the series for
the fix for the above function graph code.
On the question of "should the original return address be checked against
sym_code_ranges[]?" - I assumed that if there is a function graph trace on a
function, it had to be an ftraceable function. It would not be a part
of sym_code_ranges[]. Is that a wrong assumption on my part?
If you prefer, I could do something like this:
check_pc:
if (!__kernel_text_address(frame->pc))
frame->reliable = false;
range = lookup_range(frame->pc);
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
if (tsk->ret_stack &&
frame->pc == (unsigned long)return_to_handler) {
...
frame->pc = ret_stack->ret;
frame->pc = ptrauth_strip_insn_pac(frame->pc);
goto check_pc;
}
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
Is that acceptable?
Madhavan
Why set 'reliable' to true on every invocation of unwind_frame()?
Shouldn't it be remembered across frames?
Also, it looks like there are several error scenarios where it returns
-EINVAL but doesn't set 'reliable' to false.
--
Josh
Why set 'reliable' to true on every invocation of unwind_frame()?
Shouldn't it be remembered across frames?
This is mainly for debug purposes in case a caller wants to print the whole stack and also
print which functions are unreliable. For livepatch, it does not make any difference. It will
quit as soon as it encounters an unreliable frame.
Also, it looks like there are several error scenarios where it returns
-EINVAL but doesn't set 'reliable' to false.
I wanted to make a distinction between an error situation (like stack corruption where unwinding
has to stop) and an unreliable situation (where unwinding can still proceed). E.g., when a
stack trace is taken for informational purposes or debug purposes, the unwinding will try to
proceed until either the stack trace ends or an error happens.
Madhavan
Why set 'reliable' to true on every invocation of unwind_frame()?
Shouldn't it be remembered across frames?
This is mainly for debug purposes in case a caller wants to print the whole stack and also
print which functions are unreliable. For livepatch, it does not make any difference. It will
quit as soon as it encounters an unreliable frame.
Hm, ok. So 'frame->reliable' refers to the current frame, not the
entire stack.
quoted
Also, it looks like there are several error scenarios where it returns
-EINVAL but doesn't set 'reliable' to false.
I wanted to make a distinction between an error situation (like stack corruption where unwinding
has to stop) and an unreliable situation (where unwinding can still proceed). E.g., when a
stack trace is taken for informational purposes or debug purposes, the unwinding will try to
proceed until either the stack trace ends or an error happens.
Ok, but I don't understand how that relates to my comment.
Why wouldn't a stack corruption like !on_accessible_stack() set
'frame->reliable' to false?
In other words: for livepatch purposes, how does the caller tell the
difference between hitting the final stack record -- which returns an
error with reliable 'true' -- and a stack corruption like
!on_accessible_stack(), which also returns an error with reliable
'true'? Surely the latter should be considered unreliable?
--
Josh
Why set 'reliable' to true on every invocation of unwind_frame()?
Shouldn't it be remembered across frames?
This is mainly for debug purposes in case a caller wants to print the whole stack and also
print which functions are unreliable. For livepatch, it does not make any difference. It will
quit as soon as it encounters an unreliable frame.
Hm, ok. So 'frame->reliable' refers to the current frame, not the
entire stack.
Yes.
quoted
quoted
Also, it looks like there are several error scenarios where it returns
-EINVAL but doesn't set 'reliable' to false.
I wanted to make a distinction between an error situation (like stack corruption where unwinding
has to stop) and an unreliable situation (where unwinding can still proceed). E.g., when a
stack trace is taken for informational purposes or debug purposes, the unwinding will try to
proceed until either the stack trace ends or an error happens.
Ok, but I don't understand how that relates to my comment.
Why wouldn't a stack corruption like !on_accessible_stack() set
'frame->reliable' to false?
I do see your point. If an error has been hit, then the stack trace is essentially unreliable
regardless of anything else. So, I accept your comment. I will mark the stack trace as unreliable
if any kind of error is encountered.
Thanks!
Madhavan
Do we not need to look up the range of the restored pc and validate
what's being pointed to here? It's not immediately obvious why we do
the lookup before handling the function graph tracer, especially given
that we never look at the result and there's now a return added skipping
further reliability checks. At the very least I think this needs some
additional comments so the code is more obvious.
I want sym_code_ranges[] to contain both unwindable and non-unwindable ranges.
Unwindable ranges will be special ranges such as the return_to_handler() and
kretprobe_trampoline() functions for which the unwinder has (or will have)
special code to unwind. So, the lookup_range() has to happen before the
function graph code. Please look at the last patch in the series for
the fix for the above function graph code.
That sounds reasonable but like I say should probably be called out in
the code so it's clear to people working with it.
On the question of "should the original return address be checked against
sym_code_ranges[]?" - I assumed that if there is a function graph trace on a
function, it had to be an ftraceable function. It would not be a part
of sym_code_ranges[]. Is that a wrong assumption on my part?
I can't think of any cases where it wouldn't be right now, but it seems
easier to just do a redundant check than to have the assumption in the
code and have to think about if it's missing.
From: Mark Brown <broonie@kernel.org> Date: 2021-05-05 17:21:43
On Tue, May 04, 2021 at 02:32:35PM -0500, Madhavan T. Venkataraman wrote:
If you prefer, I could do something like this:
check_pc:
if (!__kernel_text_address(frame->pc))
frame->reliable = false;
range = lookup_range(frame->pc);
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
if (tsk->ret_stack &&
frame->pc == (unsigned long)return_to_handler) {
...
frame->pc = ret_stack->ret;
frame->pc = ptrauth_strip_insn_pac(frame->pc);
goto check_pc;
}
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
Is that acceptable?
I think that works even if it's hard to love the goto, might want some
defensiveness to ensure we can't somehow end up in an infinite loop with
a sufficiently badly formed stack.
Do we not need to look up the range of the restored pc and validate
what's being pointed to here? It's not immediately obvious why we do
the lookup before handling the function graph tracer, especially given
that we never look at the result and there's now a return added skipping
further reliability checks. At the very least I think this needs some
additional comments so the code is more obvious.
quoted
I want sym_code_ranges[] to contain both unwindable and non-unwindable ranges.
Unwindable ranges will be special ranges such as the return_to_handler() and
kretprobe_trampoline() functions for which the unwinder has (or will have)
special code to unwind. So, the lookup_range() has to happen before the
function graph code. Please look at the last patch in the series for
the fix for the above function graph code.
That sounds reasonable but like I say should probably be called out in
the code so it's clear to people working with it.
OK. To make this better, I will do the lookup_range() after the function
graph code to begin with. Then, in the last patch for the function graph
code, I will move it up. This way, the code is clear and your comment
is addressed.
quoted
On the question of "should the original return address be checked against
sym_code_ranges[]?" - I assumed that if there is a function graph trace on a
function, it had to be an ftraceable function. It would not be a part
of sym_code_ranges[]. Is that a wrong assumption on my part?
I can't think of any cases where it wouldn't be right now, but it seems
easier to just do a redundant check than to have the assumption in the
code and have to think about if it's missing.
From: Madhavan T. Venkataraman <hidden> Date: 2021-05-05 18:48:30
On 5/5/21 11:46 AM, Mark Brown wrote:
On Tue, May 04, 2021 at 02:32:35PM -0500, Madhavan T. Venkataraman wrote:
quoted
If you prefer, I could do something like this:
check_pc:
if (!__kernel_text_address(frame->pc))
frame->reliable = false;
range = lookup_range(frame->pc);
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
if (tsk->ret_stack &&
frame->pc == (unsigned long)return_to_handler) {
...
frame->pc = ret_stack->ret;
frame->pc = ptrauth_strip_insn_pac(frame->pc);
goto check_pc;
}
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
quoted
Is that acceptable?
I think that works even if it's hard to love the goto, might want some
defensiveness to ensure we can't somehow end up in an infinite loop with
a sufficiently badly formed stack.
I could do something like this:
- Move all frame->pc checking code into a function called check_frame_pc().
bool check_frame_pc(frame)
{
Do all the checks including function graph
return frame->pc changed
}
- Then, in unwind_frame()
unwind_frame()
{
int i;
...
for (i = 0; i < MAX_CHECKS; i++) {
if (!check_frame(tsk, frame))
break;
}
if (i == MAX_CHECKS)
frame->reliable = false;
return 0;
}
The above would take care of future cases like kretprobe_trampoline().
If this is acceptable, then the only question is - what should be the value of
MAX_CHECKS (I will rename it to something more appropriate)?
Madhavan
From: Madhavan T. Venkataraman <hidden> Date: 2021-05-05 18:50:30
On 5/5/21 1:48 PM, Madhavan T. Venkataraman wrote:
On 5/5/21 11:46 AM, Mark Brown wrote:
quoted
On Tue, May 04, 2021 at 02:32:35PM -0500, Madhavan T. Venkataraman wrote:
quoted
If you prefer, I could do something like this:
check_pc:
if (!__kernel_text_address(frame->pc))
frame->reliable = false;
range = lookup_range(frame->pc);
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
if (tsk->ret_stack &&
frame->pc == (unsigned long)return_to_handler) {
...
frame->pc = ret_stack->ret;
frame->pc = ptrauth_strip_insn_pac(frame->pc);
goto check_pc;
}
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
quoted
Is that acceptable?
I think that works even if it's hard to love the goto, might want some
defensiveness to ensure we can't somehow end up in an infinite loop with
a sufficiently badly formed stack.
I could do something like this:
- Move all frame->pc checking code into a function called check_frame_pc().
bool check_frame_pc(frame)
{
Do all the checks including function graph
return frame->pc changed
}
- Then, in unwind_frame()
unwind_frame()
{
int i;
...
for (i = 0; i < MAX_CHECKS; i++) {
if (!check_frame(tsk, frame))
Small typo in the last statement - It should be check_frame_pc().
Sorry.
Madhavan
break;
}
if (i == MAX_CHECKS)
frame->reliable = false;
return 0;
}
The above would take care of future cases like kretprobe_trampoline().
If this is acceptable, then the only question is - what should be the value of
MAX_CHECKS (I will rename it to something more appropriate)?
Madhavan
On Mon, 3 May 2021 at 19:38, [off-list ref] wrote:
quoted hunk
From: "Madhavan T. Venkataraman" <redacted>
Create a sym_code_ranges[] array to cover the following text sections that
contain functions defined as SYM_CODE_*(). These functions are low-level
functions (and do not have a proper frame pointer prolog and epilog). So,
they are inherently unreliable from a stack unwinding perspective.
.entry.text
.idmap.text
.hyp.idmap.text
.hyp.text
.hibernate_exit.text
.entry.tramp.text
If a return PC falls in any of these, mark the stack trace unreliable.
The only exception to this is - if the unwinder has reached the last
frame already, it will not mark the stack trace unreliable since there
is no more unwinding to do. E.g.,
- ret_from_fork() occurs at the end of the stack trace of
kernel tasks.
- el0_*() functions occur at the end of EL0 exception stack
traces. This covers all user task entries into the kernel.
NOTE:
- EL1 exception handlers are in .entry.text. So, stack traces that
contain those functions will be marked not reliable. This covers
interrupts, exceptions and breakpoints encountered while executing
in the kernel.
- At the end of an interrupt, the kernel can preempt the current
task if required. So, the stack traces of all preempted tasks will
show the interrupt frame and will be considered unreliable.
Signed-off-by: Madhavan T. Venkataraman <redacted>
---
arch/arm64/kernel/stacktrace.c | 54 ++++++++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
@@ -103,6 +143,8 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame) return 0; }+ range = lookup_range(frame->pc);+ #ifdef CONFIG_FUNCTION_GRAPH_TRACER if (tsk->ret_stack && frame->pc == (unsigned long)return_to_handler) {
@@ -118,9 +160,21 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame) return -EINVAL; frame->pc = ret_stack->ret; frame->pc = ptrauth_strip_insn_pac(frame->pc);+ return 0; } #endif /* CONFIG_FUNCTION_GRAPH_TRACER */+ if (!range->start)+ return 0;++ /*+ * The return PC falls in an unreliable function. If the final frame+ * has been reached, no more unwinding is needed. Otherwise, mark the+ * stack trace not reliable.+ */+ if (frame->fp)+ frame->reliable = false;+ return 0; } NOKPROBE_SYMBOL(unwind_frame);--
From: Madhavan T. Venkataraman <hidden> Date: 2021-05-05 20:00:49
OK. I will make all the changes you suggested.
Thanks!
Madhavan
On 5/5/21 2:30 PM, Ard Biesheuvel wrote:
On Mon, 3 May 2021 at 19:38, [off-list ref] wrote:
quoted
From: "Madhavan T. Venkataraman" <redacted>
Create a sym_code_ranges[] array to cover the following text sections that
contain functions defined as SYM_CODE_*(). These functions are low-level
functions (and do not have a proper frame pointer prolog and epilog). So,
they are inherently unreliable from a stack unwinding perspective.
.entry.text
.idmap.text
.hyp.idmap.text
.hyp.text
.hibernate_exit.text
.entry.tramp.text
If a return PC falls in any of these, mark the stack trace unreliable.
The only exception to this is - if the unwinder has reached the last
frame already, it will not mark the stack trace unreliable since there
is no more unwinding to do. E.g.,
- ret_from_fork() occurs at the end of the stack trace of
kernel tasks.
- el0_*() functions occur at the end of EL0 exception stack
traces. This covers all user task entries into the kernel.
NOTE:
- EL1 exception handlers are in .entry.text. So, stack traces that
contain those functions will be marked not reliable. This covers
interrupts, exceptions and breakpoints encountered while executing
in the kernel.
- At the end of an interrupt, the kernel can preempt the current
task if required. So, the stack traces of all preempted tasks will
show the interrupt frame and will be considered unreliable.
Signed-off-by: Madhavan T. Venkataraman <redacted>
---
arch/arm64/kernel/stacktrace.c | 54 ++++++++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
@@ -103,6 +143,8 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame) return 0; }+ range = lookup_range(frame->pc);+ #ifdef CONFIG_FUNCTION_GRAPH_TRACER if (tsk->ret_stack && frame->pc == (unsigned long)return_to_handler) {
@@ -118,9 +160,21 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame) return -EINVAL; frame->pc = ret_stack->ret; frame->pc = ptrauth_strip_insn_pac(frame->pc);+ return 0; } #endif /* CONFIG_FUNCTION_GRAPH_TRACER */+ if (!range->start)+ return 0;++ /*+ * The return PC falls in an unreliable function. If the final frame+ * has been reached, no more unwinding is needed. Otherwise, mark the+ * stack trace not reliable.+ */+ if (frame->fp)+ frame->reliable = false;+ return 0; } NOKPROBE_SYMBOL(unwind_frame);--
From: Mark Brown <broonie@kernel.org> Date: 2021-05-06 13:46:22
On Wed, May 05, 2021 at 01:48:21PM -0500, Madhavan T. Venkataraman wrote:
On 5/5/21 11:46 AM, Mark Brown wrote:
quoted
I think that works even if it's hard to love the goto, might want some
defensiveness to ensure we can't somehow end up in an infinite loop with
a sufficiently badly formed stack.
I could do something like this:
unwind_frame()
{
int i;
...
for (i = 0; i < MAX_CHECKS; i++) {
if (!check_frame(tsk, frame))
break;
}
I think that could work, yes. Have to see the actual code (and other
people's opinions!).
If this is acceptable, then the only question is - what should be the value of
MAX_CHECKS (I will rename it to something more appropriate)?
I'd expect something like 10 to be way more than we'd ever need, or we
could define it down to the 2 checks we expect to be possible ATM to be
conservative. I'm tempted to be permissive if we have sufficient other
checks but I'm not 100% sure on that.
From: Mark Brown <broonie@kernel.org> Date: 2021-05-06 14:12:52
On Mon, May 03, 2021 at 12:36:14PM -0500, madvenka@linux.microsoft.com wrote:
There are some SYM_CODE functions that are currently in ".text" or
".init.text" sections. Some of these are functions that the unwinder
does not care about as they are not "interesting" to livepatch. These
will remain in their current sections. The rest I have moved into a
new section called ".code.text".
I was thinking it'd be good to do this by modifying SYM_CODE_START() to
emit the section, that way nobody can forget to put any SYM_CODE into a
special section. That does mean we'd have to first introduce a new
variant for specifying a section that lets us override things that need
to be in some specific section and convert everything that's in a
special section over to that first which is a bit annoying but feels
like it's worth it for the robustness. It'd also put some of the don't
cares into .code.text but so long as they are actually don't cares that
should be fine!
Don't care functions
====================
We also have a bunch of things like __cpu_soft_restart which don't seem
to be called out here but need to be in .idmap.text.
From: Mark Brown <broonie@kernel.org> Date: 2021-05-06 14:44:33
On Mon, May 03, 2021 at 12:36:15PM -0500, madvenka@linux.microsoft.com wrote:
From: "Madhavan T. Venkataraman" <redacted>
The Function Graph Tracer modifies the return address of a traced function
to a return trampoline (return_to_handler()) to gather tracing data on
function return. When the unwinder encounters return_to_handler(), it calls
ftrace_graph_get_ret_stack() to lookup the original return address in the
return address stack.
This makes sense to me, I'll need to re-review properly with the changes
earlier on in the series but should be fine.
From: Madhavan T. Venkataraman <hidden> Date: 2021-05-06 15:20:40
On 5/6/21 9:43 AM, Mark Brown wrote:
On Mon, May 03, 2021 at 12:36:15PM -0500, madvenka@linux.microsoft.com wrote:
quoted
From: "Madhavan T. Venkataraman" <redacted>
The Function Graph Tracer modifies the return address of a traced function
to a return trampoline (return_to_handler()) to gather tracing data on
function return. When the unwinder encounters return_to_handler(), it calls
ftrace_graph_get_ret_stack() to lookup the original return address in the
return address stack.
This makes sense to me, I'll need to re-review properly with the changes
earlier on in the series but should be fine.
I will make changes based on the comments I have received so far and send
out version 4 so everything is current for the next round of review.
Thanks!
Madhavan
From: Madhavan T. Venkataraman <hidden> Date: 2021-05-06 15:21:44
On 5/6/21 8:45 AM, Mark Brown wrote:
On Wed, May 05, 2021 at 01:48:21PM -0500, Madhavan T. Venkataraman wrote:
quoted
On 5/5/21 11:46 AM, Mark Brown wrote:
quoted
quoted
I think that works even if it's hard to love the goto, might want some
defensiveness to ensure we can't somehow end up in an infinite loop with
a sufficiently badly formed stack.
quoted
I could do something like this:
quoted
unwind_frame()
{
int i;
...
for (i = 0; i < MAX_CHECKS; i++) {
if (!check_frame(tsk, frame))
break;
}
I think that could work, yes. Have to see the actual code (and other
people's opinions!).
quoted
If this is acceptable, then the only question is - what should be the value of
MAX_CHECKS (I will rename it to something more appropriate)?
I'd expect something like 10 to be way more than we'd ever need, or we
could define it down to the 2 checks we expect to be possible ATM to be
conservative. I'm tempted to be permissive if we have sufficient other
checks but I'm not 100% sure on that.
OK. I will implement these changes for version 4 and send it out so this
whole thing can be reviewed again with the actual changes in front of us.
Madhavan
From: Madhavan T. Venkataraman <hidden> Date: 2021-05-06 15:30:34
On 5/6/21 9:12 AM, Mark Brown wrote:
On Mon, May 03, 2021 at 12:36:14PM -0500, madvenka@linux.microsoft.com wrote:
quoted
There are some SYM_CODE functions that are currently in ".text" or
".init.text" sections. Some of these are functions that the unwinder
does not care about as they are not "interesting" to livepatch. These
will remain in their current sections. The rest I have moved into a
new section called ".code.text".
I was thinking it'd be good to do this by modifying SYM_CODE_START() to
emit the section, that way nobody can forget to put any SYM_CODE into a
special section. That does mean we'd have to first introduce a new
variant for specifying a section that lets us override things that need
to be in some specific section and convert everything that's in a
special section over to that first which is a bit annoying but feels
like it's worth it for the robustness. It'd also put some of the don't
cares into .code.text but so long as they are actually don't cares that
should be fine!
OK. I could make the section an argument to SYM_CODE*() so that a developer
will never miss that. Some documentation may be in order so the guidelines
are clear. I will do the doc patch separately, if that is alright with
you all.
About the don't car functions - most of them are OK to be moved into
.code.text. But the hypervisor vector related code has a problem. I
have not debugged that yet. If I add that code in .code.text, KVM
initialization fails in one case. In another case, when I actually
test with a VM, the VM does not come up.
I am not sure. But it looks like there may be some reloc issue going on.
I don't have a handle on this yet. So, for now, I will leave the hypervisor
stuff in .text. But I will mark it as TBD in the cover letter so we don't
lose track of it.
quoted
Don't care functions
====================
We also have a bunch of things like __cpu_soft_restart which don't seem
to be called out here but need to be in .idmap.text.
It is already in .idmap.text.
/* SPDX-License-Identifier: GPL-2.0-only */
/*
* CPU reset routines
*
* Copyright (C) 2001 Deep Blue Solutions Ltd.
* Copyright (C) 2012 ARM Ltd.
* Copyright (C) 2015 Huawei Futurewei Technologies.
*/
#include <linux/linkage.h>
#include <asm/assembler.h>
#include <asm/sysreg.h>
#include <asm/virt.h>
.text
.pushsection .idmap.text, "awx"
/*
* __cpu_soft_restart(el2_switch, entry, arg0, arg1, arg2) - Helper for
* cpu_soft_restart.
*
* @el2_switch: Flag to indicate a switch to EL2 is needed.
* @entry: Location to jump to for soft reset.
* arg0: First argument passed to @entry. (relocation list)
* arg1: Second argument passed to @entry.(physical kernel entry)
* arg2: Third argument passed to @entry. (physical dtb address)
*
* Put the CPU into the same state as it would be if it had been reset, and
* branch to what would be the reset vector. It must be executed with the
* flat identity mapping.
*/
SYM_CODE_START(__cpu_soft_restart)
mov_q x12, INIT_SCTLR_EL1_MMU_OFF
pre_disable_mmu_workaround
/*
* either disable EL1&0 translation regime or disable EL2&0 translation
* regime if HCR_EL2.E2H == 1
*/
msr sctlr_el1, x12
isb
cbz x0, 1f // el2_switch?
mov x0, #HVC_SOFT_RESTART
hvc #0 // no return
1: mov x8, x1 // entry
mov x0, x2 // arg0
mov x1, x3 // arg1
mov x2, x4 // arg2
br x8
SYM_CODE_END(__cpu_soft_restart)
.popsection
I will double check all the *.S files and make sure that every function is accounted
for in version 4.
Stay tuned.
Thanks.
Madhavan
From: Madhavan T. Venkataraman <hidden> Date: 2021-05-06 15:32:37
On 5/6/21 10:30 AM, Madhavan T. Venkataraman wrote:
quoted
I was thinking it'd be good to do this by modifying SYM_CODE_START() to
emit the section, that way nobody can forget to put any SYM_CODE into a
special section. That does mean we'd have to first introduce a new
variant for specifying a section that lets us override things that need
to be in some specific section and convert everything that's in a
special section over to that first which is a bit annoying but feels
like it's worth it for the robustness. It'd also put some of the don't
cares into .code.text but so long as they are actually don't cares that
should be fine!
OK. I could make the section an argument to SYM_CODE*() so that a developer
will never miss that. Some documentation may be in order so the guidelines
are clear. I will do the doc patch separately, if that is alright with
you all.
There is just one problem with this. Sometimes, there is some data in the
same text section. That data will not get included when we do the SYM_CODE(section)
change.
Madhavan
From: Mark Brown <broonie@kernel.org> Date: 2021-05-06 15:39:26
On Thu, May 06, 2021 at 10:30:21AM -0500, Madhavan T. Venkataraman wrote:
On 5/6/21 9:12 AM, Mark Brown wrote:
quoted
On Mon, May 03, 2021 at 12:36:14PM -0500, madvenka@linux.microsoft.com wrote:
quoted
I was thinking it'd be good to do this by modifying SYM_CODE_START() to
emit the section, that way nobody can forget to put any SYM_CODE into a
special section. That does mean we'd have to first introduce a new
OK. I could make the section an argument to SYM_CODE*() so that a developer
will never miss that. Some documentation may be in order so the guidelines
are clear. I will do the doc patch separately, if that is alright with
you all.
I was thinking to have standard SYM_CODE default to a section then a
variant for anything that cares (like how we have SYM_FUNC_PI and
friends for the PI code for EFI).
quoted
We also have a bunch of things like __cpu_soft_restart which don't seem
to be called out here but need to be in .idmap.text.
It is already in .idmap.text.
Right, I meant that I was expecting to see things that need to be in a
specific section other than .code.text called out separately here if
we're enumerating them. Though if the annotations are done separately
then this patch wouldn't need to do that calling out at all, it'd be
covered as part of fiddling around with the annotations.
From: Mark Brown <broonie@kernel.org> Date: 2021-05-06 15:45:28
On Thu, May 06, 2021 at 10:32:30AM -0500, Madhavan T. Venkataraman wrote:
On 5/6/21 10:30 AM, Madhavan T. Venkataraman wrote:
quoted
OK. I could make the section an argument to SYM_CODE*() so that a developer
will never miss that. Some documentation may be in order so the guidelines
are clear. I will do the doc patch separately, if that is alright with
you all.
There is just one problem with this. Sometimes, there is some data in the
same text section. That data will not get included when we do the SYM_CODE(section)
change.
Yes, data would need to be handled separately still. That doesn't seem
insurmountable though?
From: Madhavan T. Venkataraman <hidden> Date: 2021-05-06 15:56:39
On 5/6/21 10:44 AM, Mark Brown wrote:
On Thu, May 06, 2021 at 10:32:30AM -0500, Madhavan T. Venkataraman wrote:
quoted
On 5/6/21 10:30 AM, Madhavan T. Venkataraman wrote:
quoted
quoted
OK. I could make the section an argument to SYM_CODE*() so that a developer
will never miss that. Some documentation may be in order so the guidelines
are clear. I will do the doc patch separately, if that is alright with
you all.
quoted
There is just one problem with this. Sometimes, there is some data in the
same text section. That data will not get included when we do the SYM_CODE(section)
change.
Yes, data would need to be handled separately still. That doesn't seem
insurmountable though?
From: Madhavan T. Venkataraman <hidden> Date: 2021-05-06 15:57:14
On 5/6/21 10:37 AM, Mark Brown wrote:
On Thu, May 06, 2021 at 10:30:21AM -0500, Madhavan T. Venkataraman wrote:
quoted
On 5/6/21 9:12 AM, Mark Brown wrote:
quoted
On Mon, May 03, 2021 at 12:36:14PM -0500, madvenka@linux.microsoft.com wrote:
quoted
quoted
I was thinking it'd be good to do this by modifying SYM_CODE_START() to
emit the section, that way nobody can forget to put any SYM_CODE into a
special section. That does mean we'd have to first introduce a new
quoted
OK. I could make the section an argument to SYM_CODE*() so that a developer
will never miss that. Some documentation may be in order so the guidelines
are clear. I will do the doc patch separately, if that is alright with
you all.
I was thinking to have standard SYM_CODE default to a section then a
variant for anything that cares (like how we have SYM_FUNC_PI and
friends for the PI code for EFI).
OK.
quoted
quoted
We also have a bunch of things like __cpu_soft_restart which don't seem
to be called out here but need to be in .idmap.text.
quoted
It is already in .idmap.text.
Right, I meant that I was expecting to see things that need to be in a
specific section other than .code.text called out separately here if
we're enumerating them. Though if the annotations are done separately
then this patch wouldn't need to do that calling out at all, it'd be
covered as part of fiddling around with the annotations.