Hi Mahesh,
This patch is almost good, and there are some cleanup points and
error-handling points.
On Wed, 18 May 2011 01:31:53 +0530
Mahesh J Salgaonkar <mahesh at linux.vnet.ibm.com> wrote:
>
> This patch enables makedumpfile to load module symbol data from vmcore. This
> info is required during kernel module data filtering process. Traverse the
> modules list and load all module's symbol info data in the memory for fast
> lookup.
[..]
> +static int
> +load_module_symbols(void)
> +{
> + unsigned long head, cur, cur_module;
> + unsigned long symtab, strtab;
> + unsigned long mod_base, mod_init;
> + unsigned int mod_size, mod_init_size;
> + unsigned char *module_struct_mem, *module_core_mem;
> + unsigned char *module_init_mem = NULL;
> + unsigned char *symtab_mem;
> + char *module_name, *strtab_mem, *nameptr;
> + struct module_info *modules = NULL;
> + struct symbol_info *sym_info;
> + unsigned int num_symtab;
> + unsigned int i = 0, nsym;
> +
> + head = SYMBOL(modules);
> + if (!get_num_modules(head, &mod_st.num_modules)) {
> + ERRMSG("Can't get module count\n");
> + return FALSE;
> + }
> + if (!mod_st.num_modules) {
> + return FALSE;
If the above num_modules is 0, makedumpfile fails without any hint
message. How about the below change ?
@@ -7651,13 +7651,11 @@ load_module_symbols(void)
unsigned int i = 0, nsym;
head = SYMBOL(modules);
- if (!get_num_modules(head, &mod_st.num_modules)) {
+ if (!get_num_modules(head, &mod_st.num_modules) ||
+ !mod_st.num_modules) {
ERRMSG("Can't get module count\n");
return FALSE;
}
- if (!mod_st.num_modules) {
- return FALSE;
- }
mod_st.modules = calloc(mod_st.num_modules,
sizeof(struct module_info));
if (!mod_st.modules) {
---
[..]
> + /* Travese the list and read module symbols */
> + while (cur != head) {
[..]
> + if (mod_init_size > 0) {
> + module_init_mem = calloc(1, mod_init_size);
> + if (module_init_mem == NULL) {
> + ERRMSG("Can't allocate memory for module "
> + "init\n");
> + return FALSE;
> + }
> + if (!readmem(VADDR, mod_init, module_init_mem,
> + mod_init_size)) {
> + ERRMSG("Can't access module init in memory.\n");
> + return FALSE;
In the above error case, module_init_mem should be freed.
There are the same lacks of free, and I feel it is due to
a large load_module_symbols() function.
Hence I created the attached patch for making the function
small and fixing the lacks of free.
Can you review it ?
> + if (mod_init_size > 0)
> + free(module_init_mem);
> + } while (cur != head);
This is the same as the begining of this loop, and it is not necessary.
Thanks
Ken'ichi Ohmichi
---
diff --git a/makedumpfile.c b/makedumpfile.c
index 3ad2bd5..92ca23b 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -7635,20 +7635,160 @@ clean_module_symbols(void)
}
static int
-load_module_symbols(void)
+__load_module_symbol(struct module_info *modules, unsigned long addr_module)
{
- unsigned long head, cur, cur_module;
+ int ret = FALSE;
+ unsigned int nsym;
unsigned long symtab, strtab;
unsigned long mod_base, mod_init;
unsigned int mod_size, mod_init_size;
- unsigned char *module_struct_mem, *module_core_mem;
+ unsigned char *module_struct_mem = NULL;
+ unsigned char *module_core_mem = NULL;
unsigned char *module_init_mem = NULL;
unsigned char *symtab_mem;
char *module_name, *strtab_mem, *nameptr;
- struct module_info *modules = NULL;
- struct symbol_info *sym_info;
unsigned int num_symtab;
- unsigned int i = 0, nsym;
+
+ /* Allocate buffer to read struct module data from vmcore. */
+ if ((module_struct_mem = calloc(1, SIZE(module))) == NULL) {
+ ERRMSG("Failed to allocate buffer for module\n");
+ return FALSE;
+ }
+ if (!readmem(VADDR, addr_module, module_struct_mem,
+ SIZE(module))) {
+ ERRMSG("Can't get module info.\n");
+ goto out;
+ }
+
+ module_name = (char *)(module_struct_mem + OFFSET(module.name));
+ if (strlen(module_name) < MOD_NAME_LEN)
+ strcpy(modules->name, module_name);
+ else
+ strncpy(modules->name, module_name, MOD_NAME_LEN-1);
+
+ mod_init = ULONG(module_struct_mem +
+ OFFSET(module.module_init));
+ mod_init_size = UINT(module_struct_mem +
+ OFFSET(module.init_size));
+ mod_base = ULONG(module_struct_mem +
+ OFFSET(module.module_core));
+ mod_size = UINT(module_struct_mem +
+ OFFSET(module.core_size));
+
+ DEBUG_MSG("Module: %s, Base: 0x%lx, Size: %u\n",
+ module_name, mod_base, mod_size);
+ if (mod_init_size > 0) {
+ module_init_mem = calloc(1, mod_init_size);
+ if (module_init_mem == NULL) {
+ ERRMSG("Can't allocate memory for module "
+ "init\n");
+ goto out;
+ }
+ if (!readmem(VADDR, mod_init, module_init_mem,
+ mod_init_size)) {
+ ERRMSG("Can't access module init in memory.\n");
+ goto out;
+ }
+ }
+
+ if ((module_core_mem = calloc(1, mod_size)) == NULL) {
+ ERRMSG("Can't allocate memory for module\n");
+ goto out;
+ }
+ if (!readmem(VADDR, mod_base, module_core_mem, mod_size)) {
+ ERRMSG("Can't access module in memory.\n");
+ goto out;
+ }
+
+ num_symtab = UINT(module_struct_mem +
+ OFFSET(module.num_symtab));
+ if (!num_symtab) {
+ ERRMSG("%s: Symbol info not available\n", module_name);
+ goto out;
+ }
+ modules->num_syms = num_symtab;
+ DEBUG_MSG("num_sym: %d\n", num_symtab);
+
+ symtab = ULONG(module_struct_mem + OFFSET(module.symtab));
+ strtab = ULONG(module_struct_mem + OFFSET(module.strtab));
+
+ /* check if symtab and strtab are inside the module space. */
+ if (!IN_RANGE(symtab, mod_base, mod_size) &&
+ !IN_RANGE(symtab, mod_init, mod_init_size)) {
+ ERRMSG("%s: module symtab is outseide of module "
+ "address space\n", module_name);
+ goto out;
+ }
+ if (IN_RANGE(symtab, mod_base, mod_size))
+ symtab_mem = module_core_mem + (symtab - mod_base);
+ else
+ symtab_mem = module_init_mem + (symtab - mod_init);
+
+ if (!IN_RANGE(strtab, mod_base, mod_size) &&
+ !IN_RANGE(strtab, mod_init, mod_init_size)) {
+ ERRMSG("%s: module strtab is outseide of module "
+ "address space\n", module_name);
+ goto out;
+ }
+ if (IN_RANGE(strtab, mod_base, mod_size))
+ strtab_mem = (char *)(module_core_mem
+ + (strtab - mod_base));
+ else
+ strtab_mem = (char *)(module_init_mem
+ + (strtab - mod_init));
+
+ modules->sym_info = calloc(num_symtab, sizeof(struct symbol_info));
+ if (modules->sym_info == NULL) {
+ ERRMSG("Can't allocate memory to store sym info\n");
+ goto out;
+ }
+
+ /* symbols starts from 1 */
+ for (nsym = 1; nsym < num_symtab; nsym++) {
+ Elf32_Sym *sym32;
+ Elf64_Sym *sym64;
+ /* If case of ELF vmcore then the word size can be
+ * determined using info->flag_elf64_memory flag.
+ * But in case of kdump-compressed dump, kdump header
+ * does not carry word size info. May be in future
+ * this info will be available in kdump header.
+ * Until then, in order to make this logic work on both
+ * situation we depend on pointer_size that is
+ * extracted from vmlinux dwarf information.
+ */
+ if ((pointer_size * 8) == 64) {
+ sym64 = (Elf64_Sym *) (symtab_mem
+ + (nsym * sizeof(Elf64_Sym)));
+ modules->sym_info[nsym].value =
+ (unsigned long long) sym64->st_value;
+ nameptr = strtab_mem + sym64->st_name;
+ } else {
+ sym32 = (Elf32_Sym *) (symtab_mem
+ + (nsym * sizeof(Elf32_Sym)));
+ modules->sym_info[nsym].value =
+ (unsigned long long) sym32->st_value;
+ nameptr = strtab_mem + sym32->st_name;
+ }
+ if (strlen(nameptr))
+ modules->sym_info[nsym].name = strdup(nameptr);
+ DEBUG_MSG("\t[%d] %llx %s\n", nsym,
+ modules->sym_info[nsym].value, nameptr);
+ }
+ ret = TRUE;
+out:
+ free(module_struct_mem);
+ free(module_core_mem);
+ free(module_init_mem);
+
+ return ret;
+}
+
+static int
+load_module_symbols(void)
+{
+ unsigned long head, cur, cur_module;
+ struct module_info *modules = NULL;
+ unsigned int i = 0;
head = SYMBOL(modules);
if (!get_num_modules(head, &mod_st.num_modules) ||
@@ -7664,12 +7804,6 @@ load_module_symbols(void)
}
modules = mod_st.modules;
- /* Allocate buffer to read struct module data from vmcore. */
- if ((module_struct_mem = calloc(1, SIZE(module))) == NULL) {
- ERRMSG("Failed to allocate buffer for module\n");
- return FALSE;
- }
-
if (!readmem(VADDR, head + OFFSET(list_head.next), &cur, sizeof cur)) {
ERRMSG("Can't get next list_head.\n");
return FALSE;
@@ -7678,129 +7812,9 @@ load_module_symbols(void)
/* Travese the list and read module symbols */
while (cur != head) {
cur_module = cur - OFFSET(module.list);
- if (!readmem(VADDR, cur_module, module_struct_mem,
- SIZE(module))) {
- ERRMSG("Can't get module info.\n");
- return FALSE;
- }
-
- module_name = (char *)(module_struct_mem + OFFSET(module.name));
- if (strlen(module_name) < MOD_NAME_LEN)
- strcpy(modules[i].name, module_name);
- else
- strncpy(modules[i].name, module_name, MOD_NAME_LEN-1);
-
- mod_init = ULONG(module_struct_mem +
- OFFSET(module.module_init));
- mod_init_size = UINT(module_struct_mem +
- OFFSET(module.init_size));
- mod_base = ULONG(module_struct_mem +
- OFFSET(module.module_core));
- mod_size = UINT(module_struct_mem +
- OFFSET(module.core_size));
-
- DEBUG_MSG("Module: %s, Base: 0x%lx, Size: %u\n",
- module_name, mod_base, mod_size);
- if (mod_init_size > 0) {
- module_init_mem = calloc(1, mod_init_size);
- if (module_init_mem == NULL) {
- ERRMSG("Can't allocate memory for module "
- "init\n");
- return FALSE;
- }
- if (!readmem(VADDR, mod_init, module_init_mem,
- mod_init_size)) {
- ERRMSG("Can't access module init in memory.\n");
- return FALSE;
- }
- }
- if ((module_core_mem = calloc(1, mod_size)) == NULL) {
- ERRMSG("Can't allocate memory for module\n");
+ if (!__load_module_symbol(&modules[i], cur_module))
return FALSE;
- }
- if (!readmem(VADDR, mod_base, module_core_mem, mod_size)) {
- ERRMSG("Can't access module in memory.\n");
- return FALSE;
- }
-
- num_symtab = UINT(module_struct_mem +
- OFFSET(module.num_symtab));
- if (!num_symtab) {
- ERRMSG("%s: Symbol info not available\n", module_name);
- return FALSE;
- }
- modules[i].num_syms = num_symtab;
- DEBUG_MSG("num_sym: %d\n", num_symtab);
-
- symtab = ULONG(module_struct_mem + OFFSET(module.symtab));
- strtab = ULONG(module_struct_mem + OFFSET(module.strtab));
-
- /* check if symtab and strtab are inside the module space. */
- if (!IN_RANGE(symtab, mod_base, mod_size) &&
- !IN_RANGE(symtab, mod_init, mod_init_size)) {
- ERRMSG("%s: module symtab is outseide of module "
- "address space\n", module_name);
- return FALSE;
- }
- if (IN_RANGE(symtab, mod_base, mod_size))
- symtab_mem = module_core_mem + (symtab - mod_base);
- else
- symtab_mem = module_init_mem + (symtab - mod_init);
-
- if (!IN_RANGE(strtab, mod_base, mod_size) &&
- !IN_RANGE(strtab, mod_init, mod_init_size)) {
- ERRMSG("%s: module strtab is outseide of module "
- "address space\n", module_name);
- return FALSE;
- }
- if (IN_RANGE(strtab, mod_base, mod_size))
- strtab_mem = (char *)(module_core_mem
- + (strtab - mod_base));
- else
- strtab_mem = (char *)(module_init_mem
- + (strtab - mod_init));
-
- modules[i].sym_info = calloc(num_symtab,
- sizeof(struct symbol_info));
- if (modules[i].sym_info == NULL) {
- ERRMSG("Can't allocate memory to store sym info\n");
- return FALSE;
- }
- sym_info = modules[i].sym_info;
-
- /* symbols starts from 1 */
- for (nsym = 1; nsym < num_symtab; nsym++) {
- Elf32_Sym *sym32;
- Elf64_Sym *sym64;
- /* If case of ELF vmcore then the word size can be
- * determined using info->flag_elf64_memory flag.
- * But in case of kdump-compressed dump, kdump header
- * does not carry word size info. May be in future
- * this info will be available in kdump header.
- * Until then, in order to make this logic work on both
- * situation we depend on pointer_size that is
- * extracted from vmlinux dwarf information.
- */
- if ((pointer_size * 8) == 64) {
- sym64 = (Elf64_Sym *) (symtab_mem
- + (nsym * sizeof(Elf64_Sym)));
- sym_info[nsym].value =
- (unsigned long long) sym64->st_value;
- nameptr = strtab_mem + sym64->st_name;
- }
- else {
- sym32 = (Elf32_Sym *) (symtab_mem
- + (nsym * sizeof(Elf32_Sym)));
- sym_info[nsym].value =
- (unsigned long long) sym32->st_value;
- nameptr = strtab_mem + sym32->st_name;
- }
- if (strlen(nameptr))
- sym_info[nsym].name = strdup(nameptr);
- DEBUG_MSG("\t[%d] %llx %s\n", nsym,
- sym_info[nsym].value, nameptr);
- }
if (!readmem(VADDR, cur + OFFSET(list_head.next),
&cur, sizeof cur)) {
@@ -7808,11 +7822,7 @@ load_module_symbols(void)
return FALSE;
}
i++;
- free(module_core_mem);
- if (mod_init_size > 0)
- free(module_init_mem);
} while (cur != head);
- free(module_struct_mem);
return TRUE;
}
