[PATCH 0/3] module stable fixes.

[PATCH 0/3] module stable fixes.

[Rusty Russell]

Luis' async_probe is a recent issue, but the /proc/kallsyms vs
module insertion race has been the for a long time, maybe forever.

I expect some pain in backporting that fix, but mainly textual,
and there's no simpler fix (grabbing the mutex for /proc/kallsyms
would solve the reported case, but not fix it for the other kallsyms
users).

If no complaints, I'll send Linus a pull req.

Thanks!
Rusty.

Luis R. Rodriguez (1):
  modules: fix modparam async_probe request

Rusty Russell (2):
  module: wrapper for symbol name.
  modules: fix longstanding /proc/kallsyms vs module insertion race.

 include/linux/module.h |  19 ++++----
 kernel/module.c        | 120 ++++++++++++++++++++++++++++++-------------------
 2 files changed, 85 insertions(+), 54 deletions(-)

-- 
2.5.0

[PATCH 1/3] modules: fix modparam async_probe request

[Rusty Russell]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

Commit f2411da746985 ("driver-core: add driver module
asynchronous probe support") added async probe support,
in two forms:

  * in-kernel driver specification annotation
  * generic async_probe module parameter (modprobe foo async_probe)

To support the generic kernel parameter parse_args() was
extended via commit ecc8617053e0 ("module: add extra
argument for parse_params() callback") however commit
failed to f2411da746985 failed to add the required argument.

This causes a crash then whenever async_probe generic
module parameter is used. This was overlooked when the
form in which in-kernel async probe support was reworked
a bit... Fix this as originally intended.

Cc: Hannes Reinecke <hare@suse.de>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: stable@vger.kernel.org (4.2+)
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> [minimized]
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 kernel/module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index 8358f4697c0c..2149f7003e49 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3496,7 +3496,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 
 	/* Module is ready to execute: parsing args may do that. */
 	after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
-				  -32768, 32767, NULL,
+				  -32768, 32767, mod,
 				  unknown_module_param_cb);
 	if (IS_ERR(after_dashes)) {
 		err = PTR_ERR(after_dashes);
-- 
2.5.0

[PATCH 2/3] module: wrapper for symbol name.

[Rusty Russell]

This trivial wrapper adds clarity and makes the following patch
smaller.

Cc: stable@kernel.org
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 kernel/module.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 2149f7003e49..1e79d8157712 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3627,6 +3627,11 @@ static inline int is_arm_mapping_symbol(const char *str)
 	       && (str[2] == '\0' || str[2] == '.');
 }
 
+static const char *symname(struct module *mod, unsigned int symnum)
+{
+	return mod->strtab + mod->symtab[symnum].st_name;
+}
+
 static const char *get_ksymbol(struct module *mod,
 			       unsigned long addr,
 			       unsigned long *size,
@@ -3649,15 +3654,15 @@ static const char *get_ksymbol(struct module *mod,
 
 		/* We ignore unnamed symbols: they're uninformative
 		 * and inserted at a whim. */
+		if (*symname(mod, i) == '\0'
+		    || is_arm_mapping_symbol(symname(mod, i)))
+			continue;
+
 		if (mod->symtab[i].st_value <= addr
-		    && mod->symtab[i].st_value > mod->symtab[best].st_value
-		    && *(mod->strtab + mod->symtab[i].st_name) != '\0'
-		    && !is_arm_mapping_symbol(mod->strtab + mod->symtab[i].st_name))
+		    && mod->symtab[i].st_value > mod->symtab[best].st_value)
 			best = i;
 		if (mod->symtab[i].st_value > addr
-		    && mod->symtab[i].st_value < nextval
-		    && *(mod->strtab + mod->symtab[i].st_name) != '\0'
-		    && !is_arm_mapping_symbol(mod->strtab + mod->symtab[i].st_name))
+		    && mod->symtab[i].st_value < nextval)
 			nextval = mod->symtab[i].st_value;
 	}
 
@@ -3668,7 +3673,7 @@ static const char *get_ksymbol(struct module *mod,
 		*size = nextval - mod->symtab[best].st_value;
 	if (offset)
 		*offset = addr - mod->symtab[best].st_value;
-	return mod->strtab + mod->symtab[best].st_name;
+	return symname(mod, best);
 }
 
 /* For kallsyms to ask for address resolution.  NULL means not found.  Careful
@@ -3763,8 +3768,7 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
 		if (symnum < mod->num_symtab) {
 			*value = mod->symtab[symnum].st_value;
 			*type = mod->symtab[symnum].st_info;
-			strlcpy(name, mod->strtab + mod->symtab[symnum].st_name,
-				KSYM_NAME_LEN);
+			strlcpy(name, symname(mod, symnum), KSYM_NAME_LEN);
 			strlcpy(module_name, mod->name, MODULE_NAME_LEN);
 			*exported = is_exported(name, *value, mod);
 			preempt_enable();
@@ -3781,7 +3785,7 @@ static unsigned long mod_find_symname(struct module *mod, const char *name)
 	unsigned int i;
 
 	for (i = 0; i < mod->num_symtab; i++)
-		if (strcmp(name, mod->strtab+mod->symtab[i].st_name) == 0 &&
+		if (strcmp(name, symname(mod, i)) == 0 &&
 		    mod->symtab[i].st_info != 'U')
 			return mod->symtab[i].st_value;
 	return 0;
@@ -3825,7 +3829,7 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
 		if (mod->state == MODULE_STATE_UNFORMED)
 			continue;
 		for (i = 0; i < mod->num_symtab; i++) {
-			ret = fn(data, mod->strtab + mod->symtab[i].st_name,
+			ret = fn(data, symname(mod, i),
 				 mod, mod->symtab[i].st_value);
 			if (ret != 0)
 				return ret;
-- 
2.5.0

RE: [PATCH 2/3] module: wrapper for symbol name.

[平松雅巳 / HIRAMATU，MASAMI]

From: Rusty Russell [mailto:rusty@rustcorp.com.au]
>
>This trivial wrapper adds clarity and makes the following patch
>smaller.
>
>Cc: stable@kernel.org
>Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Looks good to me.

Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Thanks!

>---
> kernel/module.c | 26 +++++++++++++++-----------
> 1 file changed, 15 insertions(+), 11 deletions(-)
>
>diff --git a/kernel/module.c b/kernel/module.c
>index 2149f7003e49..1e79d8157712 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -3627,6 +3627,11 @@ static inline int is_arm_mapping_symbol(const char *str)
> 	       && (str[2] == '\0' || str[2] == '.');
> }
>
>+static const char *symname(struct module *mod, unsigned int symnum)
>+{
>+	return mod->strtab + mod->symtab[symnum].st_name;
>+}
>+
> static const char *get_ksymbol(struct module *mod,
> 			       unsigned long addr,
> 			       unsigned long *size,
>@@ -3649,15 +3654,15 @@ static const char *get_ksymbol(struct module *mod,
>
> 		/* We ignore unnamed symbols: they're uninformative
> 		 * and inserted at a whim. */
>+		if (*symname(mod, i) == '\0'
>+		    || is_arm_mapping_symbol(symname(mod, i)))
>+			continue;
>+
> 		if (mod->symtab[i].st_value <= addr
>-		    && mod->symtab[i].st_value > mod->symtab[best].st_value
>-		    && *(mod->strtab + mod->symtab[i].st_name) != '\0'
>-		    && !is_arm_mapping_symbol(mod->strtab + mod->symtab[i].st_name))
>+		    && mod->symtab[i].st_value > mod->symtab[best].st_value)
> 			best = i;
> 		if (mod->symtab[i].st_value > addr
>-		    && mod->symtab[i].st_value < nextval
>-		    && *(mod->strtab + mod->symtab[i].st_name) != '\0'
>-		    && !is_arm_mapping_symbol(mod->strtab + mod->symtab[i].st_name))
>+		    && mod->symtab[i].st_value < nextval)
> 			nextval = mod->symtab[i].st_value;
> 	}
>
>@@ -3668,7 +3673,7 @@ static const char *get_ksymbol(struct module *mod,
> 		*size = nextval - mod->symtab[best].st_value;
> 	if (offset)
> 		*offset = addr - mod->symtab[best].st_value;
>-	return mod->strtab + mod->symtab[best].st_name;
>+	return symname(mod, best);
> }
>
> /* For kallsyms to ask for address resolution.  NULL means not found.  Careful
>@@ -3763,8 +3768,7 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
> 		if (symnum < mod->num_symtab) {
> 			*value = mod->symtab[symnum].st_value;
> 			*type = mod->symtab[symnum].st_info;
>-			strlcpy(name, mod->strtab + mod->symtab[symnum].st_name,
>-				KSYM_NAME_LEN);
>+			strlcpy(name, symname(mod, symnum), KSYM_NAME_LEN);
> 			strlcpy(module_name, mod->name, MODULE_NAME_LEN);
> 			*exported = is_exported(name, *value, mod);
> 			preempt_enable();
>@@ -3781,7 +3785,7 @@ static unsigned long mod_find_symname(struct module *mod, const char *name)
> 	unsigned int i;
>
> 	for (i = 0; i < mod->num_symtab; i++)
>-		if (strcmp(name, mod->strtab+mod->symtab[i].st_name) == 0 &&
>+		if (strcmp(name, symname(mod, i)) == 0 &&
> 		    mod->symtab[i].st_info != 'U')
> 			return mod->symtab[i].st_value;
> 	return 0;
>@@ -3825,7 +3829,7 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
> 		if (mod->state == MODULE_STATE_UNFORMED)
> 			continue;
> 		for (i = 0; i < mod->num_symtab; i++) {
>-			ret = fn(data, mod->strtab + mod->symtab[i].st_name,
>+			ret = fn(data, symname(mod, i),
> 				 mod, mod->symtab[i].st_value);
> 			if (ret != 0)
> 				return ret;
>--
>2.5.0

[PATCH 3/3] modules: fix longstanding /proc/kallsyms vs module insertion race.

[Rusty Russell]

For CONFIG_KALLSYMS, we keep two symbol tables and two string tables.
There's one full copy, marked SHF_ALLOC and laid out at the end of the
module's init section.  There's also a cut-down version that only
contains core symbols and strings, and lives in the module's core
section.

After module init (and before we free the module memory), we switch
the mod->symtab, mod->num_symtab and mod->strtab to point to the core
versions.  We do this under the module_mutex.

However, kallsyms doesn't take the module_mutex: it uses
preempt_disable() and rcu tricks to walk through the modules, because
it's used in the oops path.  It's also used in /proc/kallsyms.
There's nothing atomic about the change of these variables, so we can
get the old (larger!) num_symtab and the new symtab pointer; in fact
this is what I saw when trying to reproduce.

By grouping these variables together, we can use a
carefully-dereferenced pointer to ensure we always get one or the
other (the free of the module init section is already done in an RCU
callback, so that's safe).  We allocate the init one at the end of the
module init section, and keep the core one inside the struct module
itself (it could also have been allocated at the end of the module
core, but that's probably overkill).

Reported-by: Weilong Chen <chenweilong@huawei.com>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=111541
Cc: stable@kernel.org
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/linux/module.h |  19 +++++----
 kernel/module.c        | 112 ++++++++++++++++++++++++++++++-------------------
 2 files changed, 79 insertions(+), 52 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 4560d8f1545d..2bb0c3085706 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -324,6 +324,12 @@ struct module_layout {
 #define __module_layout_align
 #endif
 
+struct mod_kallsyms {
+	Elf_Sym *symtab;
+	unsigned int num_symtab;
+	char *strtab;
+};
+
 struct module {
 	enum module_state state;
 
@@ -405,15 +411,10 @@ struct module {
 #endif
 
 #ifdef CONFIG_KALLSYMS
-	/*
-	 * We keep the symbol and string tables for kallsyms.
-	 * The core_* fields below are temporary, loader-only (they
-	 * could really be discarded after module init).
-	 */
-	Elf_Sym *symtab, *core_symtab;
-	unsigned int num_symtab, core_num_syms;
-	char *strtab, *core_strtab;
-
+	/* Protected by RCU and/or module_mutex: use rcu_dereference() */
+	struct mod_kallsyms *kallsyms;
+	struct mod_kallsyms core_kallsyms;
+	
 	/* Section attributes */
 	struct module_sect_attrs *sect_attrs;
 
diff --git a/kernel/module.c b/kernel/module.c
index 1e79d8157712..9537da37ce87 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -303,6 +303,9 @@ struct load_info {
 	struct _ddebug *debug;
 	unsigned int num_debug;
 	bool sig_ok;
+#ifdef CONFIG_KALLSYMS
+	unsigned long mod_kallsyms_init_off;
+#endif
 	struct {
 		unsigned int sym, str, mod, vers, info, pcpu;
 	} index;
@@ -2480,10 +2483,21 @@ static void layout_symtab(struct module *mod, struct load_info *info)
 	strsect->sh_flags |= SHF_ALLOC;
 	strsect->sh_entsize = get_offset(mod, &mod->init_layout.size, strsect,
 					 info->index.str) | INIT_OFFSET_MASK;
-	mod->init_layout.size = debug_align(mod->init_layout.size);
 	pr_debug("\t%s\n", info->secstrings + strsect->sh_name);
+
+	/* We'll tack temporary mod_kallsyms on the end. */
+	mod->init_layout.size = ALIGN(mod->init_layout.size,
+				      __alignof__(struct mod_kallsyms));
+	info->mod_kallsyms_init_off = mod->init_layout.size;
+	mod->init_layout.size += sizeof(struct mod_kallsyms);
+	mod->init_layout.size = debug_align(mod->init_layout.size);
 }
 
+/*
+ * We use the full symtab and strtab which layout_symtab arranged to
+ * be appended to the init section.  Later we switch to the cut-down
+ * core-only ones.
+ */
 static void add_kallsyms(struct module *mod, const struct load_info *info)
 {
 	unsigned int i, ndst;
@@ -2492,29 +2506,34 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
 	char *s;
 	Elf_Shdr *symsec = &info->sechdrs[info->index.sym];
 
-	mod->symtab = (void *)symsec->sh_addr;
-	mod->num_symtab = symsec->sh_size / sizeof(Elf_Sym);
+	/* Set up to point into init section. */
+	mod->kallsyms = mod->init_layout.base + info->mod_kallsyms_init_off;
+
+	mod->kallsyms->symtab = (void *)symsec->sh_addr;
+	mod->kallsyms->num_symtab = symsec->sh_size / sizeof(Elf_Sym);
 	/* Make sure we get permanent strtab: don't use info->strtab. */
-	mod->strtab = (void *)info->sechdrs[info->index.str].sh_addr;
+	mod->kallsyms->strtab = (void *)info->sechdrs[info->index.str].sh_addr;
 
 	/* Set types up while we still have access to sections. */
-	for (i = 0; i < mod->num_symtab; i++)
-		mod->symtab[i].st_info = elf_type(&mod->symtab[i], info);
-
-	mod->core_symtab = dst = mod->core_layout.base + info->symoffs;
-	mod->core_strtab = s = mod->core_layout.base + info->stroffs;
-	src = mod->symtab;
-	for (ndst = i = 0; i < mod->num_symtab; i++) {
+	for (i = 0; i < mod->kallsyms->num_symtab; i++)
+		mod->kallsyms->symtab[i].st_info
+			= elf_type(&mod->kallsyms->symtab[i], info);
+
+	/* Now populate the cut down core kallsyms for after init. */
+	mod->core_kallsyms.symtab = dst = mod->core_layout.base + info->symoffs;
+	mod->core_kallsyms.strtab = s = mod->core_layout.base + info->stroffs;
+	src = mod->kallsyms->symtab;
+	for (ndst = i = 0; i < mod->kallsyms->num_symtab; i++) {
 		if (i == 0 ||
 		    is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum,
 				   info->index.pcpu)) {
 			dst[ndst] = src[i];
-			dst[ndst++].st_name = s - mod->core_strtab;
-			s += strlcpy(s, &mod->strtab[src[i].st_name],
+			dst[ndst++].st_name = s - mod->core_kallsyms.strtab;
+			s += strlcpy(s, &mod->kallsyms->strtab[src[i].st_name],
 				     KSYM_NAME_LEN) + 1;
 		}
 	}
-	mod->core_num_syms = ndst;
+	mod->core_kallsyms.num_symtab = ndst;
 }
 #else
 static inline void layout_symtab(struct module *mod, struct load_info *info)
@@ -3263,9 +3282,8 @@ static noinline int do_init_module(struct module *mod)
 	module_put(mod);
 	trim_init_extable(mod);
 #ifdef CONFIG_KALLSYMS
-	mod->num_symtab = mod->core_num_syms;
-	mod->symtab = mod->core_symtab;
-	mod->strtab = mod->core_strtab;
+	/* Switch to core kallsyms now init is done: kallsyms may be walking! */
+	rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms);
 #endif
 	mod_tree_remove_init(mod);
 	disable_ro_nx(&mod->init_layout);
@@ -3627,9 +3645,9 @@ static inline int is_arm_mapping_symbol(const char *str)
 	       && (str[2] == '\0' || str[2] == '.');
 }
 
-static const char *symname(struct module *mod, unsigned int symnum)
+static const char *symname(struct mod_kallsyms *kallsyms, unsigned int symnum)
 {
-	return mod->strtab + mod->symtab[symnum].st_name;
+	return kallsyms->strtab + kallsyms->symtab[symnum].st_name;
 }
 
 static const char *get_ksymbol(struct module *mod,
@@ -3639,6 +3657,7 @@ static const char *get_ksymbol(struct module *mod,
 {
 	unsigned int i, best = 0;
 	unsigned long nextval;
+	struct mod_kallsyms *kallsyms = rcu_dereference_sched(mod->kallsyms);
 
 	/* At worse, next value is at end of module */
 	if (within_module_init(addr, mod))
@@ -3648,32 +3667,32 @@ static const char *get_ksymbol(struct module *mod,
 
 	/* Scan for closest preceding symbol, and next symbol. (ELF
 	   starts real symbols at 1). */
-	for (i = 1; i < mod->num_symtab; i++) {
-		if (mod->symtab[i].st_shndx == SHN_UNDEF)
+	for (i = 1; i < kallsyms->num_symtab; i++) {
+		if (kallsyms->symtab[i].st_shndx == SHN_UNDEF)
 			continue;
 
 		/* We ignore unnamed symbols: they're uninformative
 		 * and inserted at a whim. */
-		if (*symname(mod, i) == '\0'
-		    || is_arm_mapping_symbol(symname(mod, i)))
+		if (*symname(kallsyms, i) == '\0'
+		    || is_arm_mapping_symbol(symname(kallsyms, i)))
 			continue;
 
-		if (mod->symtab[i].st_value <= addr
-		    && mod->symtab[i].st_value > mod->symtab[best].st_value)
+		if (kallsyms->symtab[i].st_value <= addr
+		    && kallsyms->symtab[i].st_value > kallsyms->symtab[best].st_value)
 			best = i;
-		if (mod->symtab[i].st_value > addr
-		    && mod->symtab[i].st_value < nextval)
-			nextval = mod->symtab[i].st_value;
+		if (kallsyms->symtab[i].st_value > addr
+		    && kallsyms->symtab[i].st_value < nextval)
+			nextval = kallsyms->symtab[i].st_value;
 	}
 
 	if (!best)
 		return NULL;
 
 	if (size)
-		*size = nextval - mod->symtab[best].st_value;
+		*size = nextval - kallsyms->symtab[best].st_value;
 	if (offset)
-		*offset = addr - mod->symtab[best].st_value;
-	return symname(mod, best);
+		*offset = addr - kallsyms->symtab[best].st_value;
+	return symname(kallsyms, best);
 }
 
 /* For kallsyms to ask for address resolution.  NULL means not found.  Careful
@@ -3763,18 +3782,21 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
 
 	preempt_disable();
 	list_for_each_entry_rcu(mod, &modules, list) {
+		struct mod_kallsyms *kallsyms;
+
 		if (mod->state == MODULE_STATE_UNFORMED)
 			continue;
-		if (symnum < mod->num_symtab) {
-			*value = mod->symtab[symnum].st_value;
-			*type = mod->symtab[symnum].st_info;
-			strlcpy(name, symname(mod, symnum), KSYM_NAME_LEN);
+		kallsyms = rcu_dereference_sched(mod->kallsyms);
+		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(module_name, mod->name, MODULE_NAME_LEN);
 			*exported = is_exported(name, *value, mod);
 			preempt_enable();
 			return 0;
 		}
-		symnum -= mod->num_symtab;
+		symnum -= kallsyms->num_symtab;
 	}
 	preempt_enable();
 	return -ERANGE;
@@ -3783,11 +3805,12 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
 static unsigned long mod_find_symname(struct module *mod, const char *name)
 {
 	unsigned int i;
+	struct mod_kallsyms *kallsyms = rcu_dereference_sched(mod->kallsyms);
 
-	for (i = 0; i < mod->num_symtab; i++)
-		if (strcmp(name, symname(mod, i)) == 0 &&
-		    mod->symtab[i].st_info != 'U')
-			return mod->symtab[i].st_value;
+	for (i = 0; i < kallsyms->num_symtab; i++)
+		if (strcmp(name, symname(kallsyms, i)) == 0 &&
+		    kallsyms->symtab[i].st_info != 'U')
+			return kallsyms->symtab[i].st_value;
 	return 0;
 }
 
@@ -3826,11 +3849,14 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
 	module_assert_mutex();
 
 	list_for_each_entry(mod, &modules, list) {
+		/* We hold module_mutex: no need for rcu_dereference_sched */
+		struct mod_kallsyms *kallsyms = mod->kallsyms;
+
 		if (mod->state == MODULE_STATE_UNFORMED)
 			continue;
-		for (i = 0; i < mod->num_symtab; i++) {
-			ret = fn(data, symname(mod, i),
-				 mod, mod->symtab[i].st_value);
+		for (i = 0; i < kallsyms->num_symtab; i++) {
+			ret = fn(data, symname(kallsyms, i),
+				 mod, kallsyms->symtab[i].st_value);
 			if (ret != 0)
 				return ret;
 		}
-- 
2.5.0

RE: [PATCH 3/3] modules: fix longstanding /proc/kallsyms vs module insertion race.

[平松雅巳 / HIRAMATU，MASAMI]

From: Rusty Russell [mailto:rusty@rustcorp.com.au]
>
>For CONFIG_KALLSYMS, we keep two symbol tables and two string tables.
>There's one full copy, marked SHF_ALLOC and laid out at the end of the
>module's init section.  There's also a cut-down version that only
>contains core symbols and strings, and lives in the module's core
>section.
>
>After module init (and before we free the module memory), we switch
>the mod->symtab, mod->num_symtab and mod->strtab to point to the core
>versions.  We do this under the module_mutex.
>
>However, kallsyms doesn't take the module_mutex: it uses
>preempt_disable() and rcu tricks to walk through the modules, because
>it's used in the oops path.  It's also used in /proc/kallsyms.
>There's nothing atomic about the change of these variables, so we can
>get the old (larger!) num_symtab and the new symtab pointer; in fact
>this is what I saw when trying to reproduce.
>
>By grouping these variables together, we can use a
>carefully-dereferenced pointer to ensure we always get one or the
>other (the free of the module init section is already done in an RCU
>callback, so that's safe).  We allocate the init one at the end of the
>module init section, and keep the core one inside the struct module
>itself (it could also have been allocated at the end of the module
>core, but that's probably overkill).
>
>Reported-by: Weilong Chen <chenweilong@huawei.com>
>Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=111541
>Cc: stable@kernel.org
>Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Looks good to me.

Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Thanks!

>---
> include/linux/module.h |  19 +++++----
> kernel/module.c        | 112 ++++++++++++++++++++++++++++++-------------------
> 2 files changed, 79 insertions(+), 52 deletions(-)
>
>diff --git a/include/linux/module.h b/include/linux/module.h
>index 4560d8f1545d..2bb0c3085706 100644
>--- a/include/linux/module.h
>+++ b/include/linux/module.h
>@@ -324,6 +324,12 @@ struct module_layout {
> #define __module_layout_align
> #endif
>
>+struct mod_kallsyms {
>+	Elf_Sym *symtab;
>+	unsigned int num_symtab;
>+	char *strtab;
>+};
>+
> struct module {
> 	enum module_state state;
>
>@@ -405,15 +411,10 @@ struct module {
> #endif
>
> #ifdef CONFIG_KALLSYMS
>-	/*
>-	 * We keep the symbol and string tables for kallsyms.
>-	 * The core_* fields below are temporary, loader-only (they
>-	 * could really be discarded after module init).
>-	 */
>-	Elf_Sym *symtab, *core_symtab;
>-	unsigned int num_symtab, core_num_syms;
>-	char *strtab, *core_strtab;
>-
>+	/* Protected by RCU and/or module_mutex: use rcu_dereference() */
>+	struct mod_kallsyms *kallsyms;
>+	struct mod_kallsyms core_kallsyms;
>+
> 	/* Section attributes */
> 	struct module_sect_attrs *sect_attrs;
>
>diff --git a/kernel/module.c b/kernel/module.c
>index 1e79d8157712..9537da37ce87 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -303,6 +303,9 @@ struct load_info {
> 	struct _ddebug *debug;
> 	unsigned int num_debug;
> 	bool sig_ok;
>+#ifdef CONFIG_KALLSYMS
>+	unsigned long mod_kallsyms_init_off;
>+#endif
> 	struct {
> 		unsigned int sym, str, mod, vers, info, pcpu;
> 	} index;
>@@ -2480,10 +2483,21 @@ static void layout_symtab(struct module *mod, struct load_info *info)
> 	strsect->sh_flags |= SHF_ALLOC;
> 	strsect->sh_entsize = get_offset(mod, &mod->init_layout.size, strsect,
> 					 info->index.str) | INIT_OFFSET_MASK;
>-	mod->init_layout.size = debug_align(mod->init_layout.size);
> 	pr_debug("\t%s\n", info->secstrings + strsect->sh_name);
>+
>+	/* We'll tack temporary mod_kallsyms on the end. */
>+	mod->init_layout.size = ALIGN(mod->init_layout.size,
>+				      __alignof__(struct mod_kallsyms));
>+	info->mod_kallsyms_init_off = mod->init_layout.size;
>+	mod->init_layout.size += sizeof(struct mod_kallsyms);
>+	mod->init_layout.size = debug_align(mod->init_layout.size);
> }
>
>+/*
>+ * We use the full symtab and strtab which layout_symtab arranged to
>+ * be appended to the init section.  Later we switch to the cut-down
>+ * core-only ones.
>+ */
> static void add_kallsyms(struct module *mod, const struct load_info *info)
> {
> 	unsigned int i, ndst;
>@@ -2492,29 +2506,34 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
> 	char *s;
> 	Elf_Shdr *symsec = &info->sechdrs[info->index.sym];
>
>-	mod->symtab = (void *)symsec->sh_addr;
>-	mod->num_symtab = symsec->sh_size / sizeof(Elf_Sym);
>+	/* Set up to point into init section. */
>+	mod->kallsyms = mod->init_layout.base + info->mod_kallsyms_init_off;
>+
>+	mod->kallsyms->symtab = (void *)symsec->sh_addr;
>+	mod->kallsyms->num_symtab = symsec->sh_size / sizeof(Elf_Sym);
> 	/* Make sure we get permanent strtab: don't use info->strtab. */
>-	mod->strtab = (void *)info->sechdrs[info->index.str].sh_addr;
>+	mod->kallsyms->strtab = (void *)info->sechdrs[info->index.str].sh_addr;
>
> 	/* Set types up while we still have access to sections. */
>-	for (i = 0; i < mod->num_symtab; i++)
>-		mod->symtab[i].st_info = elf_type(&mod->symtab[i], info);
>-
>-	mod->core_symtab = dst = mod->core_layout.base + info->symoffs;
>-	mod->core_strtab = s = mod->core_layout.base + info->stroffs;
>-	src = mod->symtab;
>-	for (ndst = i = 0; i < mod->num_symtab; i++) {
>+	for (i = 0; i < mod->kallsyms->num_symtab; i++)
>+		mod->kallsyms->symtab[i].st_info
>+			= elf_type(&mod->kallsyms->symtab[i], info);
>+
>+	/* Now populate the cut down core kallsyms for after init. */
>+	mod->core_kallsyms.symtab = dst = mod->core_layout.base + info->symoffs;
>+	mod->core_kallsyms.strtab = s = mod->core_layout.base + info->stroffs;
>+	src = mod->kallsyms->symtab;
>+	for (ndst = i = 0; i < mod->kallsyms->num_symtab; i++) {
> 		if (i == 0 ||
> 		    is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum,
> 				   info->index.pcpu)) {
> 			dst[ndst] = src[i];
>-			dst[ndst++].st_name = s - mod->core_strtab;
>-			s += strlcpy(s, &mod->strtab[src[i].st_name],
>+			dst[ndst++].st_name = s - mod->core_kallsyms.strtab;
>+			s += strlcpy(s, &mod->kallsyms->strtab[src[i].st_name],
> 				     KSYM_NAME_LEN) + 1;
> 		}
> 	}
>-	mod->core_num_syms = ndst;
>+	mod->core_kallsyms.num_symtab = ndst;
> }
> #else
> static inline void layout_symtab(struct module *mod, struct load_info *info)
>@@ -3263,9 +3282,8 @@ static noinline int do_init_module(struct module *mod)
> 	module_put(mod);
> 	trim_init_extable(mod);
> #ifdef CONFIG_KALLSYMS
>-	mod->num_symtab = mod->core_num_syms;
>-	mod->symtab = mod->core_symtab;
>-	mod->strtab = mod->core_strtab;
>+	/* Switch to core kallsyms now init is done: kallsyms may be walking! */
>+	rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms);
> #endif
> 	mod_tree_remove_init(mod);
> 	disable_ro_nx(&mod->init_layout);
>@@ -3627,9 +3645,9 @@ static inline int is_arm_mapping_symbol(const char *str)
> 	       && (str[2] == '\0' || str[2] == '.');
> }
>
>-static const char *symname(struct module *mod, unsigned int symnum)
>+static const char *symname(struct mod_kallsyms *kallsyms, unsigned int symnum)
> {
>-	return mod->strtab + mod->symtab[symnum].st_name;
>+	return kallsyms->strtab + kallsyms->symtab[symnum].st_name;
> }
>
> static const char *get_ksymbol(struct module *mod,
>@@ -3639,6 +3657,7 @@ static const char *get_ksymbol(struct module *mod,
> {
> 	unsigned int i, best = 0;
> 	unsigned long nextval;
>+	struct mod_kallsyms *kallsyms = rcu_dereference_sched(mod->kallsyms);
>
> 	/* At worse, next value is at end of module */
> 	if (within_module_init(addr, mod))
>@@ -3648,32 +3667,32 @@ static const char *get_ksymbol(struct module *mod,
>
> 	/* Scan for closest preceding symbol, and next symbol. (ELF
> 	   starts real symbols at 1). */
>-	for (i = 1; i < mod->num_symtab; i++) {
>-		if (mod->symtab[i].st_shndx == SHN_UNDEF)
>+	for (i = 1; i < kallsyms->num_symtab; i++) {
>+		if (kallsyms->symtab[i].st_shndx == SHN_UNDEF)
> 			continue;
>
> 		/* We ignore unnamed symbols: they're uninformative
> 		 * and inserted at a whim. */
>-		if (*symname(mod, i) == '\0'
>-		    || is_arm_mapping_symbol(symname(mod, i)))
>+		if (*symname(kallsyms, i) == '\0'
>+		    || is_arm_mapping_symbol(symname(kallsyms, i)))
> 			continue;
>
>-		if (mod->symtab[i].st_value <= addr
>-		    && mod->symtab[i].st_value > mod->symtab[best].st_value)
>+		if (kallsyms->symtab[i].st_value <= addr
>+		    && kallsyms->symtab[i].st_value > kallsyms->symtab[best].st_value)
> 			best = i;
>-		if (mod->symtab[i].st_value > addr
>-		    && mod->symtab[i].st_value < nextval)
>-			nextval = mod->symtab[i].st_value;
>+		if (kallsyms->symtab[i].st_value > addr
>+		    && kallsyms->symtab[i].st_value < nextval)
>+			nextval = kallsyms->symtab[i].st_value;
> 	}
>
> 	if (!best)
> 		return NULL;
>
> 	if (size)
>-		*size = nextval - mod->symtab[best].st_value;
>+		*size = nextval - kallsyms->symtab[best].st_value;
> 	if (offset)
>-		*offset = addr - mod->symtab[best].st_value;
>-	return symname(mod, best);
>+		*offset = addr - kallsyms->symtab[best].st_value;
>+	return symname(kallsyms, best);
> }
>
> /* For kallsyms to ask for address resolution.  NULL means not found.  Careful
>@@ -3763,18 +3782,21 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
>
> 	preempt_disable();
> 	list_for_each_entry_rcu(mod, &modules, list) {
>+		struct mod_kallsyms *kallsyms;
>+
> 		if (mod->state == MODULE_STATE_UNFORMED)
> 			continue;
>-		if (symnum < mod->num_symtab) {
>-			*value = mod->symtab[symnum].st_value;
>-			*type = mod->symtab[symnum].st_info;
>-			strlcpy(name, symname(mod, symnum), KSYM_NAME_LEN);
>+		kallsyms = rcu_dereference_sched(mod->kallsyms);
>+		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(module_name, mod->name, MODULE_NAME_LEN);
> 			*exported = is_exported(name, *value, mod);
> 			preempt_enable();
> 			return 0;
> 		}
>-		symnum -= mod->num_symtab;
>+		symnum -= kallsyms->num_symtab;
> 	}
> 	preempt_enable();
> 	return -ERANGE;
>@@ -3783,11 +3805,12 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
> static unsigned long mod_find_symname(struct module *mod, const char *name)
> {
> 	unsigned int i;
>+	struct mod_kallsyms *kallsyms = rcu_dereference_sched(mod->kallsyms);
>
>-	for (i = 0; i < mod->num_symtab; i++)
>-		if (strcmp(name, symname(mod, i)) == 0 &&
>-		    mod->symtab[i].st_info != 'U')
>-			return mod->symtab[i].st_value;
>+	for (i = 0; i < kallsyms->num_symtab; i++)
>+		if (strcmp(name, symname(kallsyms, i)) == 0 &&
>+		    kallsyms->symtab[i].st_info != 'U')
>+			return kallsyms->symtab[i].st_value;
> 	return 0;
> }
>
>@@ -3826,11 +3849,14 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
> 	module_assert_mutex();
>
> 	list_for_each_entry(mod, &modules, list) {
>+		/* We hold module_mutex: no need for rcu_dereference_sched */
>+		struct mod_kallsyms *kallsyms = mod->kallsyms;
>+
> 		if (mod->state == MODULE_STATE_UNFORMED)
> 			continue;
>-		for (i = 0; i < mod->num_symtab; i++) {
>-			ret = fn(data, symname(mod, i),
>-				 mod, mod->symtab[i].st_value);
>+		for (i = 0; i < kallsyms->num_symtab; i++) {
>+			ret = fn(data, symname(kallsyms, i),
>+				 mod, kallsyms->symtab[i].st_value);
> 			if (ret != 0)
> 				return ret;
> 		}
>--
>2.5.0

RE: [PATCH 0/3] module stable fixes.

[平松雅巳 / HIRAMATU，MASAMI]

Hi Rusty,

I'm OK for this series of patches, and it seems that the CC list on your mail
(on patch mails) is broken.

  CC: Rusty Russell <rusty@rustcorp.com.au>, Weilong Chen
	<chenweilong@huawei.com>, Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>,
>--->
	<"PDate:Wed"@ozlabs.org>, <3@ozlabs.org>, <Feb@ozlabs.org>,
	<2016@ozlabs.org>, <"16:55:26"@ozlabs.org>, <+1030@ozlabs.org>,
<---<
	<stable@kernel.org>

Maybe just an off-topic.

Thanks!

-- 
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: masami.hiramatsu.pt@hitachi.com


>-----Original Message-----
>From: Rusty Russell [mailto:rusty@rustcorp.com.au]
>Sent: Friday, February 05, 2016 9:14 AM
>To: linux-kernel@vger.kernel.org
>Cc: Rusty Russell; Weilong Chen; 平松雅巳 / HIRAMATU，MASAMI; Peter Zijlstra
>Subject: [PATCH 0/3] module stable fixes.
>
>Luis' async_probe is a recent issue, but the /proc/kallsyms vs
>module insertion race has been the for a long time, maybe forever.
>
>I expect some pain in backporting that fix, but mainly textual,
>and there's no simpler fix (grabbing the mutex for /proc/kallsyms
>would solve the reported case, but not fix it for the other kallsyms
>users).
>
>If no complaints, I'll send Linus a pull req.
>
>Thanks!
>Rusty.
>
>Luis R. Rodriguez (1):
>  modules: fix modparam async_probe request
>
>Rusty Russell (2):
>  module: wrapper for symbol name.
>  modules: fix longstanding /proc/kallsyms vs module insertion race.
>
> include/linux/module.h |  19 ++++----
> kernel/module.c        | 120 ++++++++++++++++++++++++++++++-------------------
> 2 files changed, 85 insertions(+), 54 deletions(-)
>
>--
>2.5.0