Hi Dave,
On 4/7/20 9:17 PM, Dave Anderson wrote:
Hi Amit,
I have a few suggestions and a question regarding your patch.
First, these two warnings need to be addressed:
$ make warn
...
cc -c -g -DARM64 -DLZO -DSNAPPY -DGDB_7_6 arm64.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes -fstack-protector -Wformat-security
arm64.c: In function ‘arm64_calc_KERNELPACMASK’:
arm64.c:4090:2: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 3 has type ‘ulong’ [-Wformat=]
fprintf(fp, " got NUMBER(KERNELPACMASK) =%llx\n", value);
^
arm64.c:4090:9: warning: ‘value’ may be used uninitialized in this function [-Wmaybe-uninitialized]
fprintf(fp, " got NUMBER(KERNELPACMASK) =%llx\n", value);
^
...
The message should be moved inside the if statement, and also should be gated
with CRASHDEBUG(1) to prevent it from being displayed unconditionally:
static void
arm64_calc_KERNELPACMASK(void)
{
ulong value;
char *string;
if ((string = pc->read_vmcoreinfo("NUMBER(KERNELPACMASK)"))) {
value = htol(string, QUIET, NULL);
free(string);
machdep->machspec->CONFIG_ARM64_KERNELPACMASK = value;
if (CRASHDEBUG(1))
fprintf(fp, "CONFIG_ARM64_KERNELPACMASK: %lx\n", value);
}
}
Yes thanks for pointing this out. I just posted v2 version and added
your suggestion.
And the CONFIG_ARM64_KERNELPACMASK value should be displayed in arm64_dump_machdep_table().
Ok.
But given that the patch only modifies text return addresses on the kernel
stack is here:
@@ -1932,6 +1936,9 @@ arm64_print_stackframe_entry(struct bt_info *bt, int level, struct arm64_stackfr
* See, for example, "bl schedule" before ret_to_user().
*/
branch_pc = frame->pc - 4;
+ if (ms->CONFIG_ARM64_KERNELPACMASK)
+ branch_pc |= ms->CONFIG_ARM64_KERNELPACMASK;
+
name = closest_symbol(branch_pc);
name_plus_offset = NULL;
I'm wondering how all of the other places that check addresses found
on the kernel stack will work? For example, all of these places
check whether an address found on the stack is a kernel text address:
$ grep is_kernel_text arm64.c
is_kernel_text(regs->pc) &&
is_kernel_text(regs->regs[30])) {
if (is_kernel_text(*up)) { <===== from arm64_print_text_symbols()
if (is_kernel_text(frame->pc) ||
if (is_kernel_text(frame->pc) &&
if (is_kernel_text(regs->pc) &&
if (is_kernel_text(LR) &&
if (is_kernel_text(regs->pc) && (bt->flags & BT_LINE_NUMBERS)) {
$
Except for the call arm64_print_text_symbols(), these are checking register
values found in exception frames. Can you confirm that they will still be
unmodified kernel text addresses?
The call from arm64_print_text_symbols() is for "bt -[tT]", which just
scours a kernel stack for text addresses and dumps them. So presumably
that needs to apply the mask to each stack value as you've done in
arm64_print_stackframe_entry()? (while still recognizing unmodified text
addresses found in exception frames)
I added PAC mask changes in most of the is_kernel_text function except
arm64_get_stack_frame as it is the first function so PC should not have
any PAC details. Thanks for your detailed suggestion. It was helpful in
fixing the missing implementation.
Thanks,
Amit Daniel
Thanks,
Dave
----- Original Message -----
This value is used to mask the PAC bits and generate correct backtrace.
(amit.kachhap@xxxxxxx)
---
The kernel version for the corresponding vmcoreinfo entry is posted here[1].
[1]: https://lore.kernel.org/patchwork/patch/1211981/
arm64.c | 20 ++++++++++++++++++++
defs.h | 1 +
2 files changed, 21 insertions(+)
diff --git a/arm64.c b/arm64.c
index 09b1b76..55e084f 100644
--- a/arm64.c
+++ b/arm64.c
@@ -84,6 +84,7 @@ static int arm64_get_kvaddr_ranges(struct vaddr_range *);
static void arm64_get_crash_notes(void);
static void arm64_calc_VA_BITS(void);
static int arm64_is_uvaddr(ulong, struct task_context *);
+static void arm64_calc_KERNELPACMASK(void);
/*
@@ -213,6 +214,7 @@ arm64_init(int when)
machdep->pagemask = ~((ulonglong)machdep->pageoffset);
arm64_calc_VA_BITS();
+ arm64_calc_KERNELPACMASK();
ms = machdep->machspec;
if (ms->VA_BITS_ACTUAL) {
ms->page_offset = ARM64_PAGE_OFFSET_ACTUAL;
@@ -472,6 +474,7 @@ arm64_init(int when)
case LOG_ONLY:
machdep->machspec = &arm64_machine_specific;
arm64_calc_VA_BITS();
+ arm64_calc_KERNELPACMASK();
arm64_calc_phys_offset();
machdep->machspec->page_offset = ARM64_PAGE_OFFSET;
break;
@@ -1925,6 +1928,7 @@ arm64_print_stackframe_entry(struct bt_info *bt, int
level, struct arm64_stackfr
struct syment *sp;
struct load_module *lm;
char buf[BUFSIZE];
+ struct machine_specific *ms = machdep->machspec;
/*
* if pc comes from a saved lr, it actually points to an instruction
@@ -1932,6 +1936,9 @@ arm64_print_stackframe_entry(struct bt_info *bt, int
level, struct arm64_stackfr
* See, for example, "bl schedule" before ret_to_user().
*/
branch_pc = frame->pc - 4;
+ if (ms->CONFIG_ARM64_KERNELPACMASK)
+ branch_pc |= ms->CONFIG_ARM64_KERNELPACMASK;
+
name = closest_symbol(branch_pc);
name_plus_offset = NULL;
@@ -4070,6 +4077,19 @@ arm64_swp_offset(ulong pte)
return pte;
}
+static void arm64_calc_KERNELPACMASK(void)
+{
+ ulong value;
+ char *string;
+
+ if ((string = pc->read_vmcoreinfo("NUMBER(KERNELPACMASK)"))) {
+ value = htol(string, QUIET, NULL);
+ free(string);
+ machdep->machspec->CONFIG_ARM64_KERNELPACMASK = value;
+ }
+ fprintf(fp, " got NUMBER(KERNELPACMASK) =%llx\n", value);
+}
+
#endif /* ARM64 */
diff --git a/defs.h b/defs.h
index a3f828d..f37a957 100644
--- a/defs.h
+++ b/defs.h
@@ -3269,6 +3269,7 @@ struct machine_specific {
ulong machine_kexec_end;
ulong VA_BITS_ACTUAL;
ulong CONFIG_ARM64_VA_BITS;
+ ulong CONFIG_ARM64_KERNELPACMASK;
ulong VA_START;
};
--
2.7.4
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
