On 2023/05/23 23:03, lijiang wrote:
> On Thu, May 11, 2023 at 12:35 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx>
> wrote:
>
>> Support:
>> - sym -l
>> - sym -M
>> - sym -m module
>>
>> Signed-off-by: Kazuhito Hagio <k-hagio-ab@xxxxxxx>
>> ---
>> 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_".
I see, but there has been no problem for a long time and its change
needs to change the old pseudo symbols too, so I would like to leave
this as it is for now.
>
> # 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.
Oh thanks, just copied from symbol_dump().
>
>
>> + 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.
hmm, so can we change the enums to macros for now?
#define MOD_TEXT 0
#define MOD_DATA 1
...
I don't see enum's good point here in crash.
>
>
>> + 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?
copied from symbol_dump(), and removed in 07/15.
https://listman.redhat.com/archives/crash-utility/2023-May/010649.html
>
>
>> + 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)
Indeed.
>
>
>> + 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.
Yes, how about this?
struct module_tag {
char *start;
char *end;
char *start_str;
char *end_str;
};
#define MODULE_TAG(type, suffix) ("_MODULE_" #type "_" #suffix)
#define MODULE_STR(type, suffix) ( "MODULE " #type " " #suffix)
#define MODULE_TAGS(type) { \
.start = MODULE_TAG(type, START), \
.end = MODULE_TAG(type, END), \
.start_str = MODULE_STR(type, START), \
.end_str = MODULE_STR(type, END) \
}
static const struct module_tag module_tag[] = {
MODULE_TAGS(TEXT),
MODULE_TAGS(DATA),
MODULE_TAGS(RODATA),
MODULE_TAGS(RO_AFTER_INIT),
MODULE_TAGS(INIT_TEXT),
MODULE_TAGS(INIT_DATA),
MODULE_TAGS(INIT_RODATA),
};
>
> /*
>> * 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.
will fix.
>
>
>> + 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.
Yes, will remove the insmod related codes.
>
>
>> + 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?
To return the previous sp and offset, when value is lower than the
current sp.
>
> + 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].
Sounds good.
>
>
>> 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;
>
ok, will try.
>
>
>> - 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.
ok, will remove it.
Thanks,
Kazu
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
