On Wed, Feb 8, 2023 at 8:54 AM <crash-utility-request@redhat.com> wrote:
Date: Tue, 7 Feb 2023 14:02:27 +0000
From: "Aureau, Georges (Kernel Tools ERT)" <georges.aureau@hpe.com>
To: HAGIO KAZUHITO(?????)  <k-hagio-ab@nec.com>, "Discussion list for
        crash utility usage, maintenance and development"
        <crash-utility@redhat.com>
Subject: Re: [Crash-utility] kmem -s/-S not working properly on
        RHEL8.6/8.7
Message-ID:
        <SJ0PR84MB1482D6E7A2540ED4640603A69FDB9@SJ0PR84MB1482.NAMPRD84.PROD.OUTLOOK.COM>

Content-Type: text/plain; charset="utf-8"

Hello Kazu,

Thanks tons for your valuable comments, below is round #2.

With respect to the RHEL8 crash patch, it consisted of simply removing the 'if (THIS_KERNEL_VERSION >= LINUX(5,7,0))' from freelist_ptr(), suited for RHEL8.6/RHEL8.7, but obviously not for upstream.

You are right, Georges.
Usually I would recommend using the upstream crash tool to analyze upstream kernel vmcore. Correspondingly, RHEL crash tool analyzes the RHEL vmcore.

But anyway, this patch is a good improvement for the situation. I like the idea of searching for the bswap instruction from the asm code.
 
Cheers,
Georges
--
diff --git a/defs.h b/defs.h
index 33a823b..56d6cf4 100644
--- a/defs.h
+++ b/defs.h
@@ -2638,6 +2638,7 @@ struct vm_table {                /* kernel VM-related data */
 #define SLAB_OVERLOAD_PAGE    (0x8000000)
 #define SLAB_CPU_CACHE       (0x10000000)
 #define SLAB_ROOT_CACHES     (0x20000000)
+#define FREELIST_PTR_BSWAP   (0x40000000)

 
I would suggest  adding the prefix "SLAB_" for the  macro definition "FREELIST_PTR_BSWAP".

 #define IS_FLATMEM()           (vt->flags & FLATMEM)
 #define IS_DISCONTIGMEM()      (vt->flags & DISCONTIGMEM)
diff --git a/memory.c b/memory.c
index 5141fbe..b235b7c 100644
--- a/memory.c
+++ b/memory.c
@@ -320,6 +320,7 @@ static void dump_per_cpu_offsets(void);
 static void dump_page_flags(ulonglong);
 static ulong kmem_cache_nodelists(ulong);
 static void dump_hstates(void);
+static void freelist_ptr_init(void);
 static ulong freelist_ptr(struct meminfo *, ulong, ulong);
 static ulong handle_each_vm_area(struct handle_each_vm_area_args *);

@@ -370,7 +371,6 @@ mem_init(void)
         DISPLAY_DEFAULT = (sizeof(long) == 8) ? DISPLAY_64 : DISPLAY_32;
 }

-
 /*
  *  Stash a few popular offsets and some basic kernel virtual memory
  *  items used by routines in this file.
@@ -789,6 +789,8 @@ vm_init(void)
                MEMBER_OFFSET_INIT(kmem_cache_name, "kmem_cache", "name");
                MEMBER_OFFSET_INIT(kmem_cache_flags, "kmem_cache", "flags");
                MEMBER_OFFSET_INIT(kmem_cache_random, "kmem_cache", "random");
+               if (VALID_MEMBER(kmem_cache_random))
+                       freelist_ptr_init();
                MEMBER_OFFSET_INIT(kmem_cache_cpu_freelist, "kmem_cache_cpu", "freelist");
                MEMBER_OFFSET_INIT(kmem_cache_cpu_page, "kmem_cache_cpu", "page");
                if (INVALID_MEMBER(kmem_cache_cpu_page))
@@ -19519,13 +19521,65 @@ count_free_objects(struct meminfo *si, ulong freelist)
        return c;
 }

+/*
+ * With CONFIG_SLAB_FREELIST_HARDENED, freelist_ptr's are crypted with xor's,
+ * and for recent release with an additionnal bswap. Some releases prio to 5.7.0
+ * may be using the additionnal bswap. The only easy and reliable way to tell is
+ * to inspect assembly code (eg. "__slab_free") for a bswap instruction.
+ */
+static int
+freelist_ptr_bswap_x86(void)
+{
+       struct syment *sp, *spn;
+       char buf1[BUFSIZE];
+       char buf2[BUFSIZE];
+       char *arglist[MAXARGS];
+       ulong vaddr;
+       int found;
+
+       if (!(sp = symbol_search("__slab_free")) ||
+           !(spn = next_symbol(NULL, sp)))
+               return FALSE;
+
+       sprintf(buf1, "x/%ldi 0x%lx", spn->value - sp->value, sp->value);

The number of instructions disassembled by gdb is: spn->value - sp->value, which is much more than actually needed.

Can you please replace it with this one?
+ sprintf(buf1, "disassemble 0x%lx, 0x%lx", sp->value, spn->value);
 
+
+       found = FALSE;
+       vaddr = 0;
+       open_tmpfile();
+       gdb_pass_through(buf1, pc->tmpfile, GNU_RETURN_ON_ERROR);
+       rewind(pc->tmpfile);
+       while (fgets(buf2, BUFSIZE, pc->tmpfile)) {

With the above change, need to add it as below:

+               if (STRNEQ(buf, "Dump of assembler code"))
+                       continue;

Thanks.
Lianbo
 
+               if (parse_line(buf2, arglist) < 3)
+                       continue;
+
+               if ((vaddr = htol(strip_ending_char(arglist[0], ':'),
+                   RETURN_ON_ERROR|QUIET, NULL)) >= spn->value)
+                       continue;
+
+               if (STREQ(arglist[2], "bswap")) {
+                       found = TRUE;
+                       break;
+               }
+       }
+       close_tmpfile();
+       return found;
+}
+
+static void
+freelist_ptr_init(void)
+{
+       if (THIS_KERNEL_VERSION >= LINUX(5,7,0) ||
+           ((machine_type("X86_64") || machine_type("X86")) && freelist_ptr_bswap_x86()))
+               vt->flags |= FREELIST_PTR_BSWAP;
+}
+
 static ulong
 freelist_ptr(struct meminfo *si, ulong ptr, ulong ptr_addr)
 {
        if (VALID_MEMBER(kmem_cache_random)) {
                /* CONFIG_SLAB_FREELIST_HARDENED */

-               if (THIS_KERNEL_VERSION >= LINUX(5,7,0))
+               if (vt->flags & FREELIST_PTR_BSWAP)
                        ptr_addr = (sizeof(long) == 8) ? bswap_64(ptr_addr)
                                                       : bswap_32(ptr_addr);
                return (ptr ^ si->random ^ ptr_addr);

------------------------------