Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1326325 > unrolled thread

[RFC PATCH v4 2/6] module: preserve Elf information for livepatch modules

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC PATCH v4 2/6] module: preserve Elf information for livepatch modules Jessica Yu <jeyu@redhat.com> - 2016-02-04 02:20 +0100
    Re: [RFC PATCH v4 2/6] module: preserve Elf information for  livepatch modules Josh Poimboeuf <jpoimboe@redhat.com> - 2016-02-08 21:20 +0100
      Re: module: preserve Elf information for livepatch modules Jessica Yu <jeyu@redhat.com> - 2016-02-08 21:40 +0100
    Re: [RFC PATCH v4 2/6] module: preserve Elf information for  livepatch modules Petr Mladek <pmladek@suse.com> - 2016-02-09 09:50 +0100
      Re: [RFC PATCH v4 2/6] module: preserve Elf information for livepatch  modules Jiri Kosina <jikos@kernel.org> - 2016-02-09 11:40 +0100
        Re: [RFC PATCH v4 2/6] module: preserve Elf information for  livepatch modules Petr Mladek <pmladek@suse.com> - 2016-02-09 13:40 +0100
          Re: [RFC PATCH v4 2/6] module: preserve Elf information for livepatch modules Rusty Russell <rusty@rustcorp.com.au> - 2016-02-10 01:30 +0100
    Re: [RFC PATCH v4 2/6] module: preserve Elf information for  livepatch modules Petr Mladek <pmladek@suse.com> - 2016-02-10 17:00 +0100

#1326325 — [RFC PATCH v4 2/6] module: preserve Elf information for livepatch modules

For livepatch modules, copy Elf section, symbol, and string information
from the load_info struct in the module loader. Persist copies of the
original symbol table and string table.

Livepatch manages its own relocation sections in order to reuse module
loader code to write relocations. Livepatch modules must preserve Elf
information such as section indices in order to apply livepatch relocation
sections using the module loader's apply_relocate_add() function.

In order to apply livepatch relocation sections, livepatch modules must
keep a complete copy of their original symbol table in memory. Normally, a
stripped down copy of a module's symbol table (containing only "core"
symbols) is made available through module->core_symtab. But for livepatch
modules, the symbol table copied into memory on module load must be exactly
the same as the symbol table produced when the patch module was compiled.
This is because the relocations in each livepatch relocation section refer
to their respective symbols with their symbol indices, and the original
symbol indices (and thus the symtab ordering) must be preserved in order
for apply_relocate_add() to find the right symbol.

Signed-off-by: Jessica Yu <jeyu@redhat.com>
---
 include/linux/module.h |  25 ++++++++++
 kernel/module.c        | 133 ++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 151 insertions(+), 7 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 4560d8f..58e6200 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -324,6 +324,15 @@ struct module_layout {
 #define __module_layout_align
 #endif
 
+#ifdef CONFIG_LIVEPATCH
+struct klp_modinfo {
+	Elf_Ehdr hdr;
+	Elf_Shdr *sechdrs;
+	char *secstrings;
+	unsigned int symndx;
+};
+#endif
+
 struct module {
 	enum module_state state;
 
@@ -455,7 +464,11 @@ struct module {
 #endif
 
 #ifdef CONFIG_LIVEPATCH
+	bool klp; /* Is this a livepatch module? */
 	bool klp_alive;
+
+	/* Elf information */
+	struct klp_modinfo *klp_info;
 #endif
 
 #ifdef CONFIG_MODULE_UNLOAD
@@ -629,6 +642,18 @@ static inline bool module_requested_async_probing(struct module *module)
 	return module && module->async_probe_requested;
 }
 
+#ifdef CONFIG_LIVEPATCH
+static inline bool is_livepatch_module(struct module *mod)
+{
+	return mod->klp;
+}
+#else /* !CONFIG_LIVEPATCH */
+static inline bool is_livepatch_module(struct module *mod)
+{
+	return false;
+}
+#endif /* CONFIG_LIVEPATCH */
+
 #else /* !CONFIG_MODULES... */
 
 /* Given an address, look for it in the exception tables. */
diff --git a/kernel/module.c b/kernel/module.c
index 71c77ed..9c16eb2 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1970,6 +1970,82 @@ static void module_enable_nx(const struct module *mod) { }
 static void module_disable_nx(const struct module *mod) { }
 #endif
 
+#ifdef CONFIG_LIVEPATCH
+/*
+ * Persist Elf information about a module. Copy the Elf header,
+ * section header table, section string table, and symtab section
+ * index from info to mod->klp_info.
+ */
+static int copy_module_elf(struct module *mod, struct load_info *info)
+{
+	unsigned int size, symndx;
+	int ret = 0;
+
+	size = sizeof(*mod->klp_info);
+	mod->klp_info = kmalloc(size, GFP_KERNEL);
+	if (mod->klp_info == NULL)
+		return -ENOMEM;
+
+	/* Elf header */
+	size = sizeof(Elf_Ehdr);
+	memcpy(&mod->klp_info->hdr, info->hdr, size);
+
+	/* Elf section header table */
+	size = sizeof(Elf_Shdr) * info->hdr->e_shnum;
+	mod->klp_info->sechdrs = kmalloc(size, GFP_KERNEL);
+	if (mod->klp_info->sechdrs == NULL) {
+		ret = -ENOMEM;
+		goto free_info;
+	}
+	memcpy(mod->klp_info->sechdrs, info->sechdrs, size);
+
+	/* Elf section name string table */
+	size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
+	mod->klp_info->secstrings = kmalloc(size, GFP_KERNEL);
+	if (mod->klp_info->secstrings == NULL) {
+		ret = -ENOMEM;
+		goto free_sechdrs;
+	}
+	memcpy(mod->klp_info->secstrings, info->secstrings, size);
+
+	/* Elf symbol section index */
+	symndx = info->index.sym;
+	mod->klp_info->symndx = symndx;
+
+	/*
+	 * For livepatch modules, core_symtab is a complete copy
+	 * of the original symbol table. Adjust sh_addr to point
+	 * to core_symtab since the copy of the symtab in module
+	 * init memory is freed at the end of do_init_module().
+	 */
+	mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long) mod->core_symtab;
+
+	return ret;
+
+free_sechdrs:
+	kfree(mod->klp_info->sechdrs);
+free_info:
+	kfree(mod->klp_info);
+	return ret;
+}
+
+static void free_module_elf(struct module *mod)
+{
+	kfree(mod->klp_info->sechdrs);
+	kfree(mod->klp_info->secstrings);
+	kfree(mod->klp_info);
+}
+#else /* !CONFIG_LIVEPATCH */
+static int copy_module_elf(struct module *mod, struct load_info *info)
+{
+	return 0;
+}
+
+static void free_module_elf(struct module *mod)
+{
+}
+#endif /* CONFIG_LIVEPATCH */
+
 void __weak module_memfree(void *module_region)
 {
 	vfree(module_region);
@@ -2008,6 +2084,10 @@ static void free_module(struct module *mod)
 	/* Free any allocated parameters. */
 	destroy_params(mod->kp, mod->num_kp);
 
+	/* Free Elf information if it was saved */
+	if (is_livepatch_module(mod))
+		free_module_elf(mod);
+
 	/* Now we can delete it from the lists */
 	mutex_lock(&module_mutex);
 	/* Unlink carefully: kallsyms could be walking list. */
@@ -2123,6 +2203,10 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
 			       (long)sym[i].st_value);
 			break;
 
+		case SHN_LIVEPATCH:
+			/* Livepatch symbols are resolved by livepatch */
+			break;
+
 		case SHN_UNDEF:
 			ksym = resolve_symbol_wait(mod, info, name);
 			/* Ok if resolved.  */
@@ -2171,6 +2255,10 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
 		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
 			continue;
 
+		/* Livepatch relocation sections are applied by livepatch */
+		if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
+			continue;
+
 		if (info->sechdrs[i].sh_type == SHT_REL)
 			err = apply_relocate(info->sechdrs, info->strtab,
 					     info->index.sym, i, mod);
@@ -2466,7 +2554,7 @@ static void layout_symtab(struct module *mod, struct load_info *info)
 
 	/* Compute total space required for the core symbols' strtab. */
 	for (ndst = i = 0; i < nsrc; i++) {
-		if (i == 0 ||
+		if (i == 0 || is_livepatch_module(mod) ||
 		    is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum,
 				   info->index.pcpu)) {
 			strtab_size += strlen(&info->strtab[src[i].st_name])+1;
@@ -2509,7 +2597,7 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
 	mod->core_strtab = s = mod->core_layout.base + info->stroffs;
 	src = mod->symtab;
 	for (ndst = i = 0; i < mod->num_symtab; i++) {
-		if (i == 0 ||
+		if (i == 0 || is_livepatch_module(mod) ||
 		    is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum,
 				   info->index.pcpu)) {
 			dst[ndst] = src[i];
@@ -2676,6 +2764,23 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
 	return 0;
 }
 
+#ifdef CONFIG_LIVEPATCH
+static int find_livepatch_modinfo(struct module *mod, struct load_info *info)
+{
+	mod->klp = get_modinfo(info, "livepatch") ? true : false;
+
+	return 0;
+}
+#else /* !CONFIG_LIVEPATCH */
+static int find_livepatch_modinfo(struct module *mod, struct load_info *info)
+{
+	if (get_modinfo(info, "livepatch"))
+		return -ENOEXEC;
+
+	return 0;
+}
+#endif /* CONFIG_LIVEPATCH */
+
 /* Sets info->hdr and info->len. */
 static int copy_module_from_fd(int fd, struct load_info *info)
 {
@@ -2859,6 +2964,10 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
 			"is unknown, you have been warned.\n", mod->name);
 	}
 
+	err = find_livepatch_modinfo(mod, info);
+	if (err)
+		return err;
+
 	/* Set up license info based on the info section */
 	set_license(mod, get_modinfo(info, "license"));
 
@@ -3222,6 +3331,12 @@ static noinline int do_init_module(struct module *mod)
 	 */
 	current->flags &= ~PF_USED_ASYNC;
 
+#ifdef CONFIG_KALLSYMS
+	/* Make symtab and strtab available prior to module init call */
+	mod->num_symtab = mod->core_num_syms;
+	mod->symtab = mod->core_symtab;
+	mod->strtab = mod->core_strtab;
+#endif
 	do_mod_ctors(mod);
 	/* Start the module */
 	if (mod->init != NULL)
@@ -3266,11 +3381,6 @@ static noinline int do_init_module(struct module *mod)
 	/* Drop initial reference. */
 	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;
-#endif
 	mod_tree_remove_init(mod);
 	disable_ro_nx(&mod->init_layout);
 	module_arch_freeing_init(mod);
@@ -3522,6 +3632,13 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	if (err < 0)
 		goto bug_cleanup;
 
+	/* For livepatch modules, save Elf info from load_info struct */
+	if (is_livepatch_module(mod)) {
+		err = copy_module_elf(mod, info);
+		if (err < 0)
+			goto sysfs_cleanup;
+	}
+
 	/* Get rid of temporary copy. */
 	free_copy(info);
 
@@ -3530,6 +3647,8 @@ static int load_module(struct load_info *info, const char __user *uargs,
 
 	return do_init_module(mod);
 
+ sysfs_cleanup:
+	mod_sysfs_teardown(mod);
  bug_cleanup:
 	/* module_bug_cleanup needs module_mutex protection */
 	mutex_lock(&module_mutex);
-- 
2.4.3

[toc] | [next] | [standalone]

#1329522 — Re: [RFC PATCH v4 2/6] module: preserve Elf information for livepatch modules

On Wed, Feb 03, 2016 at 08:11:07PM -0500, Jessica Yu wrote:
> For livepatch modules, copy Elf section, symbol, and string information
> from the load_info struct in the module loader. Persist copies of the
> original symbol table and string table.
> 
> Livepatch manages its own relocation sections in order to reuse module
> loader code to write relocations. Livepatch modules must preserve Elf
> information such as section indices in order to apply livepatch relocation
> sections using the module loader's apply_relocate_add() function.
> 
> In order to apply livepatch relocation sections, livepatch modules must
> keep a complete copy of their original symbol table in memory. Normally, a
> stripped down copy of a module's symbol table (containing only "core"
> symbols) is made available through module->core_symtab. But for livepatch
> modules, the symbol table copied into memory on module load must be exactly
> the same as the symbol table produced when the patch module was compiled.
> This is because the relocations in each livepatch relocation section refer
> to their respective symbols with their symbol indices, and the original
> symbol indices (and thus the symtab ordering) must be preserved in order
> for apply_relocate_add() to find the right symbol.

This patch didn't apply clean to linux-next/master.  I didn't
investigate why, but maybe it depends on the other patch set which
removes the notifiers?  (If so, that should be mentioned in the cover
letter.)

A couple of minor comments below...

> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
>  include/linux/module.h |  25 ++++++++++
>  kernel/module.c        | 133 ++++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 151 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 4560d8f..58e6200 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -324,6 +324,15 @@ struct module_layout {
>  #define __module_layout_align
>  #endif
>  
> +#ifdef CONFIG_LIVEPATCH
> +struct klp_modinfo {
> +	Elf_Ehdr hdr;
> +	Elf_Shdr *sechdrs;
> +	char *secstrings;
> +	unsigned int symndx;
> +};
> +#endif
> +
>  struct module {
>  	enum module_state state;
>  
> @@ -455,7 +464,11 @@ struct module {
>  #endif
>  
>  #ifdef CONFIG_LIVEPATCH
> +	bool klp; /* Is this a livepatch module? */
>  	bool klp_alive;
> +
> +	/* Elf information */
> +	struct klp_modinfo *klp_info;
>  #endif
>  
>  #ifdef CONFIG_MODULE_UNLOAD
> @@ -629,6 +642,18 @@ static inline bool module_requested_async_probing(struct module *module)
>  	return module && module->async_probe_requested;
>  }
>  
> +#ifdef CONFIG_LIVEPATCH
> +static inline bool is_livepatch_module(struct module *mod)
> +{
> +	return mod->klp;
> +}
> +#else /* !CONFIG_LIVEPATCH */
> +static inline bool is_livepatch_module(struct module *mod)
> +{
> +	return false;
> +}
> +#endif /* CONFIG_LIVEPATCH */
> +
>  #else /* !CONFIG_MODULES... */
>  
>  /* Given an address, look for it in the exception tables. */
> diff --git a/kernel/module.c b/kernel/module.c
> index 71c77ed..9c16eb2 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1970,6 +1970,82 @@ static void module_enable_nx(const struct module *mod) { }
>  static void module_disable_nx(const struct module *mod) { }
>  #endif
>  
> +#ifdef CONFIG_LIVEPATCH
> +/*
> + * Persist Elf information about a module. Copy the Elf header,
> + * section header table, section string table, and symtab section
> + * index from info to mod->klp_info.
> + */
> +static int copy_module_elf(struct module *mod, struct load_info *info)
> +{
> +	unsigned int size, symndx;
> +	int ret = 0;
> +
> +	size = sizeof(*mod->klp_info);
> +	mod->klp_info = kmalloc(size, GFP_KERNEL);
> +	if (mod->klp_info == NULL)
> +		return -ENOMEM;
> +
> +	/* Elf header */
> +	size = sizeof(Elf_Ehdr);
> +	memcpy(&mod->klp_info->hdr, info->hdr, size);
> +
> +	/* Elf section header table */
> +	size = sizeof(Elf_Shdr) * info->hdr->e_shnum;
> +	mod->klp_info->sechdrs = kmalloc(size, GFP_KERNEL);
> +	if (mod->klp_info->sechdrs == NULL) {
> +		ret = -ENOMEM;
> +		goto free_info;
> +	}
> +	memcpy(mod->klp_info->sechdrs, info->sechdrs, size);
> +
> +	/* Elf section name string table */
> +	size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
> +	mod->klp_info->secstrings = kmalloc(size, GFP_KERNEL);
> +	if (mod->klp_info->secstrings == NULL) {
> +		ret = -ENOMEM;
> +		goto free_sechdrs;
> +	}
> +	memcpy(mod->klp_info->secstrings, info->secstrings, size);
> +
> +	/* Elf symbol section index */
> +	symndx = info->index.sym;
> +	mod->klp_info->symndx = symndx;
> +
> +	/*
> +	 * For livepatch modules, core_symtab is a complete copy
> +	 * of the original symbol table. Adjust sh_addr to point
> +	 * to core_symtab since the copy of the symtab in module
> +	 * init memory is freed at the end of do_init_module().
> +	 */
> +	mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long) mod->core_symtab;
> +
> +	return ret;
> +
> +free_sechdrs:
> +	kfree(mod->klp_info->sechdrs);
> +free_info:
> +	kfree(mod->klp_info);
> +	return ret;
> +}
> +
> +static void free_module_elf(struct module *mod)
> +{
> +	kfree(mod->klp_info->sechdrs);
> +	kfree(mod->klp_info->secstrings);
> +	kfree(mod->klp_info);
> +}
> +#else /* !CONFIG_LIVEPATCH */
> +static int copy_module_elf(struct module *mod, struct load_info *info)
> +{
> +	return 0;
> +}
> +
> +static void free_module_elf(struct module *mod)
> +{
> +}
> +#endif /* CONFIG_LIVEPATCH */
> +
>  void __weak module_memfree(void *module_region)
>  {
>  	vfree(module_region);
> @@ -2008,6 +2084,10 @@ static void free_module(struct module *mod)
>  	/* Free any allocated parameters. */
>  	destroy_params(mod->kp, mod->num_kp);
>  
> +	/* Free Elf information if it was saved */
> +	if (is_livepatch_module(mod))
> +		free_module_elf(mod);
> +

I think this code is self-evident, so the comment isn't necessary.


>  	/* Now we can delete it from the lists */
>  	mutex_lock(&module_mutex);
>  	/* Unlink carefully: kallsyms could be walking list. */
> @@ -2123,6 +2203,10 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
>  			       (long)sym[i].st_value);
>  			break;
>  
> +		case SHN_LIVEPATCH:
> +			/* Livepatch symbols are resolved by livepatch */
> +			break;
> +
>  		case SHN_UNDEF:
>  			ksym = resolve_symbol_wait(mod, info, name);
>  			/* Ok if resolved.  */
> @@ -2171,6 +2255,10 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
>  		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
>  			continue;
>  
> +		/* Livepatch relocation sections are applied by livepatch */
> +		if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
> +			continue;
> +
>  		if (info->sechdrs[i].sh_type == SHT_REL)
>  			err = apply_relocate(info->sechdrs, info->strtab,
>  					     info->index.sym, i, mod);
> @@ -2466,7 +2554,7 @@ static void layout_symtab(struct module *mod, struct load_info *info)
>  
>  	/* Compute total space required for the core symbols' strtab. */
>  	for (ndst = i = 0; i < nsrc; i++) {
> -		if (i == 0 ||
> +		if (i == 0 || is_livepatch_module(mod) ||
>  		    is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum,
>  				   info->index.pcpu)) {
>  			strtab_size += strlen(&info->strtab[src[i].st_name])+1;
> @@ -2509,7 +2597,7 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
>  	mod->core_strtab = s = mod->core_layout.base + info->stroffs;
>  	src = mod->symtab;
>  	for (ndst = i = 0; i < mod->num_symtab; i++) {
> -		if (i == 0 ||
> +		if (i == 0 || is_livepatch_module(mod) ||
>  		    is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum,
>  				   info->index.pcpu)) {
>  			dst[ndst] = src[i];
> @@ -2676,6 +2764,23 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
>  	return 0;
>  }
>  
> +#ifdef CONFIG_LIVEPATCH
> +static int find_livepatch_modinfo(struct module *mod, struct load_info *info)
> +{
> +	mod->klp = get_modinfo(info, "livepatch") ? true : false;
> +
> +	return 0;
> +}
> +#else /* !CONFIG_LIVEPATCH */
> +static int find_livepatch_modinfo(struct module *mod, struct load_info *info)
> +{
> +	if (get_modinfo(info, "livepatch"))
> +		return -ENOEXEC;
> +
> +	return 0;
> +}
> +#endif /* CONFIG_LIVEPATCH */
> +
>  /* Sets info->hdr and info->len. */
>  static int copy_module_from_fd(int fd, struct load_info *info)
>  {
> @@ -2859,6 +2964,10 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
>  			"is unknown, you have been warned.\n", mod->name);
>  	}
>  
> +	err = find_livepatch_modinfo(mod, info);
> +	if (err)
> +		return err;
> +
>  	/* Set up license info based on the info section */
>  	set_license(mod, get_modinfo(info, "license"));
>  
> @@ -3222,6 +3331,12 @@ static noinline int do_init_module(struct module *mod)
>  	 */
>  	current->flags &= ~PF_USED_ASYNC;
>  
> +#ifdef CONFIG_KALLSYMS
> +	/* Make symtab and strtab available prior to module init call */
> +	mod->num_symtab = mod->core_num_syms;
> +	mod->symtab = mod->core_symtab;
> +	mod->strtab = mod->core_strtab;
> +#endif
>  	do_mod_ctors(mod);
>  	/* Start the module */
>  	if (mod->init != NULL)
> @@ -3266,11 +3381,6 @@ static noinline int do_init_module(struct module *mod)
>  	/* Drop initial reference. */
>  	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;
> -#endif
>  	mod_tree_remove_init(mod);
>  	disable_ro_nx(&mod->init_layout);
>  	module_arch_freeing_init(mod);
> @@ -3522,6 +3632,13 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	if (err < 0)
>  		goto bug_cleanup;
>  
> +	/* For livepatch modules, save Elf info from load_info struct */
> +	if (is_livepatch_module(mod)) {
> +		err = copy_module_elf(mod, info);
> +		if (err < 0)
> +			goto sysfs_cleanup;
> +	}
> +

Same here, unecessary comment IMO.

>  	/* Get rid of temporary copy. */
>  	free_copy(info);
>  
> @@ -3530,6 +3647,8 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  
>  	return do_init_module(mod);
>  
> + sysfs_cleanup:
> +	mod_sysfs_teardown(mod);
>   bug_cleanup:
>  	/* module_bug_cleanup needs module_mutex protection */
>  	mutex_lock(&module_mutex);
> -- 
> 2.4.3
> 

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1329545 — Re: module: preserve Elf information for livepatch modules

+++ Josh Poimboeuf [08/02/16 14:10 -0600]:
>On Wed, Feb 03, 2016 at 08:11:07PM -0500, Jessica Yu wrote:
>> For livepatch modules, copy Elf section, symbol, and string information
>> from the load_info struct in the module loader. Persist copies of the
>> original symbol table and string table.
>>
>> Livepatch manages its own relocation sections in order to reuse module
>> loader code to write relocations. Livepatch modules must preserve Elf
>> information such as section indices in order to apply livepatch relocation
>> sections using the module loader's apply_relocate_add() function.
>>
>> In order to apply livepatch relocation sections, livepatch modules must
>> keep a complete copy of their original symbol table in memory. Normally, a
>> stripped down copy of a module's symbol table (containing only "core"
>> symbols) is made available through module->core_symtab. But for livepatch
>> modules, the symbol table copied into memory on module load must be exactly
>> the same as the symbol table produced when the patch module was compiled.
>> This is because the relocations in each livepatch relocation section refer
>> to their respective symbols with their symbol indices, and the original
>> symbol indices (and thus the symtab ordering) must be preserved in order
>> for apply_relocate_add() to find the right symbol.
>
>This patch didn't apply clean to linux-next/master.  I didn't
>investigate why, but maybe it depends on the other patch set which
>removes the notifiers?  (If so, that should be mentioned in the cover
>letter.)

A very recent commit (8244062ef) introduced some changes
kernel/module.c that intersect with this patch, so I think this is why
the patch didn't apply cleanly to today's tree...Anyhow I will rebase
again before sending out v5. Thanks for the comments :-)

Jessica

[toc] | [prev] | [next] | [standalone]

#1329966 — Re: [RFC PATCH v4 2/6] module: preserve Elf information for livepatch modules

On Wed 2016-02-03 20:11:07, Jessica Yu wrote:
> For livepatch modules, copy Elf section, symbol, and string information
> from the load_info struct in the module loader. Persist copies of the
> original symbol table and string table.
> 
> Livepatch manages its own relocation sections in order to reuse module
> loader code to write relocations. Livepatch modules must preserve Elf
> information such as section indices in order to apply livepatch relocation
> sections using the module loader's apply_relocate_add() function.
> 
> In order to apply livepatch relocation sections, livepatch modules must
> keep a complete copy of their original symbol table in memory. Normally, a
> stripped down copy of a module's symbol table (containing only "core"
> symbols) is made available through module->core_symtab. But for livepatch
> modules, the symbol table copied into memory on module load must be exactly
> the same as the symbol table produced when the patch module was compiled.
> This is because the relocations in each livepatch relocation section refer
> to their respective symbols with their symbol indices, and the original
> symbol indices (and thus the symtab ordering) must be preserved in order
> for apply_relocate_add() to find the right symbol.
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 71c77ed..9c16eb2 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3222,6 +3331,12 @@ static noinline int do_init_module(struct module *mod)
>  	 */
>  	current->flags &= ~PF_USED_ASYNC;
>  
> +#ifdef CONFIG_KALLSYMS
> +	/* Make symtab and strtab available prior to module init call */
> +	mod->num_symtab = mod->core_num_syms;
> +	mod->symtab = mod->core_symtab;
> +	mod->strtab = mod->core_strtab;
> +#endif

This should be done with module_mutex. Otherwise, it looks racy
at least against module_kallsyms_on_each_symbol().

BTW: I wonder why even the original code is not racy
for example against module_get_kallsym. It is called
without the mutex. This code sets the number of entries
before the pointer to the entries.

Note that the module is in the list even in the UNFORMED state.


>  	do_mod_ctors(mod);
>  	/* Start the module */
>  	if (mod->init != NULL)
> @@ -3266,11 +3381,6 @@ static noinline int do_init_module(struct module *mod)
>  	/* Drop initial reference. */
>  	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;
> -#endif
>  	mod_tree_remove_init(mod);
>  	disable_ro_nx(&mod->init_layout);
>  	module_arch_freeing_init(mod);

In each case, it was called with the mutex here.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1330060 — Re: [RFC PATCH v4 2/6] module: preserve Elf information for livepatch modules

On Tue, 9 Feb 2016, Petr Mladek wrote:

> > +#ifdef CONFIG_KALLSYMS
> > +	/* Make symtab and strtab available prior to module init call */
> > +	mod->num_symtab = mod->core_num_syms;
> > +	mod->symtab = mod->core_symtab;
> > +	mod->strtab = mod->core_strtab;
> > +#endif
> 
> This should be done with module_mutex. Otherwise, it looks racy at least 
> against module_kallsyms_on_each_symbol().

Hmm, if this is the case, the comment describing the locking rules for 
module_mutex should be updated.

> BTW: I wonder why even the original code is not racy for example against 
> module_get_kallsym. It is called without the mutex. This code sets the 
> number of entries before the pointer to the entries.
> 
> Note that the module is in the list even in the UNFORMED state.

module_kallsyms_on_each_symbol() gives up in such case though.

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1330197 — Re: [RFC PATCH v4 2/6] module: preserve Elf information for livepatch modules

On Tue 2016-02-09 11:33:07, Jiri Kosina wrote:
> On Tue, 9 Feb 2016, Petr Mladek wrote:
> 
> > > +#ifdef CONFIG_KALLSYMS
> > > +	/* Make symtab and strtab available prior to module init call */
> > > +	mod->num_symtab = mod->core_num_syms;
> > > +	mod->symtab = mod->core_symtab;
> > > +	mod->strtab = mod->core_strtab;
> > > +#endif
> > 
> > This should be done with module_mutex. Otherwise, it looks racy at least 
> > against module_kallsyms_on_each_symbol().
> 
> Hmm, if this is the case, the comment describing the locking rules for 
> module_mutex should be updated.
> 
> > BTW: I wonder why even the original code is not racy for example against 
> > module_get_kallsym. It is called without the mutex. This code sets the 
> > number of entries before the pointer to the entries.
> > 
> > Note that the module is in the list even in the UNFORMED state.
> 
> module_kallsyms_on_each_symbol() gives up in such case though.

The problem is that we set the three values above in the COMMING
state. They were even set in the LIVE state before this patch.

IMHO, we should reorder the writes and add a write barrier.


#ifdef CONFIG_KALLSYMS
	/* Make symtab and strtab available prior to module init call */
	mod->strtab = mod->core_strtab;
	mod->symtab = mod->core_symtab;
	/* Tables must be availabe before the number is set */
	smp_wmb();
	mod->num_symtab = mod->core_num_syms;
#endif


Then we should add the corresponding read barrier to
the read functions that are protected only by
the disabled preeption. For example:


static unsigned long mod_find_symname(struct module *mod, const char *name)
{
	unsigned int i;

	for (i = 0; i < mod->num_symtab; i++)
		/* Make sure that tables are set for the read num_symtab */
		smp_rmb();
		if (strcmp(name, mod->strtab+mod->symtab[i].st_name) == 0 &&
		    mod->symtab[i].st_info != 'U')
			return mod->symtab[i].st_value;
	return 0;
}


Or am I too paranoid, please?

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1330783

Petr Mladek <pmladek@suse.com> writes:
> On Tue 2016-02-09 11:33:07, Jiri Kosina wrote:
>> On Tue, 9 Feb 2016, Petr Mladek wrote:
>> 
>> > > +#ifdef CONFIG_KALLSYMS
>> > > +	/* Make symtab and strtab available prior to module init call */
>> > > +	mod->num_symtab = mod->core_num_syms;
>> > > +	mod->symtab = mod->core_symtab;
>> > > +	mod->strtab = mod->core_strtab;
>> > > +#endif
>> > 
>> > This should be done with module_mutex. Otherwise, it looks racy at least 
>> > against module_kallsyms_on_each_symbol().
>> 
>> Hmm, if this is the case, the comment describing the locking rules for 
>> module_mutex should be updated.
>> 
>> > BTW: I wonder why even the original code is not racy for example against 
>> > module_get_kallsym. It is called without the mutex. This code sets the 
>> > number of entries before the pointer to the entries.
>> > 
>> > Note that the module is in the list even in the UNFORMED state.
>> 
>> module_kallsyms_on_each_symbol() gives up in such case though.
>
> The problem is that we set the three values above in the COMMING
> state. They were even set in the LIVE state before this patch.

True.  Indeed, I have a fix for exactly this queued in my fixes tree,
and am about to send Linus a pull req.

Below is the fix I went with, for your reference (part of a series, but
you get the idea).

Thanks!
Rusty.

commit 8244062ef1e54502ef55f54cced659913f244c3e
Author: Rusty Russell <rusty@rustcorp.com.au>
Date:   Wed Feb 3 16:55:26 2016 +1030

    modules: fix longstanding /proc/kallsyms vs module insertion race.
    
    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>

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;
 		}

[toc] | [prev] | [next] | [standalone]

#1331285 — Re: [RFC PATCH v4 2/6] module: preserve Elf information for livepatch modules

On Wed 2016-02-03 20:11:07, Jessica Yu wrote:
> diff --git a/kernel/module.c b/kernel/module.c
> index 71c77ed..9c16eb2 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2676,6 +2764,23 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
>  	return 0;
>  }
>  
> +#ifdef CONFIG_LIVEPATCH
> +static int find_livepatch_modinfo(struct module *mod, struct load_info *info)
> +{
> +	mod->klp = get_modinfo(info, "livepatch") ? true : false;
> +
> +	return 0;
> +}
> +#else /* !CONFIG_LIVEPATCH */
> +static int find_livepatch_modinfo(struct module *mod, struct load_info *info)
> +{
> +	if (get_modinfo(info, "livepatch"))

One more thing. I suggest to write a friendly message here. For example:

	pr_err("LifePatch support is not available. Rejecting the module: s\n",
	       mod->name);

It might safe some debugging a poor user or developer.

> +		return -ENOEXEC;
> +
> +	return 0;
> +}
> +#endif /* CONFIG_LIVEPATCH */
> +

Best Regards,
Petr

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web