On Thu, May 11, 2023 at 12:35 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@nec.com> wrote:
Support:
- sym -l
- sym -M
- sym -m module

Signed-off-by: Kazuhito Hagio <k-hagio-ab@nec.com>
---
 symbols.c | 234 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 225 insertions(+), 9 deletions(-)

diff --git a/symbols.c b/symbols.c
index 88849833bada..669fa2e2f3da 100644
--- a/symbols.c
+++ b/symbols.c
@@ -103,9 +103,16 @@ static void free_structure(struct struct_elem *);
 static unsigned char is_right_brace(const char *);
 static struct struct_elem *find_node(struct struct_elem *, char *);
 static void dump_node(struct struct_elem *, char *, unsigned char, unsigned char);
-static int _in_module(ulong, struct load_module *, int);
+
+static int module_mem_type(ulong, struct load_module *);
 static ulong module_mem_end(ulong, struct load_module *);
+static int _in_module(ulong, struct load_module *, int);
+struct syment *value_search_module_v2(ulong, ulong *);

+static const char *module_start_tags[];
+static const char *module_start_strs[];
+static const char *module_end_tags[];
+static const char *module_end_strs[];

 /*
  *  structure/union printing stuff
@@ -1270,10 +1277,14 @@ symname_hash_search(struct syment *table[], char *name)
  *  Output for sym -[lL] command.
  */

+#define MODULE_PSEUDO_SYMBOL(sp)       (STRNEQ((sp)->name, "_MODULE_"))
+

Good understanding. But I'm concerned if the name "_MODULE_" is too short to distinguish kernel symbols from (pseudo)module symbols, although I do not see any kernel symbols with the prefix "_MODULE_".

# objdump -t vmlinux|grep _MODULE_

And in crash-utility, most of the time it uses the strncmp()/strcmp() to compare pseudo symbols, but sometimes it also uses strstr() to search for the pseudo symbols. Maybe the following symbols may not be loaded by crash-utility, only some strings.
# objdump -s vmlinux |grep _MODULE_
 015a20 52494659 494e475f 4d4f4455 4c455f53  RIFYING_MODULE_S
 03ee70 574e5f4d 4f44554c 455f5349 474e4154  WN_MODULE_SIGNAT
 03f180 444f574e 5f4d4f44 554c455f 50415241  DOWN_MODULE_PARA
 1bb530 45544854 4f4f4c5f 4d4f4455 4c455f50  ETHTOOL_MODULE_P
 1bba20 4f4f4c5f 4d4f4455 4c455f50 4f574552  OOL_MODULE_POWER
 1bba80 54455f4d 4f44554c 455f434d 49535f4e  TE_MODULE_CMIS_N
...

+/*
 #define MODULE_PSEUDO_SYMBOL(sp) \
     ((STRNEQ((sp)->name, "_MODULE_START_") || STRNEQ((sp)->name, "_MODULE_END_")) || \
     (STRNEQ((sp)->name, "_MODULE_INIT_START_") || STRNEQ((sp)->name, "_MODULE_INIT_END_")) || \
     (STRNEQ((sp)->name, "_MODULE_SECTION_")))
+*/

 #define MODULE_START(sp) (STRNEQ((sp)->name, "_MODULE_START_"))
 #define MODULE_END(sp)   (STRNEQ((sp)->name, "_MODULE_END_"))
@@ -1282,6 +1293,93 @@ symname_hash_search(struct syment *table[], char *name)
 #define MODULE_SECTION_START(sp) (STRNEQ((sp)->name, "_MODULE_SECTION_START"))
 #define MODULE_SECTION_END(sp)   (STRNEQ((sp)->name, "_MODULE_SECTION_END"))

+#define MODULE_MEM_START(sp,i) (STRNEQ((sp)->name, module_start_tags[i]))
+#define MODULE_MEM_END(sp,i)   (STRNEQ((sp)->name, module_end_tags[i]))
+
+/* For 6.4 and later */
+static void
+module_symbol_dump(char *module)
+{
+       int i, j, start, percpu_syms;
+        struct syment *sp, *sp_end;

Indent issues.
 
+       struct load_module *lm;
+       const char *p1, *p2;
+
+#define TBD  1
+#define DISPLAYED 2
+
+       for (i = 0; i < st->mods_installed; i++) {
+
+               lm = &st->load_modules[i];
+               if (module && !STREQ(module, lm->mod_name))
+                       continue;
+
+               if (received_SIGINT() || output_closed())
+                       return;
+

The variable "j" should be defined as enum mod_mem_type.
 
+               for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; j++) {
+
+                       if (!lm->symtable[j])
+                               continue;
+
+                       sp = lm->symtable[j];
+                       sp_end = lm->symend[j];
+                       percpu_syms = 0;
+
 
The variable start is set to FALSE, looks strange, why not just set it to zero?
 
+                       for (start = FALSE; sp <= sp_end; sp++) {
+                               /* TODO
+                               if (IN_MODULE_PERCPU(sp->value, lm)) {
+                                       if (percpu_syms == DISPLAYED)
+                                               continue;
+                                       if (!start) {
+                                               percpu_syms = TBD;
+                                               continue;
+                                       }
+                                       dump_percpu_symbols(lm);
+                                       percpu_syms = DISPLAYED;
+                               }
+                               */
+                               if (MODULE_PSEUDO_SYMBOL(sp)) {
+                                       if (MODULE_SECTION_START(sp)) {
+                                               p1 = sp->name + strlen("_MODULE_SECTION_START ");
+                                               p2 = "section start";
+                                       } else if (MODULE_SECTION_END(sp)) {
+                                               p1 = sp->name + strlen("_MODULE_SECTION_END ");
+                                               p2 = "section end";
+                                       } else if (STRNEQ(sp->name, module_start_tags[j])) {

MODULE_MEM_START(sp, i) 

+                                               p1 = module_start_strs[j];
+                                               p2 = sp->name + strlen(module_start_tags[j]);
+                                               start = TRUE;
+                                       } else if (STRNEQ(sp->name, module_end_tags[j])) {

MODULE_MEM_END(sp, i)
 
+                                               p1 = module_end_strs[j];
+                                               p2 = sp->name + strlen(module_end_tags[j]);
+                                               /* TODO
+                                               if (MODULE_PERCPU_SYMS_LOADED(lm) &&
+                                                   !percpu_syms) {
+                                                       dump_percpu_symbols(lm);
+                                                       percpu_syms = DISPLAYED;
+                                               }
+                                               */
+                                       } else {
+                                               p1 = "unknown tag";
+                                               p2 = sp->name;
+                                       }
+
+                                       fprintf(fp, "%lx %s: %s\n", sp->value, p1, p2);
+
+                                       /* TODO
+                                       if (percpu_syms == TBD) {
+                                               dump_percpu_symbols(lm);
+                                               percpu_syms = DISPLAYED;
+                                       }
+                                       */
+                               } else
+                                       show_symbol(sp, 0, SHOW_RADIX());
+                       }
+               }
+       }
+}
+
 static void
 symbol_dump(ulong flags, char *module)
 {
@@ -1304,6 +1402,11 @@ symbol_dump(ulong flags, char *module)
        if (!(flags & MODULE_SYMS))
                return;

+       if (MODULE_MEMORY()) { /* 6.4 and later */
+               module_symbol_dump(module);
+               return;
+       }
+
        for (i = 0; i < st->mods_installed; i++) {

                lm = &st->load_modules[i];
@@ -1808,6 +1911,24 @@ static const char *module_end_tags[] = {
        "_MODULE_INIT_DATA_END_",
        "_MODULE_INIT_RODATA_END_"
 };
+static const char *module_start_strs[] = {
+       "MODULE TEXT START",
+       "MODULE DATA START",
+       "MODULE RODATA START",
+       "MODULE RO_AFTER_INIT START",
+       "MODULE INIT_TEXT START",
+       "MODULE INIT_DATA START",
+       "MODULE INIT_RODATA START"
+};
+static const char *module_end_strs[] = {
+       "MODULE TEXT END",
+       "MODULE DATA END",
+       "MODULE RODATA END",
+       "MODULE RO_AFTER_INIT END",
+       "MODULE INIT_TEXT END",
+       "MODULE INIT_DATA END",
+       "MODULE INIT_RODATA END"
+};


As I mentioned in the [PATCH 01/15], similarly the above strings are in pairs, so they can be defined as one array or macro substitution.

 /*
  * Linux 6.4 introduced module.mem memory layout
@@ -5278,6 +5399,85 @@ module_symbol(ulong value,
        return FALSE;
 }

+/* Linux 6.4 and later */
+struct syment *
+value_search_module_v2(ulong value, ulong *offset)
+{
+       int i, j;
+        struct syment *sp, *sp_end, *spnext, *splast;

Indent issues. 
 
+       struct load_module *lm;
+
+        for (i = 0; i < st->mods_installed; i++) {
+                lm = &st->load_modules[i];
+
+               if (!IN_MODULE(value, lm) && !IN_MODULE_INIT(value, lm))
+                       continue;
+
 
The variable "j" should be defined as enum mod_mem_type.
 
+               for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; j++) {
+                       sp = lm->symtable[j];
+                       sp_end = lm->symend[j];
+
+                       if (value < sp->value)
+                               continue;
+
+                       splast = NULL;
+                       for ( ; sp <= sp_end; sp++) {
+                               /* TODO
+                               if (machine_type("ARM64") &&
+                                   IN_MODULE_PERCPU(sp->value, lm) &&
+                                   !IN_MODULE_PERCPU(value, lm))
+                                       continue;
+                               */
+

Currently, I'm still building the kernel for aarch64, will check it next time.

+                               if (value == sp->value) {
+                                       if (MODULE_MEM_END(sp, j))
+                                               break;
+
+                                       if (MODULE_PSEUDO_SYMBOL(sp)) {
+                                               spnext = sp + 1;
+                                               if (MODULE_PSEUDO_SYMBOL(spnext))
+                                                       continue;
+                                               if (spnext->value == value)
+                                                       sp = spnext;
+                                       }
+                                       /* TODO: probably not needed anymore? */

It's hard to track the change log for an old kernel and crash-utility. But for the kernel 6.4 and later, it could be safe to drop it, crash won't go into this code path.
 
+                                       if (is_insmod_builtin(lm, sp)) {
+                                               spnext = sp+1;
+                                               if ((spnext < sp_end) &&
+                                                   (value == spnext->value))
+                                                       sp = spnext;
+                                       }
+                                       if (sp->name[0] == '.') {
+                                               spnext = sp+1;
+                                               if (spnext->value == value)
+                                                       sp = spnext;
+                                       }
+                                       if (offset)
+                                               *offset = 0;
+                                       return sp;
+                               }
+
+                               if (sp->value > value) {
+                                       sp = splast ? splast : sp - 1;
+                                       if (offset)
+                                               *offset = value - sp->value;
+                                       return sp;
+                               }
+

Why is it needed? The splast will also store the "__insmod_"-type symbols?

+                               if (!MODULE_PSEUDO_SYMBOL(sp)) {
+                                       if (is_insmod_builtin(lm, sp)) {
+                                               if (!splast || (sp->value > splast->value))
+                                                       splast = sp;
+                                       } else
+                                               splast = sp;
+                               }
+                       }
+               }
+       }
+
+       return NULL;
+}
+
 struct syment *
 value_search_module(ulong value, ulong *offset)
 {
@@ -5286,6 +5486,9 @@ value_search_module(ulong value, ulong *offset)
        struct load_module *lm;
        int search_init_sections, search_init;

+       if (MODULE_MEMORY()) /* Linux 6.4 and later */
+               return value_search_module_v2(value, offset);
+
 
Here, the suffix _v2 also looks fine to me, but if it can be aligned with the kernel version suffix, that will keep the same code style as the [patch 01/15].
 
        search_init = FALSE;
        search_init_sections = 0;

@@ -13958,19 +14161,32 @@ _in_module(ulong addr, struct load_module *lm, int type)
        return ((addr >= base) && (addr < (base + size)));
 }

-/* Returns the end address of the module memory region. */
-static ulong
-module_mem_end(ulong addr, struct load_module *lm)
+/* Returns module memory type, otherwise MOD_INVALID(-1) */
+static int
+module_mem_type(ulong addr, struct load_module *lm)
 {
-       ulong base, end;
+       ulong base;
        int i;

The variable "i" should be defined as enum mod_mem_type, the compiler will be helpful to check for any potential errors(warnings). 

+
        for (i = MOD_TEXT; i < MOD_MEM_NUM_TYPES; i++) {
                base = lm->mem[i].base;
                if (!base)
                        continue;

Kernel usually tends to check the lm->mem[i].size instead of lm->mem[i].base, for example:

ulong size;
...
size = lm->mem[i].size;
if (!size)
    continue;

 
-               end = base + lm->mem[i].size;
-               if ((addr >= base) && (addr < end))
-                       return end;
+               if ((addr >= base) && (addr < base + lm->mem[i].size))
+                       return i;

base =  lm->mem[i].base;
if ((addr >= base) && (addr < base + size))

 
        }
-       return 0;
+
+       return MOD_INVALID;
+}
+
+/* Returns the end address of the module memory region. */
+static ulong
+module_mem_end(ulong addr, struct load_module *lm)
+{
+       int type = module_mem_type(addr, lm);
+

The module_mem_type() will ensure that the type does not exceed the range of type, just like the code comment. So no need to check for the condition "type >= MOD_MEM_NUM_TYPES" as below.
 
Thanks
Lianbo

+       if (type == MOD_INVALID || type >= MOD_MEM_NUM_TYPES)
+               return 0;
+
+       return lm->mem[type].base + lm->mem[type].size;
 }
--
2.31.1