module: make it clearer when we're handling kallsyms symbols vs exported symbols

PATCH Superseded Jessica Yu 2 participants 4 messages Nov 21, 2018 – Nov 22, 2018

Share & tools

Jessica Yu

author

2018-11-21 17:51 UTC #0 lore raw

PATCH 1/1 module: make it clearer when we're handling kallsyms symbols vs exported symbols

The module loader internally works with both exported symbols represented as struct kernel_symbol, as well as Elf symbols from a module's symbol table. It's hard to distinguish sometimes which type of symbol we're handling given that some helper function names are not consistent or helpful. Take get_ksymbol() for instance - are we looking for an exported symbol or a kallsyms symbol here? Or symname() and kernel_symbol_name() - which function handles an exported symbol and which one an Elf symbol?

Clean up and unify the function naming scheme a bit to make it clear which kind of symbol we're handling. This change only affects static functions internal to the module loader.

Signed-off-by Jessica Yu <[email protected]>

---
 kernel/module.c | 78 ++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 44 insertions(+), 34 deletions(-)

kernel/module.c +44 ~~-34~~

index 49a405891587..27d970361e3e 100644
@@ -495,9 +495,9 @@ struct find_symbol_arg {
 	const struct kernel_symbol *sym;
 };
 
-static bool check_symbol(const struct symsearch *syms,
-				 struct module *owner,
-				 unsigned int symnum, void *data)
+static bool check_exported_symbol(const struct symsearch *syms,
+				  struct module *owner,
+				  unsigned int symnum, void *data)
 {
 	struct find_symbol_arg *fsa = data;

@@ -555,9 +555,9 @@ static int cmp_name(const void *va, const void *vb)
 	return strcmp(a, kernel_symbol_name(b));
 }
 
-static bool find_symbol_in_section(const struct symsearch *syms,
-				   struct module *owner,
-				   void *data)
+static bool find_exported_symbol_in_section(const struct symsearch *syms,
+					    struct module *owner,
+					    void *data)
 {
 	struct find_symbol_arg *fsa = data;
 	struct kernel_symbol *sym;

@@ -565,13 +565,14 @@ static bool find_symbol_in_section(const struct symsearch *syms,
 	sym = bsearch(fsa->name, syms->start, syms->stop - syms->start,
 			sizeof(struct kernel_symbol), cmp_name);
 
-	if (sym != NULL && check_symbol(syms, owner, sym - syms->start, data))
+	if (sym != NULL && check_exported_symbol(syms, owner,
+						 sym - syms->start, data))
 		return true;
 
 	return false;
 }
 
-/* Find a symbol and return it, along with, (optional) crc and
+/* Find an exported symbol and return it, along with, (optional) crc and
  * (optional) module which owns it.  Needs preempt disabled or module_mutex. */
 const struct kernel_symbol *find_symbol(const char *name,
 					struct module **owner,

@@ -585,7 +586,7 @@ const struct kernel_symbol *find_symbol(const char *name,
 	fsa.gplok = gplok;
 	fsa.warn = warn;
 
-	if (each_symbol_section(find_symbol_in_section, &fsa)) {
+	if (each_symbol_section(find_exported_symbol_in_section, &fsa)) {
 		if (owner)
 			*owner = fsa.owner;
 		if (crc)

@@ -2198,7 +2199,7 @@ EXPORT_SYMBOL_GPL(__symbol_get);
  *
  * You must hold the module_mutex.
  */
-static int verify_export_symbols(struct module *mod)
+static int verify_exported_symbols(struct module *mod)
 {
 	unsigned int i;
 	struct module *owner;

@@ -2519,10 +2520,10 @@ static void free_modinfo(struct module *mod)
 
 #ifdef CONFIG_KALLSYMS
 
-/* lookup symbol in given range of kernel_symbols */
-static const struct kernel_symbol *lookup_symbol(const char *name,
-	const struct kernel_symbol *start,
-	const struct kernel_symbol *stop)
+/* Lookup exported symbol in given range of kernel_symbols */
+static const struct kernel_symbol *lookup_exported_symbol(const char *name,
+							  const struct kernel_symbol *start,
+							  const struct kernel_symbol *stop)
 {
 	return bsearch(name, start, stop - start,
 			sizeof(struct kernel_symbol), cmp_name);

@@ -2533,9 +2534,10 @@ static int is_exported(const char *name, unsigned long value,
 {
 	const struct kernel_symbol *ks;
 	if (!mod)
-		ks = lookup_symbol(name, __start___ksymtab, __stop___ksymtab);
+		ks = lookup_exported_symbol(name, __start___ksymtab, __stop___ksymtab);
 	else
-		ks = lookup_symbol(name, mod->syms, mod->syms + mod->num_syms);
+		ks = lookup_exported_symbol(name, mod->syms, mod->syms + mod->num_syms);
+
 	return ks != NULL && kernel_symbol_value(ks) == value;
 }

@@ -3592,7 +3594,7 @@ static int complete_formation(struct module *mod, struct load_info *info)
 	mutex_lock(&module_mutex);
 
 	/* Find duplicate symbols (must be called under lock). */
-	err = verify_export_symbols(mod);
+	err = verify_exported_symbols(mod);
 	if (err < 0)
 		goto out;

@@ -3911,15 +3913,19 @@ static inline int is_arm_mapping_symbol(const char *str)
 	       && (str[2] == '\0' || str[2] == '.');
 }
 
-static const char *symname(struct mod_kallsyms *kallsyms, unsigned int symnum)
+static const char *kallsyms_symbol_name(struct mod_kallsyms *kallsyms, unsigned int symnum)
 {
 	return kallsyms->strtab + kallsyms->symtab[symnum].st_name;
 }
 
-static const char *get_ksymbol(struct module *mod,
-			       unsigned long addr,
-			       unsigned long *size,
-			       unsigned long *offset)
+/*
+ * Given a module and address, find the corresponding symbol and return its name
+ * while providing its size and offset if needed.
+ */
+static const char *kallsyms_find_symbol(struct module *mod,
+					unsigned long addr,
+					unsigned long *size,
+					unsigned long *offset)
 {
 	unsigned int i, best = 0;
 	unsigned long nextval;

@@ -3939,8 +3945,8 @@ static const char *get_ksymbol(struct module *mod,
 
 		/* We ignore unnamed symbols: they're uninformative
 		 * and inserted at a whim. */
-		if (*symname(kallsyms, i) == '\0'
-		    || is_arm_mapping_symbol(symname(kallsyms, i)))
+		if (*kallsyms_symbol_name(kallsyms, i) == '\0'
+		    || is_arm_mapping_symbol(kallsyms_symbol_name(kallsyms, i)))
 			continue;
 
 		if (kallsyms->symtab[i].st_value <= addr

@@ -3958,7 +3964,8 @@ static const char *get_ksymbol(struct module *mod,
 		*size = nextval - kallsyms->symtab[best].st_value;
 	if (offset)
 		*offset = addr - kallsyms->symtab[best].st_value;
-	return symname(kallsyms, best);
+
+	return kallsyms_symbol_name(kallsyms, best);
 }
 
 void * __weak dereference_module_function_descriptor(struct module *mod,

@@ -3983,7 +3990,8 @@ const char *module_address_lookup(unsigned long addr,
 	if (mod) {
 		if (modname)
 			*modname = mod->name;
-		ret = get_ksymbol(mod, addr, size, offset);
+
+		ret = kallsyms_find_symbol(mod, addr, size, offset);
 	}
 	/* Make a copy in here where it's safe */
 	if (ret) {

@@ -4006,9 +4014,10 @@ int lookup_module_symbol_name(unsigned long addr, char *symname)
 		if (within_module(addr, mod)) {
 			const char *sym;
 
-			sym = get_ksymbol(mod, addr, NULL, NULL);
+			sym = kallsyms_find_symbol(mod, addr, NULL, NULL);
 			if (!sym)
 				goto out;
+
 			strlcpy(symname, sym, KSYM_NAME_LEN);
 			preempt_enable();
 			return 0;

@@ -4031,7 +4040,7 @@ int lookup_module_symbol_attrs(unsigned long addr, unsigned long *size,
 		if (within_module(addr, mod)) {
 			const char *sym;
 
-			sym = get_ksymbol(mod, addr, size, offset);
+			sym = kallsyms_find_symbol(mod, addr, size, offset);
 			if (!sym)
 				goto out;
 			if (modname)

@@ -4062,7 +4071,7 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
 		if (symnum < kallsyms->num_symtab) {
 			*value = kallsyms->symtab[symnum].st_value;
 			*type = kallsyms->symtab[symnum].st_info;
-			strlcpy(name, symname(kallsyms, symnum), KSYM_NAME_LEN);
+			strlcpy(name, kallsyms_symbol_name(kallsyms, symnum), KSYM_NAME_LEN);
 			strlcpy(module_name, mod->name, MODULE_NAME_LEN);
 			*exported = is_exported(name, *value, mod);
 			preempt_enable();

@@ -4074,13 +4083,14 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
 	return -ERANGE;
 }
 
-static unsigned long mod_find_symname(struct module *mod, const char *name)
+/* Given a module and name of symbol, find and return the symbol's value */
+static unsigned long kallsyms_find_symbol_value(struct module *mod, const char *name)
 {
 	unsigned int i;
 	struct mod_kallsyms *kallsyms = rcu_dereference_sched(mod->kallsyms);
 
 	for (i = 0; i < kallsyms->num_symtab; i++)
-		if (strcmp(name, symname(kallsyms, i)) == 0 &&
+		if (strcmp(name, kallsyms_symbol_name(kallsyms, i)) == 0 &&
 		    kallsyms->symtab[i].st_shndx != SHN_UNDEF)
 			return kallsyms->symtab[i].st_value;
 	return 0;

@@ -4097,12 +4107,12 @@ unsigned long module_kallsyms_lookup_name(const char *name)
 	preempt_disable();
 	if ((colon = strnchr(name, MODULE_NAME_LEN, ':')) != NULL) {
 		if ((mod = find_module_all(name, colon - name, false)) != NULL)
-			ret = mod_find_symname(mod, colon+1);
+			ret = kallsyms_find_symbol_value(mod, colon+1);
 	} else {
 		list_for_each_entry_rcu(mod, &modules, list) {
 			if (mod->state == MODULE_STATE_UNFORMED)
 				continue;
-			if ((ret = mod_find_symname(mod, name)) != 0)
+			if ((ret = kallsyms_find_symbol_value(mod, name)) != 0)
 				break;
 		}
 	}

@@ -4131,7 +4141,7 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
 			if (kallsyms->symtab[i].st_shndx == SHN_UNDEF)
 				continue;
 
-			ret = fn(data, symname(kallsyms, i),
+			ret = fn(data, kallsyms_symbol_name(kallsyms, i),
 				 mod, kallsyms->symtab[i].st_value);
 			if (ret != 0)
 				return ret;

Signature · 1 line

2.16.4

Miroslav Benes

2018-11-22 10:19 UTC #1 lore raw

Quoted 9 lines · Jessica Yu

The module loader internally works with both exported symbols represented as struct kernel_symbol, as well as Elf symbols from a module's symbol table. It's hard to distinguish sometimes which type of symbol we're handling given that some helper function names are not consistent or helpful. Take get_ksymbol() for instance - are we looking for an exported symbol or a kallsyms symbol here? Or symname() and kernel_symbol_name() - which function handles an exported symbol and which one an Elf symbol?

Quoted 5 lines

Clean up and unify the function naming scheme a bit to make it clear which kind of symbol we're handling. This change only affects static functions internal to the module loader.
Signed-off-by Jessica Yu <[email protected]>

Great. It should help a lot. Pity we cannot rename find_symbol() as well.

I have only a naming nit. I think it is nice to have <verb>_exported_<noun> convention. New kallsyms_ names don't hold it though. Wouldn't it be better to be consistent and have find_kallsyms_symbol() instead of kallsyms_find_symbol()? Or we could do the opposite and have a "namespace" prefix first. That is, exported_<verb>_<noun>. However, I don't like it that much.

To be honest, your approach may be the best in the end.

What do you think?

Regards,
Miroslav

Jessica Yu

author

2018-11-22 13:00 UTC #2 lore raw

+++ Miroslav Benes [22/11/18 11:19 +0100]:

Quoted 26 lines · Jessica Yu

Quoted 10 lines
The module loader internally works with both exported symbols represented as struct kernel_symbol, as well as Elf symbols from a module's symbol table. It's hard to distinguish sometimes which type of symbol we're handling given that some helper function names are not consistent or helpful. Take get_ksymbol() for instance - are we looking for an exported symbol or a kallsyms symbol here? Or symname() and kernel_symbol_name() - which function handles an exported symbol and which one an Elf symbol?
Quoted 5 lines
Clean up and unify the function naming scheme a bit to make it clear which kind of symbol we're handling. This change only affects static functions internal to the module loader.
Signed-off-by Jessica Yu <[email protected]>
Great. It should help a lot. Pity we cannot rename find_symbol() as well.
I have only a naming nit. I think it is nice to have <verb>_exported_<noun> convention. New kallsyms_ names don't hold it though. Wouldn't it be better to be consistent and have find_kallsyms_symbol() instead of kallsyms_find_symbol()? Or we could do the opposite and have a "namespace" prefix first. That is, exported_<verb>_<noun>. However, I don't like it that much.

Quoted 3 lines

To be honest, your approach may be the best in the end.
What do you think?

Hi Miroslav, thank you for the comment!

Yeah, that bothered me partially too. And a lot of existing helper functions in the module loader already have a <verb>_ type prefix.

Hm, how about we use <symbol_type>_* prefixes if we are just extracting a value, and <verb>_* prefixes for functions actually performing an action?

Kallsyms:

kallsyms_symbol_name()
kallsyms_symbol_value()

find_kallsyms_symbol()
find_kallsyms_symbol_value()

etc.

Exported syms:

kernel_symbol_name()
kernel_symbol_value()

lookup_exported_symbol()
check_exported_symbol()

etc.

I had left the kernel_symbol_* functions alone since I guess they do describe what they're handling, they're handling struct kernel_symbol's. But maybe I will change that to exported_symbol_name() and exported_symbol_value() to be consistent with the naming scheme..

Thanks!

Jessica

Miroslav Benes

2018-11-22 16:16 UTC #3 lore raw

Quoted 51 lines · Jessica Yu

+++ Miroslav Benes [22/11/18 11:19 +0100]:
Quoted 26 lines · Jessica Yu
Quoted 10 lines
The module loader internally works with both exported symbols represented as struct kernel_symbol, as well as Elf symbols from a module's symbol table. It's hard to distinguish sometimes which type of symbol we're handling given that some helper function names are not consistent or helpful. Take get_ksymbol() for instance - are we looking for an exported symbol or a kallsyms symbol here? Or symname() and kernel_symbol_name() - which function handles an exported symbol and which one an Elf symbol?
Quoted 5 lines
Clean up and unify the function naming scheme a bit to make it clear which kind of symbol we're handling. This change only affects static functions internal to the module loader.
Signed-off-by Jessica Yu <[email protected]>
Great. It should help a lot. Pity we cannot rename find_symbol() as well.
I have only a naming nit. I think it is nice to have <verb>_exported_<noun> convention. New kallsyms_ names don't hold it though. Wouldn't it be better to be consistent and have find_kallsyms_symbol() instead of kallsyms_find_symbol()? Or we could do the opposite and have a "namespace" prefix first. That is, exported_<verb>_<noun>. However, I don't like it that much.
Quoted 3 lines
To be honest, your approach may be the best in the end.
What do you think?
Hi Miroslav, thank you for the comment!
Yeah, that bothered me partially too. And a lot of existing helper functions in the module loader already have a <verb>_ type prefix.
Hm, how about we use <symbol_type>_* prefixes if we are just extracting a value, and <verb>_* prefixes for functions actually performing an action?
Kallsyms:
kallsyms_symbol_name()
kallsyms_symbol_value()
find_kallsyms_symbol()
find_kallsyms_symbol_value()
etc.
Exported syms:

Quoted 7 lines

kernel_symbol_name()
kernel_symbol_value()
lookup_exported_symbol()
check_exported_symbol()
etc.

Looks good to me.

Quoted 4 lines

I had left the kernel_symbol_* functions alone since I guess they do describe what they're handling, they're handling struct kernel_symbol's. But maybe I will change that to exported_symbol_name() and exported_symbol_value() to be consistent with the naming scheme..

I don't have any preference here. Reasons for both options are valid.

Miroslav