[PATCH] kallsyms: store type information in its own array

[PATCH] kallsyms: store type information in its own array

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

When a module is loaded, its symbols' Elf_Sym information is stored
in a symtab.  Further, type information is also captured.  Since
Elf_Sym has no type field, historically the st_info field has been
hijacked for storing type:  st_info was overwritten.

commit 5439c985c5a83a8419f762115afdf560ab72a452 ("module: Overwrite
st_size instead of st_info") changes that practice, as its one-liner
indicates.  Unfortunately, this change overwrites symbol size,
information that a tool like DTrace expects to find.

Allocate a typetab array to store type information so that no Elf_Sym
field needs to be overwritten.

Fixes: 5439c985c5a8 ("module: Overwrite st_size instead of st_info")
Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
Reviewed-by: Nick Alcock <nick.alcock@oracle.com>
---
 include/linux/module.h   |  1 +
 kernel/module-internal.h |  2 +-
 kernel/module.c          | 21 ++++++++++++++-------
 3 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index f5bc4c0..9c1bc21 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -315,6 +315,7 @@ struct mod_kallsyms {
 	Elf_Sym *symtab;
 	unsigned int num_symtab;
 	char *strtab;
+	char *typetab;
 };
 
 #ifdef CONFIG_LIVEPATCH
diff --git a/kernel/module-internal.h b/kernel/module-internal.h
index 79c9be2..67828af 100644
--- a/kernel/module-internal.h
+++ b/kernel/module-internal.h
@@ -20,7 +20,7 @@ struct load_info {
 	unsigned long len;
 	Elf_Shdr *sechdrs;
 	char *secstrings, *strtab;
-	unsigned long symoffs, stroffs;
+	unsigned long symoffs, stroffs, init_typeoff, core_typeoff;
 	struct _ddebug *debug;
 	unsigned int num_debug;
 	bool sig_ok;
diff --git a/kernel/module.c b/kernel/module.c
index 2ad1b52..c7c1bcd 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2647,6 +2647,8 @@ static void layout_symtab(struct module *mod, struct load_info *info)
 	info->symoffs = ALIGN(mod->core_layout.size, symsect->sh_addralign ?: 1);
 	info->stroffs = mod->core_layout.size = info->symoffs + ndst * sizeof(Elf_Sym);
 	mod->core_layout.size += strtab_size;
+	info->core_typeoff = mod->core_layout.size;
+	mod->core_layout.size += ndst * sizeof(char);
 	mod->core_layout.size = debug_align(mod->core_layout.size);
 
 	/* Put string table section at end of init part of module. */
@@ -2660,6 +2662,8 @@ static void layout_symtab(struct module *mod, struct load_info *info)
 				      __alignof__(struct mod_kallsyms));
 	info->mod_kallsyms_init_off = mod->init_layout.size;
 	mod->init_layout.size += sizeof(struct mod_kallsyms);
+	info->init_typeoff = mod->init_layout.size;
+	mod->init_layout.size += nsrc * sizeof(char);
 	mod->init_layout.size = debug_align(mod->init_layout.size);
 }
 
@@ -2683,20 +2687,23 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
 	mod->kallsyms->num_symtab = symsec->sh_size / sizeof(Elf_Sym);
 	/* Make sure we get permanent strtab: don't use info->strtab. */
 	mod->kallsyms->strtab = (void *)info->sechdrs[info->index.str].sh_addr;
+	mod->kallsyms->typetab = mod->init_layout.base + info->init_typeoff;
 
-	/* Set types up while we still have access to sections. */
-	for (i = 0; i < mod->kallsyms->num_symtab; i++)
-		mod->kallsyms->symtab[i].st_size
-			= elf_type(&mod->kallsyms->symtab[i], info);
-
-	/* Now populate the cut down core kallsyms for after init. */
+	/*
+	 * Now populate the cut down core kallsyms for after init
+	 * and set types up while we still have access to sections.
+	 */
 	mod->core_kallsyms.symtab = dst = mod->core_layout.base + info->symoffs;
 	mod->core_kallsyms.strtab = s = mod->core_layout.base + info->stroffs;
+	mod->core_kallsyms.typetab = mod->core_layout.base + info->core_typeoff;
 	src = mod->kallsyms->symtab;
 	for (ndst = i = 0; i < mod->kallsyms->num_symtab; i++) {
+		mod->kallsyms->typetab[i] = elf_type(src + i, info);
 		if (i == 0 || is_livepatch_module(mod) ||
 		    is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum,
 				   info->index.pcpu)) {
+			mod->core_kallsyms.typetab[ndst] =
+			    mod->kallsyms->typetab[i];
 			dst[ndst] = src[i];
 			dst[ndst++].st_name = s - mod->core_kallsyms.strtab;
 			s += strlcpy(s, &mod->kallsyms->strtab[src[i].st_name],
@@ -4084,7 +4091,7 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
 			const Elf_Sym *sym = &kallsyms->symtab[symnum];
 
 			*value = kallsyms_symbol_value(sym);
-			*type = sym->st_size;
+			*type = kallsyms->typetab[symnum];
 			strlcpy(name, kallsyms_symbol_name(kallsyms, symnum), KSYM_NAME_LEN);
 			strlcpy(module_name, mod->name, MODULE_NAME_LEN);
 			*exported = is_exported(name, *value, mod);
-- 
1.8.3.1

Re: [PATCH] kallsyms: store type information in its own array

[Eugene Loh]

It's been 1.5 weeks.  Could I get some feedback on this patch? Thanks.


On 02/25/2019 11:59 AM, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
>
> When a module is loaded, its symbols' Elf_Sym information is stored
> in a symtab.  Further, type information is also captured.  Since
> Elf_Sym has no type field, historically the st_info field has been
> hijacked for storing type:  st_info was overwritten.
>
> commit 5439c985c5a83a8419f762115afdf560ab72a452 ("module: Overwrite
> st_size instead of st_info") changes that practice, as its one-liner
> indicates.  Unfortunately, this change overwrites symbol size,
> information that a tool like DTrace expects to find.
>
> Allocate a typetab array to store type information so that no Elf_Sym
> field needs to be overwritten.
>
> Fixes: 5439c985c5a8 ("module: Overwrite st_size instead of st_info")
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> Reviewed-by: Nick Alcock <nick.alcock@oracle.com>
> ---
>   include/linux/module.h   |  1 +
>   kernel/module-internal.h |  2 +-
>   kernel/module.c          | 21 ++++++++++++++-------
>   3 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index f5bc4c0..9c1bc21 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -315,6 +315,7 @@ struct mod_kallsyms {
>   	Elf_Sym *symtab;
>   	unsigned int num_symtab;
>   	char *strtab;
> +	char *typetab;
>   };
>   
>   #ifdef CONFIG_LIVEPATCH
> diff --git a/kernel/module-internal.h b/kernel/module-internal.h
> index 79c9be2..67828af 100644
> --- a/kernel/module-internal.h
> +++ b/kernel/module-internal.h
> @@ -20,7 +20,7 @@ struct load_info {
>   	unsigned long len;
>   	Elf_Shdr *sechdrs;
>   	char *secstrings, *strtab;
> -	unsigned long symoffs, stroffs;
> +	unsigned long symoffs, stroffs, init_typeoff, core_typeoff;
>   	struct _ddebug *debug;
>   	unsigned int num_debug;
>   	bool sig_ok;
> diff --git a/kernel/module.c b/kernel/module.c
> index 2ad1b52..c7c1bcd 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2647,6 +2647,8 @@ static void layout_symtab(struct module *mod, struct load_info *info)
>   	info->symoffs = ALIGN(mod->core_layout.size, symsect->sh_addralign ?: 1);
>   	info->stroffs = mod->core_layout.size = info->symoffs + ndst * sizeof(Elf_Sym);
>   	mod->core_layout.size += strtab_size;
> +	info->core_typeoff = mod->core_layout.size;
> +	mod->core_layout.size += ndst * sizeof(char);
>   	mod->core_layout.size = debug_align(mod->core_layout.size);
>   
>   	/* Put string table section at end of init part of module. */
> @@ -2660,6 +2662,8 @@ static void layout_symtab(struct module *mod, struct load_info *info)
>   				      __alignof__(struct mod_kallsyms));
>   	info->mod_kallsyms_init_off = mod->init_layout.size;
>   	mod->init_layout.size += sizeof(struct mod_kallsyms);
> +	info->init_typeoff = mod->init_layout.size;
> +	mod->init_layout.size += nsrc * sizeof(char);
>   	mod->init_layout.size = debug_align(mod->init_layout.size);
>   }
>   
> @@ -2683,20 +2687,23 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
>   	mod->kallsyms->num_symtab = symsec->sh_size / sizeof(Elf_Sym);
>   	/* Make sure we get permanent strtab: don't use info->strtab. */
>   	mod->kallsyms->strtab = (void *)info->sechdrs[info->index.str].sh_addr;
> +	mod->kallsyms->typetab = mod->init_layout.base + info->init_typeoff;
>   
> -	/* Set types up while we still have access to sections. */
> -	for (i = 0; i < mod->kallsyms->num_symtab; i++)
> -		mod->kallsyms->symtab[i].st_size
> -			= elf_type(&mod->kallsyms->symtab[i], info);
> -
> -	/* Now populate the cut down core kallsyms for after init. */
> +	/*
> +	 * Now populate the cut down core kallsyms for after init
> +	 * and set types up while we still have access to sections.
> +	 */
>   	mod->core_kallsyms.symtab = dst = mod->core_layout.base + info->symoffs;
>   	mod->core_kallsyms.strtab = s = mod->core_layout.base + info->stroffs;
> +	mod->core_kallsyms.typetab = mod->core_layout.base + info->core_typeoff;
>   	src = mod->kallsyms->symtab;
>   	for (ndst = i = 0; i < mod->kallsyms->num_symtab; i++) {
> +		mod->kallsyms->typetab[i] = elf_type(src + i, info);
>   		if (i == 0 || is_livepatch_module(mod) ||
>   		    is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum,
>   				   info->index.pcpu)) {
> +			mod->core_kallsyms.typetab[ndst] =
> +			    mod->kallsyms->typetab[i];
>   			dst[ndst] = src[i];
>   			dst[ndst++].st_name = s - mod->core_kallsyms.strtab;
>   			s += strlcpy(s, &mod->kallsyms->strtab[src[i].st_name],
> @@ -4084,7 +4091,7 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
>   			const Elf_Sym *sym = &kallsyms->symtab[symnum];
>   
>   			*value = kallsyms_symbol_value(sym);
> -			*type = sym->st_size;
> +			*type = kallsyms->typetab[symnum];
>   			strlcpy(name, kallsyms_symbol_name(kallsyms, symnum), KSYM_NAME_LEN);
>   			strlcpy(module_name, mod->name, MODULE_NAME_LEN);
>   			*exported = is_exported(name, *value, mod);

Re: [PATCH] kallsyms: store type information in its own array

[Jessica Yu]

+++ Eugene Loh [07/03/19 15:37 -0800]:
>It's been 1.5 weeks.  Could I get some feedback on this patch? Thanks.

Apologies, I was on vacation the past week.

Added some CC's from the last time we talked about overwriting the
st_size/st_info Elf_Sym fields, in case anyone else has comments.

>On 02/25/2019 11:59 AM, eugene.loh@oracle.com wrote:
>>From: Eugene Loh <eugene.loh@oracle.com>
>>
>>When a module is loaded, its symbols' Elf_Sym information is stored
>>in a symtab.  Further, type information is also captured.  Since
>>Elf_Sym has no type field, historically the st_info field has been
>>hijacked for storing type:  st_info was overwritten.
>>
>>commit 5439c985c5a83a8419f762115afdf560ab72a452 ("module: Overwrite
>>st_size instead of st_info") changes that practice, as its one-liner
>>indicates.  Unfortunately, this change overwrites symbol size,
>>information that a tool like DTrace expects to find.
>>
>>Allocate a typetab array to store type information so that no Elf_Sym
>>field needs to be overwritten.
>>
>>Fixes: 5439c985c5a8 ("module: Overwrite st_size instead of st_info")
>>Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
>>Reviewed-by: Nick Alcock <nick.alcock@oracle.com>
>>---
>>  include/linux/module.h   |  1 +
>>  kernel/module-internal.h |  2 +-
>>  kernel/module.c          | 21 ++++++++++++++-------
>>  3 files changed, 16 insertions(+), 8 deletions(-)
>>
>>diff --git a/include/linux/module.h b/include/linux/module.h
>>index f5bc4c0..9c1bc21 100644
>>--- a/include/linux/module.h
>>+++ b/include/linux/module.h
>>@@ -315,6 +315,7 @@ struct mod_kallsyms {
>>  	Elf_Sym *symtab;
>>  	unsigned int num_symtab;
>>  	char *strtab;
>>+	char *typetab;
>>  };
>>  #ifdef CONFIG_LIVEPATCH
>>diff --git a/kernel/module-internal.h b/kernel/module-internal.h
>>index 79c9be2..67828af 100644
>>--- a/kernel/module-internal.h
>>+++ b/kernel/module-internal.h
>>@@ -20,7 +20,7 @@ struct load_info {
>>  	unsigned long len;
>>  	Elf_Shdr *sechdrs;
>>  	char *secstrings, *strtab;
>>-	unsigned long symoffs, stroffs;
>>+	unsigned long symoffs, stroffs, init_typeoff, core_typeoff;

Small nit: typeoff to typeoffs to match symoffs and stroffs.

Otherwise the rest looks fine to me. I guess it's about time we get
rid of the Elf_Sym field hijacking anyway :-)

Thanks!

Jessica

>>  	struct _ddebug *debug;
>>  	unsigned int num_debug;
>>  	bool sig_ok;
>>diff --git a/kernel/module.c b/kernel/module.c
>>index 2ad1b52..c7c1bcd 100644
>>--- a/kernel/module.c
>>+++ b/kernel/module.c
>>@@ -2647,6 +2647,8 @@ static void layout_symtab(struct module *mod, struct load_info *info)
>>  	info->symoffs = ALIGN(mod->core_layout.size, symsect->sh_addralign ?: 1);
>>  	info->stroffs = mod->core_layout.size = info->symoffs + ndst * sizeof(Elf_Sym);
>>  	mod->core_layout.size += strtab_size;
>>+	info->core_typeoff = mod->core_layout.size;
>>+	mod->core_layout.size += ndst * sizeof(char);
>>  	mod->core_layout.size = debug_align(mod->core_layout.size);
>>  	/* Put string table section at end of init part of module. */
>>@@ -2660,6 +2662,8 @@ static void layout_symtab(struct module *mod, struct load_info *info)
>>  				      __alignof__(struct mod_kallsyms));
>>  	info->mod_kallsyms_init_off = mod->init_layout.size;
>>  	mod->init_layout.size += sizeof(struct mod_kallsyms);
>>+	info->init_typeoff = mod->init_layout.size;
>>+	mod->init_layout.size += nsrc * sizeof(char);
>>  	mod->init_layout.size = debug_align(mod->init_layout.size);
>>  }
>>@@ -2683,20 +2687,23 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
>>  	mod->kallsyms->num_symtab = symsec->sh_size / sizeof(Elf_Sym);
>>  	/* Make sure we get permanent strtab: don't use info->strtab. */
>>  	mod->kallsyms->strtab = (void *)info->sechdrs[info->index.str].sh_addr;
>>+	mod->kallsyms->typetab = mod->init_layout.base + info->init_typeoff;
>>-	/* Set types up while we still have access to sections. */
>>-	for (i = 0; i < mod->kallsyms->num_symtab; i++)
>>-		mod->kallsyms->symtab[i].st_size
>>-			= elf_type(&mod->kallsyms->symtab[i], info);
>>-
>>-	/* Now populate the cut down core kallsyms for after init. */
>>+	/*
>>+	 * Now populate the cut down core kallsyms for after init
>>+	 * and set types up while we still have access to sections.
>>+	 */
>>  	mod->core_kallsyms.symtab = dst = mod->core_layout.base + info->symoffs;
>>  	mod->core_kallsyms.strtab = s = mod->core_layout.base + info->stroffs;
>>+	mod->core_kallsyms.typetab = mod->core_layout.base + info->core_typeoff;
>>  	src = mod->kallsyms->symtab;
>>  	for (ndst = i = 0; i < mod->kallsyms->num_symtab; i++) {
>>+		mod->kallsyms->typetab[i] = elf_type(src + i, info);
>>  		if (i == 0 || is_livepatch_module(mod) ||
>>  		    is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum,
>>  				   info->index.pcpu)) {
>>+			mod->core_kallsyms.typetab[ndst] =
>>+			    mod->kallsyms->typetab[i];
>>  			dst[ndst] = src[i];
>>  			dst[ndst++].st_name = s - mod->core_kallsyms.strtab;
>>  			s += strlcpy(s, &mod->kallsyms->strtab[src[i].st_name],
>>@@ -4084,7 +4091,7 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
>>  			const Elf_Sym *sym = &kallsyms->symtab[symnum];
>>  			*value = kallsyms_symbol_value(sym);
>>-			*type = sym->st_size;
>>+			*type = kallsyms->typetab[symnum];
>>  			strlcpy(name, kallsyms_symbol_name(kallsyms, symnum), KSYM_NAME_LEN);
>>  			strlcpy(module_name, mod->name, MODULE_NAME_LEN);
>>  			*exported = is_exported(name, *value, mod);
>