Re: [PATCH v4 16/16] arm64: Add gdb stack unwinding support
by lijiang
On Mon, Jun 3, 2024 at 10:15 AM <devel-request(a)lists.crash-utility.osci.io>
wrote:
> Date: Fri, 31 May 2024 17:19:39 +0800
> From: Tao Liu <ltao(a)redhat.com>
> Subject: [Crash-utility] [PATCH v4 16/16] arm64: Add gdb stack
> unwinding support
> To: devel(a)lists.crash-utility.osci.io
> Cc: Mahesh J Salgaonkar <mahesh(a)linux.ibm.com>, "Naveen N . Rao"
> <naveen.n.rao(a)linux.vnet.ibm.com>, Lianbo Jiang <
> lijiang(a)redhat.com>,
> Alexey Makhalov <alexey.makhalov(a)broadcom.com>
> Message-ID: <20240531091939.97828-17-ltao(a)redhat.com>
> Content-Type: text/plain; charset=UTF-8
>
> Cc: Sourabh Jain <sourabhjain(a)linux.ibm.com>
> Cc: Hari Bathini <hbathini(a)linux.ibm.com>
> Cc: Mahesh J Salgaonkar <mahesh(a)linux.ibm.com>
> Cc: Naveen N. Rao <naveen.n.rao(a)linux.vnet.ibm.com>
> Cc: Lianbo Jiang <lijiang(a)redhat.com>
> Cc: HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com>
> Cc: Tao Liu <ltao(a)redhat.com>
> Cc: Alexey Makhalov <alexey.makhalov(a)broadcom.com>
> Signed-off-by: Tao Liu <ltao(a)redhat.com>
> ---
> arm64.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
> defs.h | 36 ++++++++++++++++++
> 2 files changed, 145 insertions(+), 5 deletions(-)
>
> diff --git a/arm64.c b/arm64.c
> index af0e0d7..5f9be82 100644
> --- a/arm64.c
> +++ b/arm64.c
> @@ -116,6 +116,23 @@ static void arm64_get_struct_page_size(struct
> machine_specific *ms);
> #define mte_tag_reset(addr) (((ulong)addr &
> ~mte_tag_shifted(KASAN_TAG_KERNEL)) | \
> mte_tag_shifted(KASAN_TAG_KERNEL))
>
> +struct user_regs_bitmap_struct {
> + struct {
> + union {
> + struct arm64_user_pt_regs user_regs;
> + struct {
> + u64 regs[31];
> + u64 sp;
> + u64 pc;
> + u64 pstate;
> + };
> + };
> + u64 orig_x0;
> + u64 syscallno;
> + };
> + ulong bitmap;
> +};
> +
> static inline bool is_mte_kvaddr(ulong addr)
> {
> /* check for ARM64_MTE enabled */
> @@ -174,6 +191,85 @@ arm64_vmemmap_is_page_ptr(ulong addr, physaddr_t
> *phys)
> return FALSE;
> }
>
> +static int
> +arm64_get_current_task_reg(int regno, const char *name,
> + int size, void *value)
> +{
> + struct bt_info bt_info, bt_setup;
> + struct task_context *tc;
> + struct user_regs_bitmap_struct *ur_bitmap;
> + ulong ip, sp;
> + bool ret = FALSE;
> +
> + switch (regno) {
> + case X0_REGNUM ... PC_REGNUM:
> + break;
> + default:
> + return FALSE;
> + }
> +
>
The above switch-case code block can be defined as a separate static
function to do the sanity check, as mentioned in another thread.
Please refer to the comment here:
Link:
https://www.mail-archive.com/devel@lists.crash-utility.osci.io/msg00816.html
Hmm, basically the current patch is similar to the code design of [patch v4
08/16], so I won't comment on the following code one by one, can you try to
improve this one based on the previous comments? (See the above link).
Thanks.
Lianbo
+ tc = CURRENT_CONTEXT();
> + if (!tc)
> + return FALSE;
> + BZERO(&bt_setup, sizeof(struct bt_info));
> + clone_bt_info(&bt_setup, &bt_info, tc);
> + fill_stackbuf(&bt_info);
> +
> + get_dumpfile_regs(&bt_info, &sp, &ip);
> + if (bt_info.stackbuf)
> + FREEBUF(bt_info.stackbuf);
> + ur_bitmap = (struct user_regs_bitmap_struct *)bt_info.machdep;
> + if (!ur_bitmap)
> + return FALSE;
> +
> + if (!bt_info.need_free) {
> + goto get_all;
> + }
> + switch (ur_bitmap->bitmap) {
> + case 0x1a0000000:
> + switch (regno) {
> + case X29_REGNUM:
> + case SP_REGNUM:
> + case PC_REGNUM:
> + break;
> + default:
> + return FALSE;
> + }
> + break;
> + }
> +
> +get_all:
> + switch (regno) {
> + case X0_REGNUM ... X30_REGNUM:
> + if (size != sizeof(ur_bitmap->regs[regno])) {
> + ret = FALSE; break;
> + } else {
> + memcpy(value, &ur_bitmap->regs[regno], size);
> + ret = TRUE; break;
> + }
> + case SP_REGNUM:
> + if (size != sizeof(ur_bitmap->sp)) {
> + ret = FALSE; break;
> + } else {
> + memcpy(value, &ur_bitmap->sp, size);
> + ret = TRUE; break;
> + }
> + case PC_REGNUM:
> + if (size != sizeof(ur_bitmap->pc)) {
> + ret = FALSE; break;
> + } else {
> + memcpy(value, &ur_bitmap->pc, size);
> + ret = TRUE; break;
> + }
> + }
> +
> + if (bt_info.need_free) {
> + FREEBUF(ur_bitmap);
> + bt_info.need_free = FALSE;
> + }
> + return ret;
> +}
> +
> /*
> * Do all necessary machine-specific setup here. This is called several
> times
> * during initialization.
> @@ -472,6 +568,7 @@ arm64_init(int when)
> machdep->dumpfile_init = NULL;
> machdep->verify_line_number = NULL;
> machdep->init_kernel_pgd = arm64_init_kernel_pgd;
> + machdep->get_current_task_reg = arm64_get_current_task_reg;
>
> /* use machdep parameters */
> arm64_calc_phys_offset();
> @@ -3785,6 +3882,7 @@ arm64_get_dumpfile_stackframe(struct bt_info *bt,
> struct arm64_stackframe *frame
> try_kernel:
> frame->sp = ptregs->sp;
> frame->fp = ptregs->regs[29];
> + bt->machdep = ptregs;
> }
>
> if (arm64_in_kdump_text(bt, frame) ||
> @@ -3814,21 +3912,27 @@ static void
> arm64_get_stack_frame(struct bt_info *bt, ulong *pcp, ulong *spp)
> {
> int ret;
> + struct user_regs_bitmap_struct *ur_bitmap;
> struct arm64_stackframe stackframe = { 0 };
>
> if (DUMPFILE() && is_task_active(bt->task)) {
> ret = arm64_get_dumpfile_stackframe(bt, &stackframe);
> + bt->need_free = FALSE;
> } else {
> if (bt->flags & BT_SKIP_IDLE)
> bt->flags &= ~BT_SKIP_IDLE;
>
> ret = arm64_get_stackframe(bt, &stackframe);
> - }
>
> - if (!ret)
> - error(WARNING,
> - "cannot determine starting stack frame for task
> %lx\n",
> - bt->task);
> + ur_bitmap = (struct user_regs_bitmap_struct
> *)GETBUF(sizeof(*ur_bitmap));
> + memset(ur_bitmap, 0, sizeof(*ur_bitmap));
> + ur_bitmap->pc = stackframe.pc;
> + ur_bitmap->sp = stackframe.sp;
> + ur_bitmap->regs[29] = stackframe.fp;
> + ur_bitmap->bitmap += 0x1a0000000;
> + bt->machdep = ur_bitmap;
> + bt->need_free = TRUE;
> + }
>
> bt->frameptr = stackframe.fp;
> if (pcp)
> diff --git a/defs.h b/defs.h
> index fd00462..05d8e14 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -8073,6 +8073,42 @@ enum x86_64_regnum {
> LAST_REGNUM
> };
>
> +enum arm64_regnum {
> + X0_REGNUM,
> + X1_REGNUM,
> + X2_REGNUM,
> + X3_REGNUM,
> + X4_REGNUM,
> + X5_REGNUM,
> + X6_REGNUM,
> + X7_REGNUM,
> + X8_REGNUM,
> + X9_REGNUM,
> + X10_REGNUM,
> + X11_REGNUM,
> + X12_REGNUM,
> + X13_REGNUM,
> + X14_REGNUM,
> + X15_REGNUM,
> + X16_REGNUM,
> + X17_REGNUM,
> + X18_REGNUM,
> + X19_REGNUM,
> + X20_REGNUM,
> + X21_REGNUM,
> + X22_REGNUM,
> + X23_REGNUM,
> + X24_REGNUM,
> + X25_REGNUM,
> + X26_REGNUM,
> + X27_REGNUM,
> + X28_REGNUM,
> + X29_REGNUM,
> + X30_REGNUM,
> + SP_REGNUM,
> + PC_REGNUM,
> +};
> +
> /*
> * Register numbers to make crash_target->fetch_registers()
> * ---> machdep->get_current_task_reg() work properly.
> --
> 2.40.1
>
3 months
[PATCH] list: fatal if -r isn't used in conjunction with -H or -h
by Li Zhijian
Per the code, -r(LIST_HEAD_REVERSE) only work with LIST_HEAD_FORMAT
which is set by -H or -h.
Previously, if LIST_HEAD_FORMAT was not set, `list -r` will traverse the
list in order, that doesn't obey the -r(reverse) semantics.
Add a further check to ensure -r is used in conjunction with -H or -h.
Signed-off-by: Li Zhijian <lizhijian(a)fujitsu.com>
---
help.c | 3 ++-
tools.c | 3 +++
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/help.c b/help.c
index d80e843703c1..6f7e093cbef1 100644
--- a/help.c
+++ b/help.c
@@ -5977,7 +5977,8 @@ char *help__list[] = {
" ",
" -x Override the default output format with hexadecimal format.",
" -d Override the default output format with decimal format.",
-" -r For a list linked with list_head structures, traverse the list",
+" -r Must be used in conjunction with either -H or -h.",
+" For a list linked with list_head structures, traverse the list",
" in the reverse order by using the \"prev\" pointer instead",
" of \"next\".",
" -B Use the algorithm from R. P. Brent to detect loops instead of",
diff --git a/tools.c b/tools.c
index 0f2db108838a..67977605c276 100644
--- a/tools.c
+++ b/tools.c
@@ -3451,6 +3451,9 @@ cmd_list(void)
}
}
+ if (ld->flags & LIST_HEAD_REVERSE && !(ld->flags & LIST_HEAD_FORMAT))
+ error(FATAL, "-r must be used in conjunction with -H or -h\n");
+
if (argerrs)
cmd_usage(pc->curcmd, SYNOPSIS);
--
2.29.2
3 months
Re: [PATCH v4 12/16] x86_64: Fix invalid input "=>" for bt command
by lijiang
On Fri, May 31, 2024 at 5:38 PM <devel-request(a)lists.crash-utility.osci.io>
wrote:
> Date: Fri, 31 May 2024 17:19:35 +0800
> From: Tao Liu <ltao(a)redhat.com>
> Subject: [Crash-utility] [PATCH v4 12/16] x86_64: Fix invalid input
> "=>" for bt command
> To: devel(a)lists.crash-utility.osci.io
> Cc: Mahesh J Salgaonkar <mahesh(a)linux.ibm.com>, "Naveen N . Rao"
> <naveen.n.rao(a)linux.vnet.ibm.com>, Lianbo Jiang <
> lijiang(a)redhat.com>,
> Alexey Makhalov <alexey.makhalov(a)broadcom.com>
> Message-ID: <20240531091939.97828-13-ltao(a)redhat.com>
> Content-Type: text/plain; charset=UTF-8
>
> There may be extra "=>" prefix before gdb disassembly, as a result,
> parse_line() will return string "=>" as arglist[0], which will be
> converted to number by htol() and fails. E.g.:
>
> crash> gdb x/40i __list_del_entry
> ...
> 0xffffffff8133c384 <__list_del_entry+36>: cmp %rcx,%rax
> 0xffffffff8133c387 <__list_del_entry+39>: je 0xffffffff8133c403
> <__list_del_entry+163>
> => 0xffffffff8133c389 <__list_del_entry+41>: mov (%rax),%r8
> 0xffffffff8133c38c <__list_del_entry+44>: cmp %r8,%rdi
> 0xffffffff8133c38f <__list_del_entry+47>: jne 0xffffffff8133c3e4
> <__list_del_entry+132>
> 0xffffffff8133c391 <__list_del_entry+49>: mov 0x8(%rdx),%r8
>
> Before the patch:
>
> crash> bt
> ...
> #10 [ffff880095647c00] async_page_fault at ffffffff816a8638
> [exception RIP: __list_del_entry+41]
> RIP: ffffffff8133c389 RSP: ffff880095647cb0 RFLAGS: 00010207
> RAX: 0000000000000000 RBX: ffffea0400408020 RCX: dead000000000200
> RDX: 0000000000000000 RSI: 0000000000000246 RDI: ffffea0400408020
> RBP: ffff880095647cb0 R8: 0000000080000431 R9: ffffffff81e835c0
> R10: 0000000000000000 R11: 0000000000000400 R12: ffff880138795b58
> R13: 0000000010010201 R14: ffff880095647d70 R15: 0000000400408040
> ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
> bt: invalid input: "=>"
> #11 [ffff880095647cb8] list_del at ffffffff8133c43d
> #12 [ffff880095647cd0] devm_memremap_pages at ffffffff81180c53
>
> After the patch:
>
> No string as 'bt: invalid input: "=>"' of output.
>
> Cc: Sourabh Jain <sourabhjain(a)linux.ibm.com>
> Cc: Hari Bathini <hbathini(a)linux.ibm.com>
> Cc: Mahesh J Salgaonkar <mahesh(a)linux.ibm.com>
> Cc: Naveen N. Rao <naveen.n.rao(a)linux.vnet.ibm.com>
> Cc: Lianbo Jiang <lijiang(a)redhat.com>
> Cc: HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com>
> Cc: Tao Liu <ltao(a)redhat.com>
> Cc: Alexey Makhalov <alexey.makhalov(a)broadcom.com>
> Signed-off-by: Tao Liu <ltao(a)redhat.com>
> ---
> x86_64.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/x86_64.c b/x86_64.c
> index 54c69fd..47c215f 100644
> --- a/x86_64.c
> +++ b/x86_64.c
> @@ -8829,6 +8829,8 @@ x86_64_get_framesize(struct bt_info *bt, ulong
> textaddr, ulong rsp, char *stack_
>
> rewind(pc->tmpfile2);
> while (fgets(buf, BUFSIZE, pc->tmpfile2)) {
> + if (STRNEQ(buf, "=>"))
> + shift_string_left(buf, 2);
> strcpy(buf2, buf);
>
>
This looks good.
Thanks
Lianbo
> if (CRASHDEBUG(3))
> --
> 2.40.1
>
3 months
[PATCH] Fix "kmem -i" and "swap" commands on Linux 6.10-rc1 and later kernels
by HAGIO KAZUHITO(萩尾 一仁)
Kernel commit 798cb7f9aec3 ("swapon(2)/swapoff(2): don't bother with
block size") removed swap_info_struct.old_block_size member at Linux
6.10-rc1. The crash-utility has used this to determine whether a swap
is a partition or file and to determine the way to get the swap path.
Withtout the patch, the "kmem -i" and "swap" commands fail with the
following error messsage:
crash> kmem -i
...
TOTAL HUGE 13179392 50.3 GB ----
HUGE FREE 13179392 50.3 GB 100% of TOTAL HUGE
swap: invalid (optional) structure member offsets: swap_info_struct_swap_device or swap_info_struct_old_block_size
FILE: memory.c LINE: 16032 FUNCTION: dump_swap_info()
The swap_file member of recent swap_info_struct is a pointer to a
struct file (once upon a time it was dentry), use this fact directly.
Signed-off-by: Kazuhito Hagio <k-hagio-ab(a)nec.com>
---
NOTE: This patch can be applied on top of my '[PATCH] Fix "kmem -v"
option on Linux 6.9 and later kernels' patch. Otherwise, please adjust
the hunk for offset_table.
defs.h | 1 +
filesys.c | 1 +
memory.c | 28 +++++++++++++++++++++++-----
symbols.c | 1 +
4 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/defs.h b/defs.h
index 95de33188070..64bf785b7a1b 100644
--- a/defs.h
+++ b/defs.h
@@ -2242,6 +2242,7 @@ struct offset_table { /* stash of commonly-used offsets */
long log_caller_id;
long vmap_node_busy;
long rb_list_head;
+ long file_f_inode;
};
struct size_table { /* stash of commonly-used sizes */
diff --git a/filesys.c b/filesys.c
index 81fe856699e1..406ebb299780 100644
--- a/filesys.c
+++ b/filesys.c
@@ -2038,6 +2038,7 @@ vfs_init(void)
MEMBER_OFFSET_INIT(file_f_dentry, "file", "f_dentry");
MEMBER_OFFSET_INIT(file_f_vfsmnt, "file", "f_vfsmnt");
MEMBER_OFFSET_INIT(file_f_count, "file", "f_count");
+ MEMBER_OFFSET_INIT(file_f_inode, "file", "f_inode");
MEMBER_OFFSET_INIT(path_mnt, "path", "mnt");
MEMBER_OFFSET_INIT(path_dentry, "path", "dentry");
if (INVALID_MEMBER(file_f_dentry)) {
diff --git a/memory.c b/memory.c
index acb8507cfb75..28f901f16adf 100644
--- a/memory.c
+++ b/memory.c
@@ -16075,6 +16075,8 @@ dump_swap_info(ulong swapflags, ulong *totalswap_pages, ulong *totalused_pages)
char buf3[BUFSIZE];
char buf4[BUFSIZE];
char buf5[BUFSIZE];
+ int swap_file_is_file =
+ STREQ(MEMBER_TYPE_NAME("swap_info_struct", "swap_file"), "file");
if (!symbol_exists("nr_swapfiles"))
error(FATAL, "nr_swapfiles doesn't exist in this kernel!\n");
@@ -16118,9 +16120,21 @@ dump_swap_info(ulong swapflags, ulong *totalswap_pages, ulong *totalused_pages)
swap_file = ULONG(vt->swap_info_struct +
OFFSET(swap_info_struct_swap_file));
- swap_device = INT(vt->swap_info_struct +
- OFFSET_OPTION(swap_info_struct_swap_device,
- swap_info_struct_old_block_size));
+ /* Linux 6.10 and later */
+ if (INVALID_MEMBER(swap_info_struct_swap_device) &&
+ INVALID_MEMBER(swap_info_struct_old_block_size) &&
+ swap_file_is_file) {
+ ulong inode;
+ ushort mode;
+ readmem(swap_file + OFFSET(file_f_inode), KVADDR, &inode,
+ sizeof(ulong), "swap_file.f_inode", FAULT_ON_ERROR);
+ readmem(inode + OFFSET(inode_i_mode), KVADDR, &mode,
+ sizeof(ushort), "inode.i_mode", FAULT_ON_ERROR);
+ swap_device = S_ISBLK(mode);
+ } else
+ swap_device = INT(vt->swap_info_struct +
+ OFFSET_OPTION(swap_info_struct_swap_device,
+ swap_info_struct_old_block_size));
pages = INT(vt->swap_info_struct +
OFFSET(swap_info_struct_pages));
@@ -16161,8 +16175,12 @@ dump_swap_info(ulong swapflags, ulong *totalswap_pages, ulong *totalused_pages)
OFFSET(swap_info_struct_swap_vfsmnt));
get_pathname(swap_file, buf, BUFSIZE,
1, vfsmnt);
- } else if (VALID_MEMBER
- (swap_info_struct_old_block_size)) {
+ } else if (VALID_MEMBER(swap_info_struct_old_block_size) ||
+ swap_file_is_file) {
+ /*
+ * Linux 6.10 and later kernels do not have old_block_size,
+ * but this still should work, if swap_file is file.
+ */
devname = vfsmount_devname(file_to_vfsmnt(swap_file),
buf1, BUFSIZE);
get_pathname(file_to_dentry(swap_file),
diff --git a/symbols.c b/symbols.c
index 283b98a8fbfe..0bf050ab62d0 100644
--- a/symbols.c
+++ b/symbols.c
@@ -10522,6 +10522,7 @@ dump_offset_table(char *spec, ulong makestruct)
OFFSET(file_f_count));
fprintf(fp, " file_f_path: %ld\n",
OFFSET(file_f_path));
+ fprintf(fp, " file_f_inode: %ld\n", OFFSET(file_f_inode));
fprintf(fp, " path_mnt: %ld\n",
OFFSET(path_mnt));
fprintf(fp, " path_dentry: %ld\n",
--
2.31.1
3 months
Re: [PATCH v4 02/16] Leave only one gdb thread for crash
by Lianbo Jiang
On 5/31/24 5:21 PM, devel-request(a)lists.crash-utility.osci.io wrote:
> Date: Fri, 31 May 2024 17:19:25 +0800
> From: Tao Liu<ltao(a)redhat.com>
> Subject: [Crash-utility] [PATCH v4 02/16] Leave only one gdb thread
> for crash
> To:devel@lists.crash-utility.osci.io
> Cc: Alexey Makhalov<alexey.makhalov(a)broadcom.com>, Mahesh J
> Salgaonkar<mahesh(a)linux.ibm.com>, "Naveen N . Rao"
> <naveen.n.rao(a)linux.vnet.ibm.com>, Lianbo Jiang<lijiang(a)redhat.com>
> Message-ID:<20240531091939.97828-3-ltao(a)redhat.com>
> Content-Type: text/plain; charset=UTF-8
>
> This patch is a preparation of gdb stack unwinding support.
> There are 3 designs for supporting arbitrary tasks stack unwinding:
>
> 1) One gdb thread represent a task[1][2].
> 2) One gdb thread represent a cpu[3].
> 3) Leaving only one gdb thread[4].
>
> 1 & 2 have a flaw that, when there are lots of tasks/cpus, it will slow
> the startup of crash, introduce complexity of the registers context
> synchronization between crash and gdb, hard to cover live debug mode
> etc. So here we used the 3rd design.
>
> To switch task, or view arbitrary tasks stack unwinding, we will reuse
> the current gdb thread, and load the target task's regcache to the
> thread(see the next patch). This will simplify many code.
>
> [1]:https://www.mail-archive.com/devel@lists.crash-utility.osci.io/msg0052...
> [2]:https://www.mail-archive.com/devel@lists.crash-utility.osci.io/msg0052...
> [3]:https://www.mail-archive.com/devel@lists.crash-utility.osci.io/msg0047...
> [4]:https://www.mail-archive.com/devel@lists.crash-utility.osci.io/msg0054...
>
> Co-developed-by: Aditya Gupta<adityag(a)linux.ibm.com>
> Co-developed-by: Alexey Makhalov<alexey.makhalov(a)broadcom.com>
> Co-developed-by: Tao Liu<ltao(a)redhat.com>
> Cc: Sourabh Jain<sourabhjain(a)linux.ibm.com>
> Cc: Hari Bathini<hbathini(a)linux.ibm.com>
> Cc: Mahesh J Salgaonkar<mahesh(a)linux.ibm.com>
> Cc: Naveen N. Rao<naveen.n.rao(a)linux.vnet.ibm.com>
> Cc: Lianbo Jiang<lijiang(a)redhat.com>
> Cc: HAGIO KAZUHITO(萩尾 一仁)<k-hagio-ab(a)nec.com>
> Cc: Tao Liu<ltao(a)redhat.com>
> Cc: Alexey Makhalov<alexey.makhalov(a)broadcom.com>
> Signed-off-by: Tao Liu<ltao(a)redhat.com>
> ---
> crash_target.c | 14 +++++---------
> gdb_interface.c | 19 -------------------
> 2 files changed, 5 insertions(+), 28 deletions(-)
>
> diff --git a/crash_target.c b/crash_target.c
> index 4554806..1f62bf6 100644
> --- a/crash_target.c
> +++ b/crash_target.c
> @@ -26,7 +26,6 @@
> void crash_target_init (void);
>
> extern "C" int gdb_readmem_callback(unsigned long, void *, int, int);
> -extern "C" int crash_get_nr_cpus(void);
> extern "C" int crash_get_cpu_reg (int cpu, int regno, const char *regname,
> int regsize, void *val);
>
> @@ -110,7 +109,6 @@ crash_target::xfer_partial (enum target_object object, const char *annex,
> void
> crash_target_init (void)
> {
> - int nr_cpus = crash_get_nr_cpus();
> crash_target *target = new crash_target ();
>
> /* Own the target until it is successfully pushed. */
> @@ -119,13 +117,11 @@ crash_target_init (void)
> push_target (std::move (target_holder));
>
> inferior_appeared (current_inferior (), CRASH_INFERIOR_PID);
> - for (int i = 0; i < nr_cpus; i++)
> - {
> - thread_info *thread = add_thread_silent (target,
> - ptid_t(CRASH_INFERIOR_PID, 0, i));
> - if (!i)
> - switch_to_thread (thread);
> - }
> +
> + /*Only create 1 gdb threads to view tasks' stack unwinding*/
> + thread_info *thread = add_thread_silent (target,
> + ptid_t(CRASH_INFERIOR_PID, 0, 0));
> + switch_to_thread (thread);
>
> /* Fetch all registers from core file. */
> target_fetch_registers (get_current_regcache (), -1);
> diff --git a/gdb_interface.c b/gdb_interface.c
> index 8f99a0d..ab1bd52 100644
> --- a/gdb_interface.c
> +++ b/gdb_interface.c
> @@ -1067,25 +1067,6 @@ unsigned long crash_get_kaslr_offset(void)
> }
>
> /* Callbacks for crash_target */
> -int crash_get_nr_cpus(void);
> -int crash_get_cpu_reg (int cpu, int regno, const char *regname,
> - int regsize, void *val);
> -
Removing the above declaration will trigger a warning:
cc -c -g -DX86_64 -DLZO -DGDB_10_2 gdb_interface.c -Wall -O2
-Wstrict-prototypes -Wmissing-prototypes -fstack-protector
-Wformat-security
gdb_interface.c:1070:5: warning: no previous prototype for
‘crash_get_cpu_reg’ [-Wmissing-prototypes]
1070 | int crash_get_cpu_reg (int cpu, int regno, const char *regname,
| ^~~~~~~~~~~~~~~~~
Thanks
Lianbo
> -int crash_get_nr_cpus(void)
> -{
> - if (SADUMP_DUMPFILE())
> - return sadump_get_nr_cpus();
> - else if (DISKDUMP_DUMPFILE())
> - return diskdump_get_nr_cpus();
> - else if (KDUMP_DUMPFILE())
> - return kdump_get_nr_cpus();
> - else if (VMSS_DUMPFILE())
> - return vmware_vmss_get_nr_cpus();
> -
> - /* Just CPU #0 */
> - return 1;
> -}
> -
> int crash_get_cpu_reg (int cpu, int regno, const char *regname,
> int regsize, void *value)
> {
> -- 2.40.1
3 months, 1 week
Re: [PATCH v4 03/16] Let crash change gdb context
by Lianbo Jiang
On 5/31/24 5:22 PM, devel-request(a)lists.crash-utility.osci.io wrote:
> Date: Fri, 31 May 2024 17:19:26 +0800
> From: Tao Liu<ltao(a)redhat.com>
> Subject: [Crash-utility] [PATCH v4 03/16] Let crash change gdb context
> To:devel@lists.crash-utility.osci.io
> Cc: Alexey Makhalov<alexey.makhalov(a)broadcom.com>, Mahesh J
> Salgaonkar<mahesh(a)linux.ibm.com>, "Naveen N . Rao"
> <naveen.n.rao(a)linux.vnet.ibm.com>, Lianbo Jiang<lijiang(a)redhat.com>
> Message-ID:<20240531091939.97828-4-ltao(a)redhat.com>
> Content-Type: text/plain; charset=UTF-8
>
> This patch is a preparation of gdb stack unwinding support. "set" command
> is extended with gdb context change:
>
> crash> set <pid>
> or
> crash> set <task>
>
> Then the task context of crash and gdb will both be switched to
> pid/task, and the following command: "bt" "gdb bt" will output
> the same task context.
>
> Co-developed-by: Aditya Gupta<adityag(a)linux.ibm.com>
> Co-developed-by: Alexey Makhalov<alexey.makhalov(a)broadcom.com>
> Co-developed-by: Tao Liu<ltao(a)redhat.com>
> Cc: Sourabh Jain<sourabhjain(a)linux.ibm.com>
> Cc: Hari Bathini<hbathini(a)linux.ibm.com>
> Cc: Mahesh J Salgaonkar<mahesh(a)linux.ibm.com>
> Cc: Naveen N. Rao<naveen.n.rao(a)linux.vnet.ibm.com>
> Cc: Lianbo Jiang<lijiang(a)redhat.com>
> Cc: HAGIO KAZUHITO(萩尾 一仁)<k-hagio-ab(a)nec.com>
> Cc: Tao Liu<ltao(a)redhat.com>
> Cc: Alexey Makhalov<alexey.makhalov(a)broadcom.com>
> Signed-off-by: Tao Liu<ltao(a)redhat.com>
> ---
> crash_target.c | 20 +++++++++++++++++---
> defs.h | 5 ++++-
> kernel.c | 11 ++++++++---
> task.c | 21 +++++++++++++--------
> tools.c | 8 ++++----
> 5 files changed, 46 insertions(+), 19 deletions(-)
>
> diff --git a/crash_target.c b/crash_target.c
> index 1f62bf6..03718b5 100644
> --- a/crash_target.c
> +++ b/crash_target.c
> @@ -28,7 +28,7 @@ void crash_target_init (void);
> extern "C" int gdb_readmem_callback(unsigned long, void *, int, int);
> extern "C" int crash_get_cpu_reg (int cpu, int regno, const char *regname,
> int regsize, void *val);
> -
> +extern "C" int gdb_change_thread_context ();
>
> /* The crash target. */
>
> @@ -63,16 +63,22 @@ public:
>
> };
>
> -/* We just get all the registers, so we don't use regno. */
> void
> crash_target::fetch_registers (struct regcache *regcache, int regno)
> {
> + int r;
> gdb_byte regval[16];
> int cpu = inferior_ptid.tid();
> struct gdbarch *arch = regcache->arch ();
>
> - for (int r = 0; r < gdbarch_num_regs (arch); r++)
> + if (regno >= 0) {
> + r = regno;
> + goto onetime;
> + }
> +
> + for (r = 0; regno == -1 && r < gdbarch_num_regs (arch); r++)
> {
> +onetime:
> const char *regname = gdbarch_register_name(arch, r);
> int regsize = register_size (arch, r);
> if (regsize > sizeof (regval))
> @@ -129,3 +135,11 @@ crash_target_init (void)
> /* Now, set up the frame cache. */
> reinit_frame_cache ();
> }
This function looks weird to me. Can you try to refactor this one?
For example:
+static void supply_registers(struct regcache *regcache, int regno)
+{
+ gdb_byte regval[16];
+ struct gdbarch *arch = regcache->arch ();
+ const char *regname = gdbarch_register_name(arch, regno);
+ int regsize = register_size(arch, regno);
+
+ if (regsize > sizeof (regval))
+ error (_("fatal error: buffer size is not enough to fit
register value"));
+
+ if (crash_get_current_task_reg (regno, regname, regsize, (void
*)®val))
+ regcache->raw_supply (regno, regval);
+ else
+ regcache->raw_supply (regno, NULL);
+}
void
crash_target::fetch_registers (struct regcache *regcache, int regno)
{
int r;
if (regno >= 0) {
supply_registers(regcache, regno);
return;
}
for (r = 0; regno == -1 && r < gdbarch_num_regs (regcache->arch ()); r++)
supply_registers(regcache, r);
}
That can avoid jumping into a for-loop code block with goto.
And It looks more friendly and readable. Just an idea, What do you think?
Thanks
Lianbo
> +
> +extern "C" int
> +gdb_change_thread_context ()
> +{
> + target_fetch_registers(get_current_regcache(), -1);
> + reinit_frame_cache();
> + return TRUE;
> +}
> diff --git a/defs.h b/defs.h
> index 3cb8e63..0d872b2 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -6013,7 +6013,7 @@ extern char *help_map[];
> * task.c
> */
> void task_init(void);
> -int set_context(ulong, ulong);
> +int set_context(ulong, ulong, uint);
> void show_context(struct task_context *);
> ulong pid_to_task(ulong);
> ulong task_to_pid(ulong);
> @@ -8051,4 +8051,7 @@ enum x86_64_regnum {
> LAST_REGNUM
> };
>
> +/* crash_target.c */
> +extern int gdb_change_thread_context ();
> +
> #endif /* !GDB_COMMON */
> diff --git a/kernel.c b/kernel.c
> index 1728b70..78b7b1e 100644
> --- a/kernel.c
> +++ b/kernel.c
> @@ -6494,15 +6494,20 @@ set_cpu(int cpu)
> if (hide_offline_cpu(cpu))
> error(FATAL, "invalid cpu number: cpu %d is OFFLINE\n", cpu);
>
> - if ((task = get_active_task(cpu)))
> - set_context(task, NO_PID);
> + task = get_active_task(cpu);
> +
> + /* Check if context is already set to given cpu */
> + if (task == CURRENT_TASK())
> + return;
> +
> + if (task)
> + set_context(task, NO_PID, TRUE);
> else
> error(FATAL, "cannot determine active task on cpu %ld\n", cpu);
>
> show_context(CURRENT_CONTEXT());
> }
>
> -
> /*
> * Collect the irq_desc[] entry along with its associated handler and
> * action structures.
> diff --git a/task.c b/task.c
> index ebdb5be..d47d268 100644
> --- a/task.c
> +++ b/task.c
> @@ -672,7 +672,7 @@ task_init(void)
> if (ACTIVE()) {
> active_pid = REMOTE() ? pc->server_pid :
> LOCAL_ACTIVE() ? pc->program_pid : 1;
> - set_context(NO_TASK, active_pid);
> + set_context(NO_TASK, active_pid, FALSE);
> tt->this_task = pid_to_task(active_pid);
> }
> else {
> @@ -684,7 +684,7 @@ task_init(void)
> else if (ELF_NOTES_VALID() && DISKDUMP_DUMPFILE())
> map_cpus_to_prstatus_kdump_cmprs();
> please_wait("determining panic task");
> - set_context(get_panic_context(), NO_PID);
> + set_context(get_panic_context(), NO_PID, TRUE);
> please_wait_done();
> }
>
> @@ -2985,9 +2985,9 @@ refresh_context(ulong curtask, ulong curpid)
> struct task_context *tc;
>
> if (task_exists(curtask) && pid_exists(curpid)) {
> - set_context(curtask, NO_PID);
> + set_context(curtask, NO_PID, FALSE);
> } else {
> - set_context(tt->this_task, NO_PID);
> + set_context(tt->this_task, NO_PID, FALSE);
>
> complain = TRUE;
> if (STREQ(args[0], "set") && (argcnt == 2) &&
> @@ -3053,7 +3053,7 @@ sort_context_array(void)
> curtask = CURRENT_TASK();
> qsort((void *)tt->context_array, (size_t)tt->running_tasks,
> sizeof(struct task_context), sort_by_pid);
> - set_context(curtask, NO_PID);
> + set_context(curtask, NO_PID, TRUE);
>
> sort_context_by_task();
> }
> @@ -3100,7 +3100,7 @@ sort_context_array_by_last_run(void)
> curtask = CURRENT_TASK();
> qsort((void *)tt->context_array, (size_t)tt->running_tasks,
> sizeof(struct task_context), sort_by_last_run);
> - set_context(curtask, NO_PID);
> + set_context(curtask, NO_PID, TRUE);
>
> sort_context_by_task();
> }
> @@ -5281,7 +5281,7 @@ comm_exists(char *s)
> * that pid is selected.
> */
> int
> -set_context(ulong task, ulong pid)
> +set_context(ulong task, ulong pid, uint update_gdb_thread)
> {
> int i;
> struct task_context *tc;
> @@ -5301,7 +5301,12 @@ set_context(ulong task, ulong pid)
>
> if (found) {
> CURRENT_CONTEXT() = tc;
> - return TRUE;
> +
> + /* change the selected thread in gdb, according to current context */
> + if (update_gdb_thread)
> + return gdb_change_thread_context();
> + else
> + return TRUE;
> } else {
> if (task)
> error(INFO, "cannot set context for task: %lx\n", task);
> diff --git a/tools.c b/tools.c
> index 0f2db10..80d4244 100644
> --- a/tools.c
> +++ b/tools.c
> @@ -1871,7 +1871,7 @@ cmd_set(void)
> return;
>
> if (ACTIVE()) {
> - set_context(tt->this_task, NO_PID);
> + set_context(tt->this_task, NO_PID, TRUE);
> show_context(CURRENT_CONTEXT());
> return;
> }
> @@ -1880,7 +1880,7 @@ cmd_set(void)
> error(INFO, "no panic task found!\n");
> return;
> }
> - set_context(tt->panic_task, NO_PID);
> + set_context(tt->panic_task, NO_PID, TRUE);
> show_context(CURRENT_CONTEXT());
> return;
>
> @@ -2559,14 +2559,14 @@ cmd_set(void)
> case STR_PID:
> pid = value;
> task = NO_TASK;
> - if (set_context(task, pid))
> + if (set_context(task, pid, TRUE))
> show_context(CURRENT_CONTEXT());
> break;
>
> case STR_TASK:
> task = value;
> pid = NO_PID;
> - if (set_context(task, pid))
> + if (set_context(task, pid, TRUE))
> show_context(CURRENT_CONTEXT());
> break;
>
> -- 2.40.1
3 months, 1 week
[PATCH v4 00/16] gdb stack unwinding support for crash utility
by Tao Liu
This patchset is a rebase/merged version of the following 3 patchsets:
1): [PATCH v10 0/5] Improve stack unwind on ppc64 [1]
2): [PATCH 0/5] x86_64 gdb stack unwinding support [2]
3): Clean up on top of one-thread-v2 [3]
A complete description of gdb stack unwinding support for crash can be
found in [1].
This patchset can be divided into the following 2 parts:
1) part1: arch independent, mainly modify on the
crash_target.c/gdb_interface.c files, in preparation of the
gdb side.
2) part2: arch specific part, for implementing ppc64/x86_64/arm64/vmware
gdb stack unwinding support.
=== part 2
- arm64:
arm64: Add gdb stack unwinding support
- vmware:
vmware_guestdump: Various format versions support
x86_64: fix gdb bt for vmware dumps
set_context(): check if context is already current
- x86_64:
x86_64: Fix invalid input "=>" for bt command
Fix cpumask_t recursive dependence issue
Parse stack by inactive_stack_frame priorily if the struct is valid
x86_64: Add gdb stack unwinding support
- ppc64:
ppc64: correct gdb passthroughs by implementing machdep->get_cpu_reg
=== part 1
Stop stack unwinding at non-kernel address
Fix gdb_interface: restore gdb's output streams at end of gdb_interface
Print task pid/command instead of CPU index
Rename get_cpu_reg to get_current_task_reg
Let crash change gdb context
Leave only one gdb thread for crash
Remove 'frame' from prohibited commands list
===
v4 -> v3:
Fixed the author issue in [PATCH v3 06/16] Fix gdb_interface: restore gdb's
output streams at end of gdb_interface.
v3 -> v2:
1) Updated CC list as pointed out in [4]
2) Compiling issues as in [5]
v2 -> v1:
1) Added the patch: x86_64: Fix invalid input "=>" for bt command,
thanks for Kazu's testing.
2) Modify the patch: x86_64: Add gdb stack unwinding support, added the
pcp_save, spp_save and sp, for restoring the value in match of the original
code logic.
[1]: https://www.mail-archive.com/devel@lists.crash-utility.osci.io/msg00469.html
[2]: https://www.mail-archive.com/devel@lists.crash-utility.osci.io/msg00488.html
[3]: https://www.mail-archive.com/devel@lists.crash-utility.osci.io/msg00554.html
[4]: https://www.mail-archive.com/devel@lists.crash-utility.osci.io/msg00681.html
[5]: https://www.mail-archive.com/devel@lists.crash-utility.osci.io/msg00715.html
Aditya Gupta (2):
Remove 'frame' from prohibited commands list
ppc64: correct gdb passthroughs by implementing machdep->get_cpu_reg
Alexey Makhalov (3):
set_context(): check if context is already current
x86_64: fix gdb bt for vmware dumps
vmware_guestdump: Various format versions support
Tao Liu (11):
Leave only one gdb thread for crash
Let crash change gdb context
Rename get_cpu_reg to get_current_task_reg
Print task pid/command instead of CPU index
Fix gdb_interface: restore gdb's output streams at end of
gdb_interface
Stop stack unwinding at non-kernel address
x86_64: Add gdb stack unwinding support
Parse stack by inactive_stack_frame priorily if the struct is valid
Fix cpumask_t recursive dependence issue
x86_64: Fix invalid input "=>" for bt command
arm64: Add gdb stack unwinding support
arm64.c | 114 +++++++++++++++-
crash_target.c | 47 ++++---
defs.h | 187 ++++++++++++++++++++++++++-
gdb-10.2.patch | 79 ++++++++++++
gdb_interface.c | 33 ++---
kernel.c | 61 +++++++--
ppc64.c | 163 ++++++++++++++++++++++-
task.c | 33 +++--
tools.c | 8 +-
vmware_guestdump.c | 316 ++++++++++++++++++++++++++++++++-------------
x86_64.c | 302 ++++++++++++++++++++++++++++++++++++++-----
11 files changed, 1151 insertions(+), 192 deletions(-)
--
2.40.1
3 months, 1 week
Re: PATCH] Fix "kmem -i" and "swap" commands on Linux 6.10-rc1 and later kernels
by lijiang
On Wed, Jun 12, 2024 at 2:08 PM <devel-request(a)lists.crash-utility.osci.io>
wrote:
> Date: Wed, 12 Jun 2024 06:06:35 +0000
> From: HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com>
> Subject: [Crash-utility] Re: PATCH] Fix "kmem -i" and "swap" commands
> on Linux 6.10-rc1 and later kernels
> To: lijiang <lijiang(a)redhat.com>, "devel(a)lists.crash-utility.osci.io"
> <devel(a)lists.crash-utility.osci.io>
> Message-ID: <2db40a61-d404-4733-a62a-6068f2911346(a)nec.com>
> Content-Type: text/plain; charset="utf-8"
>
> On 2024/06/12 13:55, lijiang wrote:
> > Thank you for the fix, Kazu.
> >
> > On Tue, Jun 11, 2024 at 10:42 AM <
> devel-request(a)lists.crash-utility.osci.io <mailto:
> devel-request(a)lists.crash-utility.osci.io>> wrote:
> >
> > Date: Tue, 11 Jun 2024 02:40:55 +0000
> > From: HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com <mailto:
> k-hagio-ab(a)nec.com>>
> > Subject: [Crash-utility] [PATCH] Fix "kmem -i" and "swap" commands on
> > Linux 6.10-rc1 and later kernels
> > To: "devel(a)lists.crash-utility.osci.io <mailto:
> devel(a)lists.crash-utility.osci.io>"
> > <devel(a)lists.crash-utility.osci.io <mailto:
> devel(a)lists.crash-utility.osci.io>>
> > Message-ID: <1718073652-13444-1-git-send-email-k-hagio-ab(a)nec.com
> <mailto:1718073652-13444-1-git-send-email-k-hagio-ab@nec.com>>
> > Content-Type: text/plain; charset="iso-2022-jp"
> >
> > Kernel commit 798cb7f9aec3 ("swapon(2)/swapoff(2): don't bother with
> > block size") removed swap_info_struct.old_block_size member at Linux
> > 6.10-rc1. The crash-utility has used this to determine whether a
> swap
> > is a partition or file and to determine the way to get the swap path.
> >
> > Withtout the patch, the "kmem -i" and "swap" commands fail with the
> > following error messsage:
> >
> > crash> kmem -i
> > ...
> > TOTAL HUGE 13179392 50.3 GB ----
> > HUGE FREE 13179392 50.3 GB 100% of TOTAL HUGE
> >
> > swap: invalid (optional) structure member offsets:
> swap_info_struct_swap_device or swap_info_struct_old_block_size
> > FILE: memory.c LINE: 16032 FUNCTION: dump_swap_info()
> >
> > The swap_file member of recent swap_info_struct is a pointer to a
> > struct file (once upon a time it was dentry), use this fact directly.
> >
> > Signed-off-by: Kazuhito Hagio <k-hagio-ab(a)nec.com <mailto:
> k-hagio-ab(a)nec.com>>
> > ---
> > NOTE: This patch can be applied on top of my '[PATCH] Fix "kmem -v"
> > option on Linux 6.9 and later kernels' patch. Otherwise, please
> adjust
> > the hunk for offset_table.
> >
> > defs.h | 1 +
> > filesys.c | 1 +
> > memory.c | 28 +++++++++++++++++++++++-----
> > symbols.c | 1 +
> > 4 files changed, 26 insertions(+), 5 deletions(-)
> >
> > diff --git a/defs.h b/defs.h
> > index 95de33188070..64bf785b7a1b 100644
> > --- a/defs.h
> > +++ b/defs.h
> > @@ -2242,6 +2242,7 @@ struct offset_table { /*
> stash of commonly-used offsets */
> > long log_caller_id;
> > long vmap_node_busy;
> > long rb_list_head;
> > + long file_f_inode;
> > };
> >
> > struct size_table { /* stash of commonly-used sizes */
> > diff --git a/filesys.c b/filesys.c
> > index 81fe856699e1..406ebb299780 100644
> > --- a/filesys.c
> > +++ b/filesys.c
> > @@ -2038,6 +2038,7 @@ vfs_init(void)
> > MEMBER_OFFSET_INIT(file_f_dentry, "file", "f_dentry");
> > MEMBER_OFFSET_INIT(file_f_vfsmnt, "file", "f_vfsmnt");
> > MEMBER_OFFSET_INIT(file_f_count, "file", "f_count");
> > + MEMBER_OFFSET_INIT(file_f_inode, "file", "f_inode");
> > MEMBER_OFFSET_INIT(path_mnt, "path", "mnt");
> > MEMBER_OFFSET_INIT(path_dentry, "path", "dentry");
> > if (INVALID_MEMBER(file_f_dentry)) {
> > diff --git a/memory.c b/memory.c
> > index acb8507cfb75..28f901f16adf 100644
> > --- a/memory.c
> > +++ b/memory.c
> > @@ -16075,6 +16075,8 @@ dump_swap_info(ulong swapflags, ulong
> *totalswap_pages, ulong *totalused_pages)
> > char buf3[BUFSIZE];
> > char buf4[BUFSIZE];
> > char buf5[BUFSIZE];
> > + int swap_file_is_file =
> > + STREQ(MEMBER_TYPE_NAME("swap_info_struct",
> "swap_file"), "file");
> >
> >
> > This patch looks good, but I still have one question:
> > The 'swap_file' has been added into the struct swap_info_struct since
> linux v2.6.12-rc2,
> > and until now it(its type) has been never changed, it indicates that the
> above checking is always true.
> >
> > STREQ(MEMBER_TYPE_NAME("swap_info_struct", "swap_file"), "file")
> >
> > Is that expected behavior? Just want to confirm with you.
>
> Yes.
>
Thank you for the confirmation, and explanation below, Kazu.
As far as I checked, swap_file was changed from 'dentry *' to 'file *'
> at somewhere between v2.4.0 and v2.6.0, so not always true.
>
>
Seems no full history in linux github. I checked it from the linux initial
git repo:
commit 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 (tag: v2.6.12-rc2)
Author: Linus Torvalds <torvalds(a)ppc970.osdl.org>
Date: Sat Apr 16 15:20:36 2005 -0700
Linux-2.6.12-rc2
Initial git repository build. I'm not bothering with the full history,
even though we have it. We can create a separate "historical" git
archive of that later if we want to, and in the meantime it's about
3.2GB when imported into git - space that would just make the early
git days unnecessarily complicated, when we don't have a lot of good
infrastructure for it.
Let it rip!
get_pathname()'s first arg is dentry, we have to choose the else-if block
> without old_block_size, so I decided to use its type information directly
>
> } else if
> (VALID_MEMBER(swap_info_struct_old_block_size) ||
> swap_file_is_file) {
>
> The other if / else blocks are for very old kernels, so maybe they won't
> be used, but if we don't remove these, I think this is a reasonable way.
>
>
Sounds good to me.
> Does this answer your question?
>
Yes. Thank you, Kazu. I have no other issues. For the patch: Ack.
Thanks
Lianbo
>
> Thanks,
> Kazu
>
> >
> > Thanks
> > Lianbo
> >
> > if (!symbol_exists("nr_swapfiles"))
> > error(FATAL, "nr_swapfiles doesn't exist in this
> kernel!\n");
> > @@ -16118,9 +16120,21 @@ dump_swap_info(ulong swapflags, ulong
> *totalswap_pages, ulong *totalused_pages)
> > swap_file = ULONG(vt->swap_info_struct +
> > OFFSET(swap_info_struct_swap_file));
> >
> > - swap_device = INT(vt->swap_info_struct +
> > - OFFSET_OPTION(swap_info_struct_swap_device,
> > - swap_info_struct_old_block_size));
> > + /* Linux 6.10 and later */
> > + if (INVALID_MEMBER(swap_info_struct_swap_device) &&
> > + INVALID_MEMBER(swap_info_struct_old_block_size)
> &&
> > + swap_file_is_file) {
> > + ulong inode;
> > + ushort mode;
> > + readmem(swap_file + OFFSET(file_f_inode),
> KVADDR, &inode,
> > + sizeof(ulong), "swap_file.f_inode",
> FAULT_ON_ERROR);
> > + readmem(inode + OFFSET(inode_i_mode),
> KVADDR, &mode,
> > + sizeof(ushort), "inode.i_mode",
> FAULT_ON_ERROR);
> > + swap_device = S_ISBLK(mode);
> > + } else
> > + swap_device = INT(vt->swap_info_struct +
> > +
> OFFSET_OPTION(swap_info_struct_swap_device,
> > + swap_info_struct_old_block_size));
> >
> > pages = INT(vt->swap_info_struct +
> > OFFSET(swap_info_struct_pages));
> > @@ -16161,8 +16175,12 @@ dump_swap_info(ulong swapflags, ulong
> *totalswap_pages, ulong *totalused_pages)
> >
> OFFSET(swap_info_struct_swap_vfsmnt));
> > get_pathname(swap_file, buf,
> BUFSIZE,
> > 1, vfsmnt);
> > - } else if (VALID_MEMBER
> > - (swap_info_struct_old_block_size)) {
> > + } else if
> (VALID_MEMBER(swap_info_struct_old_block_size) ||
> > + swap_file_is_file) {
> > + /*
> > + * Linux 6.10 and later kernels do
> not have old_block_size,
> > + * but this still should work, if
> swap_file is file.
> > + */
> > devname =
> vfsmount_devname(file_to_vfsmnt(swap_file),
> > buf1, BUFSIZE);
> >
> get_pathname(file_to_dentry(swap_file),
> > diff --git a/symbols.c b/symbols.c
> > index 283b98a8fbfe..0bf050ab62d0 100644
> > --- a/symbols.c
> > +++ b/symbols.c
> > @@ -10522,6 +10522,7 @@ dump_offset_table(char *spec, ulong
> makestruct)
> > OFFSET(file_f_count));
> > fprintf(fp, " file_f_path: %ld\n",
> > OFFSET(file_f_path));
> > + fprintf(fp, " file_f_inode: %ld\n",
> OFFSET(file_f_inode));
> > fprintf(fp, " path_mnt: %ld\n",
> > OFFSET(path_mnt));
> > fprintf(fp, " path_dentry: %ld\n",
> > --
> > 2.31.1
> >
>
3 months, 2 weeks
[PATCH] X86 64: fix a regression issue about kernel stack padding
by Lianbo Jiang
The commit 48764a14bc58 may cause a regression issue when the CONFIG_X86_FRED
is not enabled, this is because the SIZE(fred_frame) will call the
SIZE_verify() to determine if the fred_frame is valid, otherwise it will
emit an error:
crash> bt 1
bt: invalid structure size: fred_frame
FILE: x86_64.c LINE: 4089 FUNCTION: x86_64_low_budget_back_trace_cmd()
[/home/k-hagio/bin/crash] error trace: 588df3 => 5cbc72 => 5eb3e1 => 5eb366
PID: 1 TASK: ffff9f94c024b980 CPU: 2 COMMAND: "systemd"
#0 [ffffade44001bca8] __schedule at ffffffffb948ebbb
#1 [ffffade44001bd10] schedule at ffffffffb948f04d
#2 [ffffade44001bd20] schedule_hrtimeout_range_clock at ffffffffb9494fef
#3 [ffffade44001bda8] ep_poll at ffffffffb8c91be8
#4 [ffffade44001be48] do_epoll_wait at ffffffffb8c91d11
#5 [ffffade44001be80] __x64_sys_epoll_wait at ffffffffb8c92590
#6 [ffffade44001bed0] do_syscall_64 at ffffffffb947f459
#7 [ffffade44001bf50] entry_SYSCALL_64_after_hwframe at ffffffffb96000ea
5eb366: SIZE_verify.part.42+70
5eb3e1: SIZE_verify+49
5cbc72: x86_64_low_budget_back_trace_cmd+3010
588df3: back_trace+1523
bt: invalid structure size: fred_frame
FILE: x86_64.c LINE: 4089 FUNCTION: x86_64_low_budget_back_trace_cmd()
Let's replace the SIZE(fred_frame) with the VALID_SIZE(fred_frame) to
fix it.
Fixes: 48764a14bc58 ("x86_64: fix for adding top_of_kernel_stack_padding for kernel stack")
Reported-by: Kazuhito Hagio <k-hagio-ab(a)nec.com>
Signed-off-by: Lianbo Jiang <lijiang(a)redhat.com>
---
x86_64.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/x86_64.c b/x86_64.c
index 6777c93e6b47..469d26b05e24 100644
--- a/x86_64.c
+++ b/x86_64.c
@@ -4086,7 +4086,7 @@ in_exception_stack:
if (!irq_eframe && !is_kernel_thread(bt->tc->task) &&
(GET_STACKBASE(bt->tc->task) == bt->stackbase)) {
- long stack_padding_size = SIZE(fred_frame) > 0 ? (2*8) : 0;
+ long stack_padding_size = VALID_SIZE(fred_frame) ? (2*8) : 0;
user_mode_eframe = bt->stacktop - SIZE(pt_regs);
if (last_process_stack_eframe < user_mode_eframe)
x86_64_exception_frame(EFRAME_PRINT, 0, bt->stackbuf +
@@ -4408,7 +4408,7 @@ in_exception_stack:
if (!irq_eframe && !is_kernel_thread(bt->tc->task) &&
(GET_STACKBASE(bt->tc->task) == bt->stackbase)) {
- long stack_padding_size = SIZE(fred_frame) > 0 ? (2*8) : 0;
+ long stack_padding_size = VALID_SIZE(fred_frame) ? (2*8) : 0;
user_mode_eframe = bt->stacktop - SIZE(pt_regs);
if (last_process_stack_eframe < user_mode_eframe)
x86_64_exception_frame(EFRAME_PRINT, 0, bt->stackbuf +
--
2.45.1
3 months, 2 weeks
Re: PATCH] Fix "kmem -i" and "swap" commands on Linux 6.10-rc1 and later kernels
by lijiang
Thank you for the fix, Kazu.
On Tue, Jun 11, 2024 at 10:42 AM <devel-request(a)lists.crash-utility.osci.io>
wrote:
> Date: Tue, 11 Jun 2024 02:40:55 +0000
> From: HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com>
> Subject: [Crash-utility] [PATCH] Fix "kmem -i" and "swap" commands on
> Linux 6.10-rc1 and later kernels
> To: "devel(a)lists.crash-utility.osci.io"
> <devel(a)lists.crash-utility.osci.io>
> Message-ID: <1718073652-13444-1-git-send-email-k-hagio-ab(a)nec.com>
> Content-Type: text/plain; charset="iso-2022-jp"
>
> Kernel commit 798cb7f9aec3 ("swapon(2)/swapoff(2): don't bother with
> block size") removed swap_info_struct.old_block_size member at Linux
> 6.10-rc1. The crash-utility has used this to determine whether a swap
> is a partition or file and to determine the way to get the swap path.
>
> Withtout the patch, the "kmem -i" and "swap" commands fail with the
> following error messsage:
>
> crash> kmem -i
> ...
> TOTAL HUGE 13179392 50.3 GB ----
> HUGE FREE 13179392 50.3 GB 100% of TOTAL HUGE
>
> swap: invalid (optional) structure member offsets:
> swap_info_struct_swap_device or swap_info_struct_old_block_size
> FILE: memory.c LINE: 16032 FUNCTION: dump_swap_info()
>
> The swap_file member of recent swap_info_struct is a pointer to a
> struct file (once upon a time it was dentry), use this fact directly.
>
> Signed-off-by: Kazuhito Hagio <k-hagio-ab(a)nec.com>
> ---
> NOTE: This patch can be applied on top of my '[PATCH] Fix "kmem -v"
> option on Linux 6.9 and later kernels' patch. Otherwise, please adjust
> the hunk for offset_table.
>
> defs.h | 1 +
> filesys.c | 1 +
> memory.c | 28 +++++++++++++++++++++++-----
> symbols.c | 1 +
> 4 files changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/defs.h b/defs.h
> index 95de33188070..64bf785b7a1b 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -2242,6 +2242,7 @@ struct offset_table { /* stash of
> commonly-used offsets */
> long log_caller_id;
> long vmap_node_busy;
> long rb_list_head;
> + long file_f_inode;
> };
>
> struct size_table { /* stash of commonly-used sizes */
> diff --git a/filesys.c b/filesys.c
> index 81fe856699e1..406ebb299780 100644
> --- a/filesys.c
> +++ b/filesys.c
> @@ -2038,6 +2038,7 @@ vfs_init(void)
> MEMBER_OFFSET_INIT(file_f_dentry, "file", "f_dentry");
> MEMBER_OFFSET_INIT(file_f_vfsmnt, "file", "f_vfsmnt");
> MEMBER_OFFSET_INIT(file_f_count, "file", "f_count");
> + MEMBER_OFFSET_INIT(file_f_inode, "file", "f_inode");
> MEMBER_OFFSET_INIT(path_mnt, "path", "mnt");
> MEMBER_OFFSET_INIT(path_dentry, "path", "dentry");
> if (INVALID_MEMBER(file_f_dentry)) {
> diff --git a/memory.c b/memory.c
> index acb8507cfb75..28f901f16adf 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -16075,6 +16075,8 @@ dump_swap_info(ulong swapflags, ulong
> *totalswap_pages, ulong *totalused_pages)
> char buf3[BUFSIZE];
> char buf4[BUFSIZE];
> char buf5[BUFSIZE];
> + int swap_file_is_file =
> + STREQ(MEMBER_TYPE_NAME("swap_info_struct", "swap_file"),
> "file");
>
>
This patch looks good, but I still have one question:
The 'swap_file' has been added into the struct swap_info_struct since linux
v2.6.12-rc2, and until now it(its type) has been never changed, it
indicates that the above checking is always true.
STREQ(MEMBER_TYPE_NAME("swap_info_struct", "swap_file"), "file")
Is that expected behavior? Just want to confirm with you.
Thanks
Lianbo
> if (!symbol_exists("nr_swapfiles"))
> error(FATAL, "nr_swapfiles doesn't exist in this
> kernel!\n");
> @@ -16118,9 +16120,21 @@ dump_swap_info(ulong swapflags, ulong
> *totalswap_pages, ulong *totalused_pages)
> swap_file = ULONG(vt->swap_info_struct +
> OFFSET(swap_info_struct_swap_file));
>
> - swap_device = INT(vt->swap_info_struct +
> - OFFSET_OPTION(swap_info_struct_swap_device,
> - swap_info_struct_old_block_size));
> + /* Linux 6.10 and later */
> + if (INVALID_MEMBER(swap_info_struct_swap_device) &&
> + INVALID_MEMBER(swap_info_struct_old_block_size) &&
> + swap_file_is_file) {
> + ulong inode;
> + ushort mode;
> + readmem(swap_file + OFFSET(file_f_inode), KVADDR,
> &inode,
> + sizeof(ulong), "swap_file.f_inode",
> FAULT_ON_ERROR);
> + readmem(inode + OFFSET(inode_i_mode), KVADDR,
> &mode,
> + sizeof(ushort), "inode.i_mode",
> FAULT_ON_ERROR);
> + swap_device = S_ISBLK(mode);
> + } else
> + swap_device = INT(vt->swap_info_struct +
> + OFFSET_OPTION(swap_info_struct_swap_device,
> + swap_info_struct_old_block_size));
>
> pages = INT(vt->swap_info_struct +
> OFFSET(swap_info_struct_pages));
> @@ -16161,8 +16175,12 @@ dump_swap_info(ulong swapflags, ulong
> *totalswap_pages, ulong *totalused_pages)
>
> OFFSET(swap_info_struct_swap_vfsmnt));
> get_pathname(swap_file, buf, BUFSIZE,
> 1, vfsmnt);
> - } else if (VALID_MEMBER
> - (swap_info_struct_old_block_size)) {
> + } else if
> (VALID_MEMBER(swap_info_struct_old_block_size) ||
> + swap_file_is_file) {
> + /*
> + * Linux 6.10 and later kernels do not
> have old_block_size,
> + * but this still should work, if
> swap_file is file.
> + */
> devname =
> vfsmount_devname(file_to_vfsmnt(swap_file),
> buf1, BUFSIZE);
> get_pathname(file_to_dentry(swap_file),
> diff --git a/symbols.c b/symbols.c
> index 283b98a8fbfe..0bf050ab62d0 100644
> --- a/symbols.c
> +++ b/symbols.c
> @@ -10522,6 +10522,7 @@ dump_offset_table(char *spec, ulong makestruct)
> OFFSET(file_f_count));
> fprintf(fp, " file_f_path: %ld\n",
> OFFSET(file_f_path));
> + fprintf(fp, " file_f_inode: %ld\n",
> OFFSET(file_f_inode));
> fprintf(fp, " path_mnt: %ld\n",
> OFFSET(path_mnt));
> fprintf(fp, " path_dentry: %ld\n",
> --
> 2.31.1
>
>
3 months, 2 weeks