Re: [PATCH v6 2/5] module: preserve Elf information for livepatch modules

[Rusty Russell]

Jessica Yu  writes:
> 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 
> Reviewed-by: Miroslav Benes 
> ---
>  include/linux/module.h |  25 ++
>  kernel/module.c| 125 
> +++--
>  2 files changed, 147 insertions(+), 3 deletions(-)

Acked-by: Rusty Russell 

(I guess that takes precedence over "Reviewed-by", which I did too).

Thanks,
Rusty.

> diff --git a/include/linux/module.h b/include/linux/module.h
> index 2bb0c30..3daf2b3 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -330,6 +330,15 @@ struct mod_kallsyms {
>   char *strtab;
>  };
>  
> +#ifdef CONFIG_LIVEPATCH
> +struct klp_modinfo {
> + Elf_Ehdr hdr;
> + Elf_Shdr *sechdrs;
> + char *secstrings;
> + unsigned int symndx;
> +};
> +#endif
> +
>  struct module {
>   enum module_state state;
>  
> @@ -456,7 +465,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
> @@ -630,6 +643,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 041200c..5f71aa6 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1973,6 +1973,83 @@ 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;
> +
> + size = sizeof(*mod->klp_info);
> + mod->klp_info = kmalloc(size, GFP_KERNEL);
> + if (mod->klp_info == NULL)
> + return -ENOMEM;
> +
> + /* Elf header */
> + size = sizeof(mod->klp_info->hdr);
> + memcpy(>klp_info->hdr, info->hdr, size);
> +
> + /* Elf section header table */
> + size = sizeof(*info->sechdrs) * 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_kallsyms.symtab is a complete
> +  * copy of the original symbol table. Adjust sh_addr to point
> +  * to core_kallsyms.symtab since the copy of the symtab in module
> +  * init memory is freed at the end of 

Re: [PATCH v6 2/5] module: preserve Elf information for livepatch modules

[Rusty Russell]

Jessica Yu  writes:
> 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 
> Reviewed-by: Miroslav Benes 
> ---
>  include/linux/module.h |  25 ++
>  kernel/module.c| 125 
> +++--
>  2 files changed, 147 insertions(+), 3 deletions(-)

Acked-by: Rusty Russell 

(I guess that takes precedence over "Reviewed-by", which I did too).

Thanks,
Rusty.

> diff --git a/include/linux/module.h b/include/linux/module.h
> index 2bb0c30..3daf2b3 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -330,6 +330,15 @@ struct mod_kallsyms {
>   char *strtab;
>  };
>  
> +#ifdef CONFIG_LIVEPATCH
> +struct klp_modinfo {
> + Elf_Ehdr hdr;
> + Elf_Shdr *sechdrs;
> + char *secstrings;
> + unsigned int symndx;
> +};
> +#endif
> +
>  struct module {
>   enum module_state state;
>  
> @@ -456,7 +465,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
> @@ -630,6 +643,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 041200c..5f71aa6 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1973,6 +1973,83 @@ 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;
> +
> + size = sizeof(*mod->klp_info);
> + mod->klp_info = kmalloc(size, GFP_KERNEL);
> + if (mod->klp_info == NULL)
> + return -ENOMEM;
> +
> + /* Elf header */
> + size = sizeof(mod->klp_info->hdr);
> + memcpy(>klp_info->hdr, info->hdr, size);
> +
> + /* Elf section header table */
> + size = sizeof(*info->sechdrs) * 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_kallsyms.symtab is a complete
> +  * copy of the original symbol table. Adjust sh_addr to point
> +  * to core_kallsyms.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 = \
> +  

[PATCH v6 2/5] module: preserve Elf information for livepatch modules

[Jessica Yu]

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 
Reviewed-by: Miroslav Benes 
---
 include/linux/module.h |  25 ++
 kernel/module.c| 125 +++--
 2 files changed, 147 insertions(+), 3 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 2bb0c30..3daf2b3 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -330,6 +330,15 @@ struct mod_kallsyms {
char *strtab;
 };
 
+#ifdef CONFIG_LIVEPATCH
+struct klp_modinfo {
+   Elf_Ehdr hdr;
+   Elf_Shdr *sechdrs;
+   char *secstrings;
+   unsigned int symndx;
+};
+#endif
+
 struct module {
enum module_state state;
 
@@ -456,7 +465,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
@@ -630,6 +643,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 041200c..5f71aa6 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1973,6 +1973,83 @@ 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;
+
+   size = sizeof(*mod->klp_info);
+   mod->klp_info = kmalloc(size, GFP_KERNEL);
+   if (mod->klp_info == NULL)
+   return -ENOMEM;
+
+   /* Elf header */
+   size = sizeof(mod->klp_info->hdr);
+   memcpy(>klp_info->hdr, info->hdr, size);
+
+   /* Elf section header table */
+   size = sizeof(*info->sechdrs) * 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_kallsyms.symtab is a complete
+* copy of the original symbol table. Adjust sh_addr to point
+* to core_kallsyms.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_kallsyms.symtab;
+
+   return 0;
+
+free_sechdrs:
+   kfree(mod->klp_info->sechdrs);
+free_info:
+   kfree(mod->klp_info);
+   return ret;
+}
+
+static void free_module_elf(struct module *mod)
+{
+   

[PATCH v6 2/5] module: preserve Elf information for livepatch modules

[Jessica Yu]

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 
Reviewed-by: Miroslav Benes 
---
 include/linux/module.h |  25 ++
 kernel/module.c| 125 +++--
 2 files changed, 147 insertions(+), 3 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 2bb0c30..3daf2b3 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -330,6 +330,15 @@ struct mod_kallsyms {
char *strtab;
 };
 
+#ifdef CONFIG_LIVEPATCH
+struct klp_modinfo {
+   Elf_Ehdr hdr;
+   Elf_Shdr *sechdrs;
+   char *secstrings;
+   unsigned int symndx;
+};
+#endif
+
 struct module {
enum module_state state;
 
@@ -456,7 +465,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
@@ -630,6 +643,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 041200c..5f71aa6 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1973,6 +1973,83 @@ 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;
+
+   size = sizeof(*mod->klp_info);
+   mod->klp_info = kmalloc(size, GFP_KERNEL);
+   if (mod->klp_info == NULL)
+   return -ENOMEM;
+
+   /* Elf header */
+   size = sizeof(mod->klp_info->hdr);
+   memcpy(>klp_info->hdr, info->hdr, size);
+
+   /* Elf section header table */
+   size = sizeof(*info->sechdrs) * 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_kallsyms.symtab is a complete
+* copy of the original symbol table. Adjust sh_addr to point
+* to core_kallsyms.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_kallsyms.symtab;
+
+   return 0;
+
+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);
+   

Re: module: preserve Elf information for livepatch modules

[Josh Poimboeuf]

On Tue, Mar 22, 2016 at 01:57:01PM -0400, Jessica Yu wrote:
> +++ Josh Poimboeuf [21/03/16 09:06 -0500]:
> >On Wed, Mar 16, 2016 at 03:47:04PM -0400, 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.
> >>
> >>Signed-off-by: Jessica Yu 
> >>---
> >> include/linux/module.h |  25 ++
> >> kernel/module.c| 123 
> >> -
> >> 2 files changed, 146 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/include/linux/module.h b/include/linux/module.h
> >>index 2bb0c30..3daf2b3 100644
> >>--- a/include/linux/module.h
> >>+++ b/include/linux/module.h
> >>@@ -330,6 +330,15 @@ struct mod_kallsyms {
> >>char *strtab;
> >> };
> >>
> >>+#ifdef CONFIG_LIVEPATCH
> >>+struct klp_modinfo {
> >>+   Elf_Ehdr hdr;
> >>+   Elf_Shdr *sechdrs;
> >>+   char *secstrings;
> >>+   unsigned int symndx;
> >>+};
> >>+#endif
> >>+
> >> struct module {
> >>enum module_state state;
> >>
> >>@@ -456,7 +465,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
> >>@@ -630,6 +643,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 87cfeb2..80b7fd9 100644
> >>--- a/kernel/module.c
> >>+++ b/kernel/module.c
> >>@@ -1971,6 +1971,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;
> >>+
> >>+   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(>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;
> 
> >
> >nit: The 'symndx' local variable is superfluous.
> >
> 
> The symndx variable is just there to keep the line below from reaching
> an ugly length (although it is already 

Re: module: preserve Elf information for livepatch modules

[Josh Poimboeuf]

On Tue, Mar 22, 2016 at 01:57:01PM -0400, Jessica Yu wrote:
> +++ Josh Poimboeuf [21/03/16 09:06 -0500]:
> >On Wed, Mar 16, 2016 at 03:47:04PM -0400, 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.
> >>
> >>Signed-off-by: Jessica Yu 
> >>---
> >> include/linux/module.h |  25 ++
> >> kernel/module.c| 123 
> >> -
> >> 2 files changed, 146 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/include/linux/module.h b/include/linux/module.h
> >>index 2bb0c30..3daf2b3 100644
> >>--- a/include/linux/module.h
> >>+++ b/include/linux/module.h
> >>@@ -330,6 +330,15 @@ struct mod_kallsyms {
> >>char *strtab;
> >> };
> >>
> >>+#ifdef CONFIG_LIVEPATCH
> >>+struct klp_modinfo {
> >>+   Elf_Ehdr hdr;
> >>+   Elf_Shdr *sechdrs;
> >>+   char *secstrings;
> >>+   unsigned int symndx;
> >>+};
> >>+#endif
> >>+
> >> struct module {
> >>enum module_state state;
> >>
> >>@@ -456,7 +465,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
> >>@@ -630,6 +643,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 87cfeb2..80b7fd9 100644
> >>--- a/kernel/module.c
> >>+++ b/kernel/module.c
> >>@@ -1971,6 +1971,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;
> >>+
> >>+   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(>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;
> 
> >
> >nit: The 'symndx' local variable is superfluous.
> >
> 
> The symndx variable is just there to keep the line below from reaching
> an ugly length (although it is already quite long...)
> 
> 

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Josh Poimboeuf [21/03/16 09:06 -0500]:

On Wed, Mar 16, 2016 at 03:47:04PM -0400, 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.

Signed-off-by: Jessica Yu 
---
 include/linux/module.h |  25 ++
 kernel/module.c| 123 -
 2 files changed, 146 insertions(+), 2 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 2bb0c30..3daf2b3 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -330,6 +330,15 @@ struct mod_kallsyms {
char *strtab;
 };

+#ifdef CONFIG_LIVEPATCH
+struct klp_modinfo {
+   Elf_Ehdr hdr;
+   Elf_Shdr *sechdrs;
+   char *secstrings;
+   unsigned int symndx;
+};
+#endif
+
 struct module {
enum module_state state;

@@ -456,7 +465,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
@@ -630,6 +643,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 87cfeb2..80b7fd9 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1971,6 +1971,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;
+
+   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(>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;




nit: The 'symndx' local variable is superfluous.



The symndx variable is just there to keep the line below from reaching
an ugly length (although it is already quite long...)


+   mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long) 
mod->core_kallsyms.symtab;

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Josh Poimboeuf [21/03/16 09:06 -0500]:

On Wed, Mar 16, 2016 at 03:47:04PM -0400, 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.

Signed-off-by: Jessica Yu 
---
 include/linux/module.h |  25 ++
 kernel/module.c| 123 -
 2 files changed, 146 insertions(+), 2 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 2bb0c30..3daf2b3 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -330,6 +330,15 @@ struct mod_kallsyms {
char *strtab;
 };

+#ifdef CONFIG_LIVEPATCH
+struct klp_modinfo {
+   Elf_Ehdr hdr;
+   Elf_Shdr *sechdrs;
+   char *secstrings;
+   unsigned int symndx;
+};
+#endif
+
 struct module {
enum module_state state;

@@ -456,7 +465,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
@@ -630,6 +643,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 87cfeb2..80b7fd9 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1971,6 +1971,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;
+
+   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(>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;




nit: The 'symndx' local variable is superfluous.



The symndx variable is just there to keep the line below from reaching
an ugly length (although it is already quite long...)


+   mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long) 
mod->core_kallsyms.symtab;

Re: [PATCH v5 2/6] module: preserve Elf information for livepatch modules

[Petr Mladek]

On Wed 2016-03-16 15:47:04, 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.
> 
> Signed-off-by: Jessica Yu 
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1971,6 +1971,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;
> +
> + 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);

It seems that you are going to do one more respin. Please, use
the size of the struct member here:

size = sizeof(mod->klp_info->hdr);

> + memcpy(>klp_info->hdr, info->hdr, size);
> +
> + /* Elf section header table */
> + size = sizeof(Elf_Shdr) * info->hdr->e_shnum;

and here

size = sizeof(*info->sechdrs) * 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);

Best Regards,
Petr

Re: [PATCH v5 2/6] module: preserve Elf information for livepatch modules

[Petr Mladek]

On Wed 2016-03-16 15:47:04, 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.
> 
> Signed-off-by: Jessica Yu 
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1971,6 +1971,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;
> +
> + 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);

It seems that you are going to do one more respin. Please, use
the size of the struct member here:

size = sizeof(mod->klp_info->hdr);

> + memcpy(>klp_info->hdr, info->hdr, size);
> +
> + /* Elf section header table */
> + size = sizeof(Elf_Shdr) * info->hdr->e_shnum;

and here

size = sizeof(*info->sechdrs) * 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);

Best Regards,
Petr

Re: [PATCH v5 2/6] module: preserve Elf information for livepatch modules

[Josh Poimboeuf]

On Wed, Mar 16, 2016 at 03:47:04PM -0400, 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.
> 
> Signed-off-by: Jessica Yu 
> ---
>  include/linux/module.h |  25 ++
>  kernel/module.c| 123 
> -
>  2 files changed, 146 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 2bb0c30..3daf2b3 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -330,6 +330,15 @@ struct mod_kallsyms {
>   char *strtab;
>  };
>  
> +#ifdef CONFIG_LIVEPATCH
> +struct klp_modinfo {
> + Elf_Ehdr hdr;
> + Elf_Shdr *sechdrs;
> + char *secstrings;
> + unsigned int symndx;
> +};
> +#endif
> +
>  struct module {
>   enum module_state state;
>  
> @@ -456,7 +465,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
> @@ -630,6 +643,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 87cfeb2..80b7fd9 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1971,6 +1971,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;
> +
> + 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(>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;

nit: The 'symndx' local variable is superfluous.

> +
> + /*
> +  * For livepatch modules, core_symtab is a complete copy

s/core_symtab/core_kallsyms.symtab/ ?

> +  * 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) 
> 

Re: [PATCH v5 2/6] module: preserve Elf information for livepatch modules

[Josh Poimboeuf]

On Wed, Mar 16, 2016 at 03:47:04PM -0400, 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.
> 
> Signed-off-by: Jessica Yu 
> ---
>  include/linux/module.h |  25 ++
>  kernel/module.c| 123 
> -
>  2 files changed, 146 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 2bb0c30..3daf2b3 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -330,6 +330,15 @@ struct mod_kallsyms {
>   char *strtab;
>  };
>  
> +#ifdef CONFIG_LIVEPATCH
> +struct klp_modinfo {
> + Elf_Ehdr hdr;
> + Elf_Shdr *sechdrs;
> + char *secstrings;
> + unsigned int symndx;
> +};
> +#endif
> +
>  struct module {
>   enum module_state state;
>  
> @@ -456,7 +465,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
> @@ -630,6 +643,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 87cfeb2..80b7fd9 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1971,6 +1971,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;
> +
> + 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(>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;

nit: The 'symndx' local variable is superfluous.

> +
> + /*
> +  * For livepatch modules, core_symtab is a complete copy

s/core_symtab/core_kallsyms.symtab/ ?

> +  * 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_kallsyms.symtab;

-- 

Re: [PATCH v5 2/6] module: preserve Elf information for livepatch modules

[Miroslav Benes]

On Wed, 16 Mar 2016, 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.
> 
> Signed-off-by: Jessica Yu 

With a fix for a bug reported by kbuild test robot

Reviewed-by: Miroslav Benes 

Re: [PATCH v5 2/6] module: preserve Elf information for livepatch modules

[Miroslav Benes]

On Wed, 16 Mar 2016, 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.
> 
> Signed-off-by: Jessica Yu 

With a fix for a bug reported by kbuild test robot

Reviewed-by: Miroslav Benes 

Re: [PATCH v5 2/6] module: preserve Elf information for livepatch modules

[kbuild test robot]

Hi Jessica,

[auto build test WARNING on s390/features]
[also build test WARNING on v4.5 next-20160316]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improving the system]

url:
https://github.com/0day-ci/linux/commits/Jessica-Yu/mostly-Arch-independent-livepatch/20160317-035230
base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
config: xtensa-allyesconfig (attached as .config)
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=xtensa 

All warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:13:0,
from include/linux/list.h:8,
from include/linux/module.h:9,
from include/linux/moduleloader.h:5,
from kernel/module.c:20:
   kernel/module.c: In function 'find_livepatch_modinfo':
   kernel/module.c:2767:10: error: expected ')' before 'mod'
 mod->name);
 ^
   include/linux/printk.h:236:21: note: in definition of macro 'pr_fmt'
#define pr_fmt(fmt) fmt
^
   kernel/module.c:2766:3: note: in expansion of macro 'pr_err'
  pr_err("%s: module is marked as livepatch module, but livepatch support 
is disabled"
  ^
>> kernel/module.c:2767:10: warning: format '%s' expects a matching 'char *' 
>> argument [-Wformat=]
 mod->name);
 ^
   include/linux/printk.h:236:21: note: in definition of macro 'pr_fmt'
#define pr_fmt(fmt) fmt
^
   kernel/module.c:2766:3: note: in expansion of macro 'pr_err'
  pr_err("%s: module is marked as livepatch module, but livepatch support 
is disabled"
  ^

vim +2767 kernel/module.c

  2751  } while (len);
  2752  return 0;
  2753  }
  2754  
  2755  #ifdef CONFIG_LIVEPATCH
  2756  static int find_livepatch_modinfo(struct module *mod, struct load_info 
*info)
  2757  {
  2758  mod->klp = get_modinfo(info, "livepatch") ? true : false;
  2759  
  2760  return 0;
  2761  }
  2762  #else /* !CONFIG_LIVEPATCH */
  2763  static int find_livepatch_modinfo(struct module *mod, struct load_info 
*info)
  2764  {
  2765  if (get_modinfo(info, "livepatch")) {
  2766  pr_err("%s: module is marked as livepatch module, but 
livepatch support is disabled"
> 2767 mod->name);
  2768  return -ENOEXEC;
  2769  }
  2770  
  2771  return 0;
  2772  }
  2773  #endif /* CONFIG_LIVEPATCH */
  2774  
  2775  /* Sets info->hdr and info->len. */

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCH v5 2/6] module: preserve Elf information for livepatch modules

[kbuild test robot]

Hi Jessica,

[auto build test WARNING on s390/features]
[also build test WARNING on v4.5 next-20160316]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improving the system]

url:
https://github.com/0day-ci/linux/commits/Jessica-Yu/mostly-Arch-independent-livepatch/20160317-035230
base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
config: xtensa-allyesconfig (attached as .config)
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=xtensa 

All warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:13:0,
from include/linux/list.h:8,
from include/linux/module.h:9,
from include/linux/moduleloader.h:5,
from kernel/module.c:20:
   kernel/module.c: In function 'find_livepatch_modinfo':
   kernel/module.c:2767:10: error: expected ')' before 'mod'
 mod->name);
 ^
   include/linux/printk.h:236:21: note: in definition of macro 'pr_fmt'
#define pr_fmt(fmt) fmt
^
   kernel/module.c:2766:3: note: in expansion of macro 'pr_err'
  pr_err("%s: module is marked as livepatch module, but livepatch support 
is disabled"
  ^
>> kernel/module.c:2767:10: warning: format '%s' expects a matching 'char *' 
>> argument [-Wformat=]
 mod->name);
 ^
   include/linux/printk.h:236:21: note: in definition of macro 'pr_fmt'
#define pr_fmt(fmt) fmt
^
   kernel/module.c:2766:3: note: in expansion of macro 'pr_err'
  pr_err("%s: module is marked as livepatch module, but livepatch support 
is disabled"
  ^

vim +2767 kernel/module.c

  2751  } while (len);
  2752  return 0;
  2753  }
  2754  
  2755  #ifdef CONFIG_LIVEPATCH
  2756  static int find_livepatch_modinfo(struct module *mod, struct load_info 
*info)
  2757  {
  2758  mod->klp = get_modinfo(info, "livepatch") ? true : false;
  2759  
  2760  return 0;
  2761  }
  2762  #else /* !CONFIG_LIVEPATCH */
  2763  static int find_livepatch_modinfo(struct module *mod, struct load_info 
*info)
  2764  {
  2765  if (get_modinfo(info, "livepatch")) {
  2766  pr_err("%s: module is marked as livepatch module, but 
livepatch support is disabled"
> 2767 mod->name);
  2768  return -ENOEXEC;
  2769  }
  2770  
  2771  return 0;
  2772  }
  2773  #endif /* CONFIG_LIVEPATCH */
  2774  
  2775  /* Sets info->hdr and info->len. */

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

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

[Jessica Yu]

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 
---
 include/linux/module.h |  25 ++
 kernel/module.c| 123 -
 2 files changed, 146 insertions(+), 2 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 2bb0c30..3daf2b3 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -330,6 +330,15 @@ struct mod_kallsyms {
char *strtab;
 };
 
+#ifdef CONFIG_LIVEPATCH
+struct klp_modinfo {
+   Elf_Ehdr hdr;
+   Elf_Shdr *sechdrs;
+   char *secstrings;
+   unsigned int symndx;
+};
+#endif
+
 struct module {
enum module_state state;
 
@@ -456,7 +465,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
@@ -630,6 +643,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 87cfeb2..80b7fd9 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1971,6 +1971,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;
+
+   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(>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_kallsyms.symtab;
+
+   return 0;
+
+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 /* 

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

[Jessica Yu]

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 
---
 include/linux/module.h |  25 ++
 kernel/module.c| 123 -
 2 files changed, 146 insertions(+), 2 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 2bb0c30..3daf2b3 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -330,6 +330,15 @@ struct mod_kallsyms {
char *strtab;
 };
 
+#ifdef CONFIG_LIVEPATCH
+struct klp_modinfo {
+   Elf_Ehdr hdr;
+   Elf_Shdr *sechdrs;
+   char *secstrings;
+   unsigned int symndx;
+};
+#endif
+
 struct module {
enum module_state state;
 
@@ -456,7 +465,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
@@ -630,6 +643,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 87cfeb2..80b7fd9 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1971,6 +1971,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;
+
+   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(>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_kallsyms.symtab;
+
+   return 0;
+
+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 

Re: [PATCH v5 2/6] module: preserve Elf information for livepatch modules

[kbuild test robot]

Hi Jessica,

[auto build test ERROR on s390/features]
[also build test ERROR on v4.5 next-20160316]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improving the system]

url:
https://github.com/0day-ci/linux/commits/Jessica-Yu/mostly-Arch-independent-livepatch/20160317-035230
base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
config: x86_64-randconfig-x016-201611 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:13:0,
from include/linux/list.h:8,
from include/linux/module.h:9,
from include/linux/moduleloader.h:5,
from kernel/module.c:20:
   kernel/module.c: In function 'find_livepatch_modinfo':
>> kernel/module.c:2767:10: error: expected ')' before 'mod'
 mod->name);
 ^
   include/linux/printk.h:236:21: note: in definition of macro 'pr_fmt'
#define pr_fmt(fmt) fmt
^
>> kernel/module.c:2766:3: note: in expansion of macro 'pr_err'
  pr_err("%s: module is marked as livepatch module, but livepatch support 
is disabled"
  ^
   In file included from include/linux/printk.h:6:0,
from include/linux/kernel.h:13,
from include/linux/list.h:8,
from include/linux/module.h:9,
from include/linux/moduleloader.h:5,
from kernel/module.c:20:
>> include/linux/kern_levels.h:4:18: warning: format '%s' expects a matching 
>> 'char *' argument [-Wformat=]
#define KERN_SOH "\001"  /* ASCII Start Of Header */
 ^
   include/linux/kern_levels.h:10:18: note: in expansion of macro 'KERN_SOH'
#define KERN_ERR KERN_SOH "3" /* error conditions */
 ^
   include/linux/printk.h:252:9: note: in expansion of macro 'KERN_ERR'
 printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
^
>> kernel/module.c:2766:3: note: in expansion of macro 'pr_err'
  pr_err("%s: module is marked as livepatch module, but livepatch support 
is disabled"
  ^

vim +2767 kernel/module.c

  2760  return 0;
  2761  }
  2762  #else /* !CONFIG_LIVEPATCH */
  2763  static int find_livepatch_modinfo(struct module *mod, struct load_info 
*info)
  2764  {
  2765  if (get_modinfo(info, "livepatch")) {
> 2766  pr_err("%s: module is marked as livepatch module, but 
> livepatch support is disabled"
> 2767 mod->name);
  2768  return -ENOEXEC;
  2769  }
  2770  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCH v5 2/6] module: preserve Elf information for livepatch modules

[kbuild test robot]

Hi Jessica,

[auto build test ERROR on s390/features]
[also build test ERROR on v4.5 next-20160316]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improving the system]

url:
https://github.com/0day-ci/linux/commits/Jessica-Yu/mostly-Arch-independent-livepatch/20160317-035230
base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
config: x86_64-randconfig-x016-201611 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:13:0,
from include/linux/list.h:8,
from include/linux/module.h:9,
from include/linux/moduleloader.h:5,
from kernel/module.c:20:
   kernel/module.c: In function 'find_livepatch_modinfo':
>> kernel/module.c:2767:10: error: expected ')' before 'mod'
 mod->name);
 ^
   include/linux/printk.h:236:21: note: in definition of macro 'pr_fmt'
#define pr_fmt(fmt) fmt
^
>> kernel/module.c:2766:3: note: in expansion of macro 'pr_err'
  pr_err("%s: module is marked as livepatch module, but livepatch support 
is disabled"
  ^
   In file included from include/linux/printk.h:6:0,
from include/linux/kernel.h:13,
from include/linux/list.h:8,
from include/linux/module.h:9,
from include/linux/moduleloader.h:5,
from kernel/module.c:20:
>> include/linux/kern_levels.h:4:18: warning: format '%s' expects a matching 
>> 'char *' argument [-Wformat=]
#define KERN_SOH "\001"  /* ASCII Start Of Header */
 ^
   include/linux/kern_levels.h:10:18: note: in expansion of macro 'KERN_SOH'
#define KERN_ERR KERN_SOH "3" /* error conditions */
 ^
   include/linux/printk.h:252:9: note: in expansion of macro 'KERN_ERR'
 printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
^
>> kernel/module.c:2766:3: note: in expansion of macro 'pr_err'
  pr_err("%s: module is marked as livepatch module, but livepatch support 
is disabled"
  ^

vim +2767 kernel/module.c

  2760  return 0;
  2761  }
  2762  #else /* !CONFIG_LIVEPATCH */
  2763  static int find_livepatch_modinfo(struct module *mod, struct load_info 
*info)
  2764  {
  2765  if (get_modinfo(info, "livepatch")) {
> 2766  pr_err("%s: module is marked as livepatch module, but 
> livepatch support is disabled"
> 2767 mod->name);
  2768  return -ENOEXEC;
  2769  }
  2770  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCH v5 2/6] module: preserve Elf information for livepatch modules

[kbuild test robot]

Hi Jessica,

[auto build test WARNING on s390/features]
[also build test WARNING on v4.5 next-20160316]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improving the system]

url:
https://github.com/0day-ci/linux/commits/Jessica-Yu/mostly-Arch-independent-livepatch/20160317-035230
base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
config: openrisc-or1ksim_defconfig (attached as .config)
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=openrisc 

All warnings (new ones prefixed by >>):

   kernel/module.c: In function 'find_livepatch_modinfo':
   kernel/module.c:2766:3: error: expected ')' before 'mod'
>> kernel/module.c:2766:3: warning: too few arguments for format

vim +2766 kernel/module.c

  2750  len -= n;
  2751  } while (len);
  2752  return 0;
  2753  }
  2754  
  2755  #ifdef CONFIG_LIVEPATCH
  2756  static int find_livepatch_modinfo(struct module *mod, struct load_info 
*info)
  2757  {
  2758  mod->klp = get_modinfo(info, "livepatch") ? true : false;
  2759  
  2760  return 0;
  2761  }
  2762  #else /* !CONFIG_LIVEPATCH */
  2763  static int find_livepatch_modinfo(struct module *mod, struct load_info 
*info)
  2764  {
  2765  if (get_modinfo(info, "livepatch")) {
> 2766  pr_err("%s: module is marked as livepatch module, but 
> livepatch support is disabled"
  2767 mod->name);
  2768  return -ENOEXEC;
  2769  }
  2770  
  2771  return 0;
  2772  }
  2773  #endif /* CONFIG_LIVEPATCH */
  2774  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCH v5 2/6] module: preserve Elf information for livepatch modules

[kbuild test robot]

Hi Jessica,

[auto build test WARNING on s390/features]
[also build test WARNING on v4.5 next-20160316]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improving the system]

url:
https://github.com/0day-ci/linux/commits/Jessica-Yu/mostly-Arch-independent-livepatch/20160317-035230
base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
config: openrisc-or1ksim_defconfig (attached as .config)
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=openrisc 

All warnings (new ones prefixed by >>):

   kernel/module.c: In function 'find_livepatch_modinfo':
   kernel/module.c:2766:3: error: expected ')' before 'mod'
>> kernel/module.c:2766:3: warning: too few arguments for format

vim +2766 kernel/module.c

  2750  len -= n;
  2751  } while (len);
  2752  return 0;
  2753  }
  2754  
  2755  #ifdef CONFIG_LIVEPATCH
  2756  static int find_livepatch_modinfo(struct module *mod, struct load_info 
*info)
  2757  {
  2758  mod->klp = get_modinfo(info, "livepatch") ? true : false;
  2759  
  2760  return 0;
  2761  }
  2762  #else /* !CONFIG_LIVEPATCH */
  2763  static int find_livepatch_modinfo(struct module *mod, struct load_info 
*info)
  2764  {
  2765  if (get_modinfo(info, "livepatch")) {
> 2766  pr_err("%s: module is marked as livepatch module, but 
> livepatch support is disabled"
  2767 mod->name);
  2768  return -ENOEXEC;
  2769  }
  2770  
  2771  return 0;
  2772  }
  2773  #endif /* CONFIG_LIVEPATCH */
  2774  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Jessica Yu [16/03/16 15:47 -0400]:

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 
---
include/linux/module.h |  25 ++
kernel/module.c| 123 -
2 files changed, 146 insertions(+), 2 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 2bb0c30..3daf2b3 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -330,6 +330,15 @@ struct mod_kallsyms {
char *strtab;
};

+#ifdef CONFIG_LIVEPATCH
+struct klp_modinfo {
+   Elf_Ehdr hdr;
+   Elf_Shdr *sechdrs;
+   char *secstrings;
+   unsigned int symndx;
+};
+#endif
+
struct module {
enum module_state state;

@@ -456,7 +465,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
@@ -630,6 +643,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 87cfeb2..80b7fd9 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1971,6 +1971,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;
+
+   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(>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_kallsyms.symtab;
+
+   return 0;
+
+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);
+   

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Jessica Yu [16/03/16 15:47 -0400]:

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 
---
include/linux/module.h |  25 ++
kernel/module.c| 123 -
2 files changed, 146 insertions(+), 2 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 2bb0c30..3daf2b3 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -330,6 +330,15 @@ struct mod_kallsyms {
char *strtab;
};

+#ifdef CONFIG_LIVEPATCH
+struct klp_modinfo {
+   Elf_Ehdr hdr;
+   Elf_Shdr *sechdrs;
+   char *secstrings;
+   unsigned int symndx;
+};
+#endif
+
struct module {
enum module_state state;

@@ -456,7 +465,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
@@ -630,6 +643,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 87cfeb2..80b7fd9 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1971,6 +1971,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;
+
+   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(>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_kallsyms.symtab;
+
+   return 0;
+
+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 

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

[Petr Mladek]

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

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

[Petr Mladek]

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

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

[Rusty Russell]

Petr Mladek  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 
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 
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=111541
Cc: sta...@kernel.org
Signed-off-by: Rusty Russell 

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, >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 

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

[Petr Mladek]

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

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

[Jiri Kosina]

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

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

[Petr Mladek]

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(>init_layout);
>   module_arch_freeing_init(mod);

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

Best Regards,
Petr

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

[Jiri Kosina]

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

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

[Petr Mladek]

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(>init_layout);
>   module_arch_freeing_init(mod);

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

Best Regards,
Petr

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

[Petr Mladek]

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

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

[Rusty Russell]

Petr Mladek  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 
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 
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=111541
Cc: sta...@kernel.org
Signed-off-by: Rusty Russell 

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, >init_layout.size, strsect,
 info->index.str) | INIT_OFFSET_MASK;
-   mod->init_layout.size = debug_align(mod->init_layout.size);

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ 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

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

[Josh Poimboeuf]

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 
> ---
>  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(>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
> + 

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ 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

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

[Josh Poimboeuf]

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 
> ---
>  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(>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 

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

[Jessica Yu]

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 
---
 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(>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 /* 

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

[Jessica Yu]

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 
---
 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(>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);
+}

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Petr Mladek [17/12/15 17:26 +0100]:

On Mon 2015-11-30 23:21:15, Jessica Yu wrote:

For livepatch modules, copy Elf section, symbol, and string information
from the load_info struct in the module loader.

Livepatch uses special relocation sections in order to be able to patch
modules that are not yet loaded, as well as apply patches to the kernel
when the addresses of symbols cannot be determined at compile time (for
example, when kaslr is enabled). Livepatch modules must preserve Elf
information such as section indices in order to apply the remaining
relocation sections at the appropriate time (i.e. when the target module
loads).

Signed-off-by: Jessica Yu 
---
 include/linux/module.h |  9 +
 kernel/module.c| 98 --
 2 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 3a19c79..9b46256 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -425,6 +425,14 @@ struct module {

/* Notes attributes */
struct module_notes_attrs *notes_attrs;
+
+   /* Elf information (optionally saved) */
+   Elf_Ehdr *hdr;
+   Elf_Shdr *sechdrs;
+   char *secstrings;
+   struct {
+   unsigned int sym, str, mod, vers, info, pcpu;
+   } index;
 #endif


I would hide this into a structure. It is 3 pointers and 6 integers
that are mostly unused. I think about using a pointer to struct load_info
here. We could set the unused stuff to zero and NULL.

Any better idea how to share the definition with struct load_info
is welcome.


Sure. If we want to encapsulate all this information, we can perhaps
replace this with a single pointer to a copy of load_info, and maybe
move the struct definition of load_info to module.h.

Jessica
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Petr Mladek [17/12/15 17:26 +0100]:

On Mon 2015-11-30 23:21:15, Jessica Yu wrote:

For livepatch modules, copy Elf section, symbol, and string information
from the load_info struct in the module loader.

Livepatch uses special relocation sections in order to be able to patch
modules that are not yet loaded, as well as apply patches to the kernel
when the addresses of symbols cannot be determined at compile time (for
example, when kaslr is enabled). Livepatch modules must preserve Elf
information such as section indices in order to apply the remaining
relocation sections at the appropriate time (i.e. when the target module
loads).

Signed-off-by: Jessica Yu 
---
 include/linux/module.h |  9 +
 kernel/module.c| 98 --
 2 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 3a19c79..9b46256 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -425,6 +425,14 @@ struct module {

/* Notes attributes */
struct module_notes_attrs *notes_attrs;
+
+   /* Elf information (optionally saved) */
+   Elf_Ehdr *hdr;
+   Elf_Shdr *sechdrs;
+   char *secstrings;
+   struct {
+   unsigned int sym, str, mod, vers, info, pcpu;
+   } index;
 #endif


I would hide this into a structure. It is 3 pointers and 6 integers
that are mostly unused. I think about using a pointer to struct load_info
here. We could set the unused stuff to zero and NULL.

Any better idea how to share the definition with struct load_info
is welcome.


Sure. If we want to encapsulate all this information, we can perhaps
replace this with a single pointer to a copy of load_info, and maybe
move the struct definition of load_info to module.h.

Jessica
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Petr Mladek]

On Tue 2015-12-08 12:32:12, Josh Poimboeuf wrote:
> On Mon, Nov 30, 2015 at 11:21:15PM -0500, Jessica Yu wrote:
> > For livepatch modules, copy Elf section, symbol, and string information
> > from the load_info struct in the module loader.
> > 
> > Livepatch uses special relocation sections in order to be able to patch
> > modules that are not yet loaded, as well as apply patches to the kernel
> > when the addresses of symbols cannot be determined at compile time (for
> > example, when kaslr is enabled). Livepatch modules must preserve Elf
> > information such as section indices in order to apply the remaining
> > relocation sections at the appropriate time (i.e. when the target module
> > loads).
> > 
> > Signed-off-by: Jessica Yu 
> > ---
> >  include/linux/module.h |  9 +
> >  kernel/module.c| 98 
> > --
> >  2 files changed, 105 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 3a19c79..9b46256 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -425,6 +425,14 @@ struct module {
> >  
> > /* Notes attributes */
> > struct module_notes_attrs *notes_attrs;
> > +
> > +   /* Elf information (optionally saved) */
> > +   Elf_Ehdr *hdr;
> 
> I would rename "hdr" to "elf_hdr" to make its purpose clearer.
> 
> > +   Elf_Shdr *sechdrs;
> > +   char *secstrings;
> 
> Probably a good idea to add underscores to the names ("sec_hdrs" and
> "sec_strings") to be consistent with most of the other fields in the
> struct.

We should keep the names in sync with struct load_info.

> > +   struct {
> > +   unsigned int sym, str, mod, vers, info, pcpu;
> > +   } index;
> 
> I might be contradicting myself from what I said before.  But I'm
> thinking we should put all these fields inside a CONFIG_LIVEPATCH ifdef.
> Then below, there could be two versions of copy_module_elf(), the real
> one for LIVEPATCH and and an empty one for !LIVEPATCH.  And the same
> story for free_module_elf().

Yes, I would hide these fields for !LIVEPATCH.


Best Regards,
Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Petr Mladek]

On Mon 2015-11-30 23:21:15, Jessica Yu wrote:
> For livepatch modules, copy Elf section, symbol, and string information
> from the load_info struct in the module loader.
> 
> Livepatch uses special relocation sections in order to be able to patch
> modules that are not yet loaded, as well as apply patches to the kernel
> when the addresses of symbols cannot be determined at compile time (for
> example, when kaslr is enabled). Livepatch modules must preserve Elf
> information such as section indices in order to apply the remaining
> relocation sections at the appropriate time (i.e. when the target module
> loads).
> 
> Signed-off-by: Jessica Yu 
> ---
>  include/linux/module.h |  9 +
>  kernel/module.c| 98 
> --
>  2 files changed, 105 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 3a19c79..9b46256 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -425,6 +425,14 @@ struct module {
>  
>   /* Notes attributes */
>   struct module_notes_attrs *notes_attrs;
> +
> + /* Elf information (optionally saved) */
> + Elf_Ehdr *hdr;
> + Elf_Shdr *sechdrs;
> + char *secstrings;
> + struct {
> + unsigned int sym, str, mod, vers, info, pcpu;
> + } index;
>  #endif

I would hide this into a structure. It is 3 pointers and 6 integers
that are mostly unused. I think about using a pointer to struct load_info
here. We could set the unused stuff to zero and NULL.

Any better idea how to share the definition with struct load_info
is welcome.

Best Regards,
Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Petr Mladek]

On Tue 2015-12-08 12:32:12, Josh Poimboeuf wrote:
> On Mon, Nov 30, 2015 at 11:21:15PM -0500, Jessica Yu wrote:
> > For livepatch modules, copy Elf section, symbol, and string information
> > from the load_info struct in the module loader.
> > 
> > Livepatch uses special relocation sections in order to be able to patch
> > modules that are not yet loaded, as well as apply patches to the kernel
> > when the addresses of symbols cannot be determined at compile time (for
> > example, when kaslr is enabled). Livepatch modules must preserve Elf
> > information such as section indices in order to apply the remaining
> > relocation sections at the appropriate time (i.e. when the target module
> > loads).
> > 
> > Signed-off-by: Jessica Yu 
> > ---
> >  include/linux/module.h |  9 +
> >  kernel/module.c| 98 
> > --
> >  2 files changed, 105 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 3a19c79..9b46256 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -425,6 +425,14 @@ struct module {
> >  
> > /* Notes attributes */
> > struct module_notes_attrs *notes_attrs;
> > +
> > +   /* Elf information (optionally saved) */
> > +   Elf_Ehdr *hdr;
> 
> I would rename "hdr" to "elf_hdr" to make its purpose clearer.
> 
> > +   Elf_Shdr *sechdrs;
> > +   char *secstrings;
> 
> Probably a good idea to add underscores to the names ("sec_hdrs" and
> "sec_strings") to be consistent with most of the other fields in the
> struct.

We should keep the names in sync with struct load_info.

> > +   struct {
> > +   unsigned int sym, str, mod, vers, info, pcpu;
> > +   } index;
> 
> I might be contradicting myself from what I said before.  But I'm
> thinking we should put all these fields inside a CONFIG_LIVEPATCH ifdef.
> Then below, there could be two versions of copy_module_elf(), the real
> one for LIVEPATCH and and an empty one for !LIVEPATCH.  And the same
> story for free_module_elf().

Yes, I would hide these fields for !LIVEPATCH.


Best Regards,
Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Petr Mladek]

On Mon 2015-11-30 23:21:15, Jessica Yu wrote:
> For livepatch modules, copy Elf section, symbol, and string information
> from the load_info struct in the module loader.
> 
> Livepatch uses special relocation sections in order to be able to patch
> modules that are not yet loaded, as well as apply patches to the kernel
> when the addresses of symbols cannot be determined at compile time (for
> example, when kaslr is enabled). Livepatch modules must preserve Elf
> information such as section indices in order to apply the remaining
> relocation sections at the appropriate time (i.e. when the target module
> loads).
> 
> Signed-off-by: Jessica Yu 
> ---
>  include/linux/module.h |  9 +
>  kernel/module.c| 98 
> --
>  2 files changed, 105 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 3a19c79..9b46256 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -425,6 +425,14 @@ struct module {
>  
>   /* Notes attributes */
>   struct module_notes_attrs *notes_attrs;
> +
> + /* Elf information (optionally saved) */
> + Elf_Ehdr *hdr;
> + Elf_Shdr *sechdrs;
> + char *secstrings;
> + struct {
> + unsigned int sym, str, mod, vers, info, pcpu;
> + } index;
>  #endif

I would hide this into a structure. It is 3 pointers and 6 integers
that are mostly unused. I think about using a pointer to struct load_info
here. We could set the unused stuff to zero and NULL.

Any better idea how to share the definition with struct load_info
is welcome.

Best Regards,
Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Miroslav Benes [16/12/15 11:58 +0100]:

On Mon, 30 Nov 2015, Jessica Yu wrote:


@@ -3530,6 +3614,16 @@ static int load_module(struct load_info *info, const 
char __user *uargs,
if (err < 0)
goto bug_cleanup;

+   /*
+* Save sechdrs, indices, and other data from info
+* in order to patch to-be-loaded modules.
+* Do not call free_copy() for livepatch modules.
+*/
+   if (mod->klp)
+   err = copy_module_elf(mod, info);
+   if (err < 0)
+   goto bug_cleanup;
+


I think goto bug_cleanup is not sufficient. Just before this hunk sysfs is
created. So in case of error here we should call mod_sysfs_teardown() or
something before going to bug_cleanup. That is new error label is needed
before bug_cleanup. Correct?


Yes I think you're right. So before bug_cleanup, we'll just need a new
label (maybe called sysfs_cleanup) that calls mod_sysfs_teardown(). In
addition, the if (err < 0) check should have been enclosed in the if
(mod->klp) block.

Jessica
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Miroslav Benes]

On Mon, 30 Nov 2015, Jessica Yu wrote:

> @@ -3530,6 +3614,16 @@ static int load_module(struct load_info *info, const 
> char __user *uargs,
>   if (err < 0)
>   goto bug_cleanup;
>  
> + /*
> +  * Save sechdrs, indices, and other data from info
> +  * in order to patch to-be-loaded modules.
> +  * Do not call free_copy() for livepatch modules.
> +  */
> + if (mod->klp)
> + err = copy_module_elf(mod, info);
> + if (err < 0)
> + goto bug_cleanup;
> +

I think goto bug_cleanup is not sufficient. Just before this hunk sysfs is 
created. So in case of error here we should call mod_sysfs_teardown() or 
something before going to bug_cleanup. That is new error label is needed 
before bug_cleanup. Correct?

Miroslav
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: module: preserve Elf information for livepatch modules

[Miroslav Benes]

On Thu, 10 Dec 2015, Josh Poimboeuf wrote:

> On Wed, Dec 09, 2015 at 03:05:23PM -0500, Jessica Yu wrote:
> > +++ Josh Poimboeuf [08/12/15 12:32 -0600]:
> > >On Mon, Nov 30, 2015 at 11:21:15PM -0500, Jessica Yu wrote:
> > >>For livepatch modules, copy Elf section, symbol, and string information
> > >>from the load_info struct in the module loader.
> > >>
> > >>Livepatch uses special relocation sections in order to be able to patch
> > >>modules that are not yet loaded, as well as apply patches to the kernel
> > >>when the addresses of symbols cannot be determined at compile time (for
> > >>example, when kaslr is enabled). Livepatch modules must preserve Elf
> > >>information such as section indices in order to apply the remaining
> > >>relocation sections at the appropriate time (i.e. when the target module
> > >>loads).
> > >>
> > >>Signed-off-by: Jessica Yu 
> > >>---
> > >> include/linux/module.h |  9 +
> > >> kernel/module.c| 98 
> > >> --
> > >> 2 files changed, 105 insertions(+), 2 deletions(-)
> > >>
> > >>diff --git a/include/linux/module.h b/include/linux/module.h
> > >>index 3a19c79..9b46256 100644
> > >>--- a/include/linux/module.h
> > >>+++ b/include/linux/module.h
> > >>@@ -425,6 +425,14 @@ struct module {
> > >>
> > >>  /* Notes attributes */
> > >>  struct module_notes_attrs *notes_attrs;
> > >>+
> > >>+ /* Elf information (optionally saved) */
> > >>+ Elf_Ehdr *hdr;
> > >
> > >I would rename "hdr" to "elf_hdr" to make its purpose clearer.
> > >
> > >>+ Elf_Shdr *sechdrs;
> > >>+ char *secstrings;
> > >
> > >Probably a good idea to add underscores to the names ("sec_hdrs" and
> > >"sec_strings") to be consistent with most of the other fields in the
> > >struct.
> > >
> > >>+ struct {
> > >>+ unsigned int sym, str, mod, vers, info, pcpu;
> > >>+ } index;
> > >
> > >I might be contradicting myself from what I said before.  But I'm
> > >thinking we should put all these fields inside a CONFIG_LIVEPATCH ifdef.
> > >Then below, there could be two versions of copy_module_elf(), the real
> > >one for LIVEPATCH and and an empty one for !LIVEPATCH.  And the same
> > >story for free_module_elf().
> > 
> > I think in the v1 discussion we were leaning more towards making this
> > generic. We could potentially just have the Elf module fields
> > available in the generic case, independent of whether CONFIG_LIVEPATCH
> > is set, whereas the mod->klp field should probably be only available
> > when LIVEPATCH is set. I think this makes sense since the Elf fields
> > aren't dependent on livepatch (although livepatch would be the only
> > user of these fields at the moment). I don't know if there would be
> > any users in the future that would be interested in using this Elf
> > information. Thoughts on this?
> 
> IIRC, I think I made the suggestion to always save the elf fields
> because otherwise it was looking like we were going to need a lot of
> spaghetti code.
> 
> But if we can find a way to wrap the elf fields in LIVEPATCH while
> keeping the code simple, then there's no real downside and I think
> that's the way to go.  If somebody else wants to use the fields later,
> then they can remove or change the ifdefs as needed.

I agree with Josh. There is a generic function prepared 
(copy_module_elf()) if someone needs it in the future. But for now we can 
hide everything under CONFIG_LIVEPATCH in the same way layout_symtab() and 
add_kallsyms() are under CONFIG_KALLSYMS.

Miroslav
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Miroslav Benes [16/12/15 11:58 +0100]:

On Mon, 30 Nov 2015, Jessica Yu wrote:


@@ -3530,6 +3614,16 @@ static int load_module(struct load_info *info, const 
char __user *uargs,
if (err < 0)
goto bug_cleanup;

+   /*
+* Save sechdrs, indices, and other data from info
+* in order to patch to-be-loaded modules.
+* Do not call free_copy() for livepatch modules.
+*/
+   if (mod->klp)
+   err = copy_module_elf(mod, info);
+   if (err < 0)
+   goto bug_cleanup;
+


I think goto bug_cleanup is not sufficient. Just before this hunk sysfs is
created. So in case of error here we should call mod_sysfs_teardown() or
something before going to bug_cleanup. That is new error label is needed
before bug_cleanup. Correct?


Yes I think you're right. So before bug_cleanup, we'll just need a new
label (maybe called sysfs_cleanup) that calls mod_sysfs_teardown(). In
addition, the if (err < 0) check should have been enclosed in the if
(mod->klp) block.

Jessica
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Miroslav Benes]

On Mon, 30 Nov 2015, Jessica Yu wrote:

> @@ -3530,6 +3614,16 @@ static int load_module(struct load_info *info, const 
> char __user *uargs,
>   if (err < 0)
>   goto bug_cleanup;
>  
> + /*
> +  * Save sechdrs, indices, and other data from info
> +  * in order to patch to-be-loaded modules.
> +  * Do not call free_copy() for livepatch modules.
> +  */
> + if (mod->klp)
> + err = copy_module_elf(mod, info);
> + if (err < 0)
> + goto bug_cleanup;
> +

I think goto bug_cleanup is not sufficient. Just before this hunk sysfs is 
created. So in case of error here we should call mod_sysfs_teardown() or 
something before going to bug_cleanup. That is new error label is needed 
before bug_cleanup. Correct?

Miroslav
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: module: preserve Elf information for livepatch modules

[Miroslav Benes]

On Thu, 10 Dec 2015, Josh Poimboeuf wrote:

> On Wed, Dec 09, 2015 at 03:05:23PM -0500, Jessica Yu wrote:
> > +++ Josh Poimboeuf [08/12/15 12:32 -0600]:
> > >On Mon, Nov 30, 2015 at 11:21:15PM -0500, Jessica Yu wrote:
> > >>For livepatch modules, copy Elf section, symbol, and string information
> > >>from the load_info struct in the module loader.
> > >>
> > >>Livepatch uses special relocation sections in order to be able to patch
> > >>modules that are not yet loaded, as well as apply patches to the kernel
> > >>when the addresses of symbols cannot be determined at compile time (for
> > >>example, when kaslr is enabled). Livepatch modules must preserve Elf
> > >>information such as section indices in order to apply the remaining
> > >>relocation sections at the appropriate time (i.e. when the target module
> > >>loads).
> > >>
> > >>Signed-off-by: Jessica Yu 
> > >>---
> > >> include/linux/module.h |  9 +
> > >> kernel/module.c| 98 
> > >> --
> > >> 2 files changed, 105 insertions(+), 2 deletions(-)
> > >>
> > >>diff --git a/include/linux/module.h b/include/linux/module.h
> > >>index 3a19c79..9b46256 100644
> > >>--- a/include/linux/module.h
> > >>+++ b/include/linux/module.h
> > >>@@ -425,6 +425,14 @@ struct module {
> > >>
> > >>  /* Notes attributes */
> > >>  struct module_notes_attrs *notes_attrs;
> > >>+
> > >>+ /* Elf information (optionally saved) */
> > >>+ Elf_Ehdr *hdr;
> > >
> > >I would rename "hdr" to "elf_hdr" to make its purpose clearer.
> > >
> > >>+ Elf_Shdr *sechdrs;
> > >>+ char *secstrings;
> > >
> > >Probably a good idea to add underscores to the names ("sec_hdrs" and
> > >"sec_strings") to be consistent with most of the other fields in the
> > >struct.
> > >
> > >>+ struct {
> > >>+ unsigned int sym, str, mod, vers, info, pcpu;
> > >>+ } index;
> > >
> > >I might be contradicting myself from what I said before.  But I'm
> > >thinking we should put all these fields inside a CONFIG_LIVEPATCH ifdef.
> > >Then below, there could be two versions of copy_module_elf(), the real
> > >one for LIVEPATCH and and an empty one for !LIVEPATCH.  And the same
> > >story for free_module_elf().
> > 
> > I think in the v1 discussion we were leaning more towards making this
> > generic. We could potentially just have the Elf module fields
> > available in the generic case, independent of whether CONFIG_LIVEPATCH
> > is set, whereas the mod->klp field should probably be only available
> > when LIVEPATCH is set. I think this makes sense since the Elf fields
> > aren't dependent on livepatch (although livepatch would be the only
> > user of these fields at the moment). I don't know if there would be
> > any users in the future that would be interested in using this Elf
> > information. Thoughts on this?
> 
> IIRC, I think I made the suggestion to always save the elf fields
> because otherwise it was looking like we were going to need a lot of
> spaghetti code.
> 
> But if we can find a way to wrap the elf fields in LIVEPATCH while
> keeping the code simple, then there's no real downside and I think
> that's the way to go.  If somebody else wants to use the fields later,
> then they can remove or change the ifdefs as needed.

I agree with Josh. There is a generic function prepared 
(copy_module_elf()) if someone needs it in the future. But for now we can 
hide everything under CONFIG_LIVEPATCH in the same way layout_symtab() and 
add_kallsyms() are under CONFIG_KALLSYMS.

Miroslav
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: module: preserve Elf information for livepatch modules

[Josh Poimboeuf]

On Wed, Dec 09, 2015 at 03:05:23PM -0500, Jessica Yu wrote:
> +++ Josh Poimboeuf [08/12/15 12:32 -0600]:
> >On Mon, Nov 30, 2015 at 11:21:15PM -0500, Jessica Yu wrote:
> >>For livepatch modules, copy Elf section, symbol, and string information
> >>from the load_info struct in the module loader.
> >>
> >>Livepatch uses special relocation sections in order to be able to patch
> >>modules that are not yet loaded, as well as apply patches to the kernel
> >>when the addresses of symbols cannot be determined at compile time (for
> >>example, when kaslr is enabled). Livepatch modules must preserve Elf
> >>information such as section indices in order to apply the remaining
> >>relocation sections at the appropriate time (i.e. when the target module
> >>loads).
> >>
> >>Signed-off-by: Jessica Yu 
> >>---
> >> include/linux/module.h |  9 +
> >> kernel/module.c| 98 
> >> --
> >> 2 files changed, 105 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/include/linux/module.h b/include/linux/module.h
> >>index 3a19c79..9b46256 100644
> >>--- a/include/linux/module.h
> >>+++ b/include/linux/module.h
> >>@@ -425,6 +425,14 @@ struct module {
> >>
> >>/* Notes attributes */
> >>struct module_notes_attrs *notes_attrs;
> >>+
> >>+   /* Elf information (optionally saved) */
> >>+   Elf_Ehdr *hdr;
> >
> >I would rename "hdr" to "elf_hdr" to make its purpose clearer.
> >
> >>+   Elf_Shdr *sechdrs;
> >>+   char *secstrings;
> >
> >Probably a good idea to add underscores to the names ("sec_hdrs" and
> >"sec_strings") to be consistent with most of the other fields in the
> >struct.
> >
> >>+   struct {
> >>+   unsigned int sym, str, mod, vers, info, pcpu;
> >>+   } index;
> >
> >I might be contradicting myself from what I said before.  But I'm
> >thinking we should put all these fields inside a CONFIG_LIVEPATCH ifdef.
> >Then below, there could be two versions of copy_module_elf(), the real
> >one for LIVEPATCH and and an empty one for !LIVEPATCH.  And the same
> >story for free_module_elf().
> 
> I think in the v1 discussion we were leaning more towards making this
> generic. We could potentially just have the Elf module fields
> available in the generic case, independent of whether CONFIG_LIVEPATCH
> is set, whereas the mod->klp field should probably be only available
> when LIVEPATCH is set. I think this makes sense since the Elf fields
> aren't dependent on livepatch (although livepatch would be the only
> user of these fields at the moment). I don't know if there would be
> any users in the future that would be interested in using this Elf
> information. Thoughts on this?

IIRC, I think I made the suggestion to always save the elf fields
because otherwise it was looking like we were going to need a lot of
spaghetti code.

But if we can find a way to wrap the elf fields in LIVEPATCH while
keeping the code simple, then there's no real downside and I think
that's the way to go.  If somebody else wants to use the fields later,
then they can remove or change the ifdefs as needed.

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: module: preserve Elf information for livepatch modules

[Josh Poimboeuf]

On Wed, Dec 09, 2015 at 03:05:23PM -0500, Jessica Yu wrote:
> +++ Josh Poimboeuf [08/12/15 12:32 -0600]:
> >On Mon, Nov 30, 2015 at 11:21:15PM -0500, Jessica Yu wrote:
> >>For livepatch modules, copy Elf section, symbol, and string information
> >>from the load_info struct in the module loader.
> >>
> >>Livepatch uses special relocation sections in order to be able to patch
> >>modules that are not yet loaded, as well as apply patches to the kernel
> >>when the addresses of symbols cannot be determined at compile time (for
> >>example, when kaslr is enabled). Livepatch modules must preserve Elf
> >>information such as section indices in order to apply the remaining
> >>relocation sections at the appropriate time (i.e. when the target module
> >>loads).
> >>
> >>Signed-off-by: Jessica Yu 
> >>---
> >> include/linux/module.h |  9 +
> >> kernel/module.c| 98 
> >> --
> >> 2 files changed, 105 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/include/linux/module.h b/include/linux/module.h
> >>index 3a19c79..9b46256 100644
> >>--- a/include/linux/module.h
> >>+++ b/include/linux/module.h
> >>@@ -425,6 +425,14 @@ struct module {
> >>
> >>/* Notes attributes */
> >>struct module_notes_attrs *notes_attrs;
> >>+
> >>+   /* Elf information (optionally saved) */
> >>+   Elf_Ehdr *hdr;
> >
> >I would rename "hdr" to "elf_hdr" to make its purpose clearer.
> >
> >>+   Elf_Shdr *sechdrs;
> >>+   char *secstrings;
> >
> >Probably a good idea to add underscores to the names ("sec_hdrs" and
> >"sec_strings") to be consistent with most of the other fields in the
> >struct.
> >
> >>+   struct {
> >>+   unsigned int sym, str, mod, vers, info, pcpu;
> >>+   } index;
> >
> >I might be contradicting myself from what I said before.  But I'm
> >thinking we should put all these fields inside a CONFIG_LIVEPATCH ifdef.
> >Then below, there could be two versions of copy_module_elf(), the real
> >one for LIVEPATCH and and an empty one for !LIVEPATCH.  And the same
> >story for free_module_elf().
> 
> I think in the v1 discussion we were leaning more towards making this
> generic. We could potentially just have the Elf module fields
> available in the generic case, independent of whether CONFIG_LIVEPATCH
> is set, whereas the mod->klp field should probably be only available
> when LIVEPATCH is set. I think this makes sense since the Elf fields
> aren't dependent on livepatch (although livepatch would be the only
> user of these fields at the moment). I don't know if there would be
> any users in the future that would be interested in using this Elf
> information. Thoughts on this?

IIRC, I think I made the suggestion to always save the elf fields
because otherwise it was looking like we were going to need a lot of
spaghetti code.

But if we can find a way to wrap the elf fields in LIVEPATCH while
keeping the code simple, then there's no real downside and I think
that's the way to go.  If somebody else wants to use the fields later,
then they can remove or change the ifdefs as needed.

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Josh Poimboeuf [08/12/15 12:32 -0600]:

On Mon, Nov 30, 2015 at 11:21:15PM -0500, Jessica Yu wrote:

For livepatch modules, copy Elf section, symbol, and string information
from the load_info struct in the module loader.

Livepatch uses special relocation sections in order to be able to patch
modules that are not yet loaded, as well as apply patches to the kernel
when the addresses of symbols cannot be determined at compile time (for
example, when kaslr is enabled). Livepatch modules must preserve Elf
information such as section indices in order to apply the remaining
relocation sections at the appropriate time (i.e. when the target module
loads).

Signed-off-by: Jessica Yu 
---
 include/linux/module.h |  9 +
 kernel/module.c| 98 --
 2 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 3a19c79..9b46256 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -425,6 +425,14 @@ struct module {

/* Notes attributes */
struct module_notes_attrs *notes_attrs;
+
+   /* Elf information (optionally saved) */
+   Elf_Ehdr *hdr;


I would rename "hdr" to "elf_hdr" to make its purpose clearer.


+   Elf_Shdr *sechdrs;
+   char *secstrings;


Probably a good idea to add underscores to the names ("sec_hdrs" and
"sec_strings") to be consistent with most of the other fields in the
struct.


+   struct {
+   unsigned int sym, str, mod, vers, info, pcpu;
+   } index;


I might be contradicting myself from what I said before.  But I'm
thinking we should put all these fields inside a CONFIG_LIVEPATCH ifdef.
Then below, there could be two versions of copy_module_elf(), the real
one for LIVEPATCH and and an empty one for !LIVEPATCH.  And the same
story for free_module_elf().


I think in the v1 discussion we were leaning more towards making this
generic. We could potentially just have the Elf module fields
available in the generic case, independent of whether CONFIG_LIVEPATCH
is set, whereas the mod->klp field should probably be only available
when LIVEPATCH is set. I think this makes sense since the Elf fields
aren't dependent on livepatch (although livepatch would be the only
user of these fields at the moment). I don't know if there would be
any users in the future that would be interested in using this Elf
information. Thoughts on this?


 #endif

/* The command line arguments (may be mangled).  People like
@@ -461,6 +469,7 @@ struct module {
 #endif

 #ifdef CONFIG_LIVEPATCH
+   bool klp; /* Is this a livepatch module? */
bool klp_alive;
 #endif

diff --git a/kernel/module.c b/kernel/module.c
index 8f051a1..433c2d6 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1984,6 +1984,13 @@ static void unset_module_core_ro_nx(struct module *mod) 
{ }
 static void unset_module_init_ro_nx(struct module *mod) { }
 #endif

+static void free_module_elf(struct module *mod)
+{
+   kfree(mod->hdr);
+   kfree(mod->sechdrs);
+   kfree(mod->secstrings);
+}
+
 void __weak module_memfree(void *module_region)
 {
vfree(module_region);
@@ -2022,6 +2029,9 @@ 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 */
+   free_module_elf(mod);
+
/* Now we can delete it from the lists */
mutex_lock(_mutex);
/* Unlink carefully: kallsyms could be walking list. */
@@ -2137,6 +2147,10 @@ static int simplify_symbols(struct module *mod, const 
struct load_info *info)
   (long)sym[i].st_value);
break;

+   case SHN_LIVEPATCH:
+   /* klp symbols are resolved by livepatch */
+   break;
+
case SHN_UNDEF:
ksym = resolve_symbol_wait(mod, info, name);
/* Ok if resolved.  */
@@ -2185,6 +2199,10 @@ static int apply_relocations(struct module *mod, const 
struct load_info *info)
if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
continue;

+   /* klp 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);
@@ -2393,6 +2411,11 @@ static char elf_type(const Elf_Sym *sym, const struct 
load_info *info)
 {
const Elf_Shdr *sechdrs = info->sechdrs;

+   if (ELF_ST_BIND(sym->st_info) == STB_LIVEPATCH_EXT)
+   return 'K';
+   if (sym->st_shndx == SHN_LIVEPATCH)
+   return 'k';
+
if (ELF_ST_BIND(sym->st_info) == STB_WEAK) {
   

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Josh Poimboeuf [08/12/15 12:32 -0600]:

On Mon, Nov 30, 2015 at 11:21:15PM -0500, Jessica Yu wrote:

For livepatch modules, copy Elf section, symbol, and string information
from the load_info struct in the module loader.

Livepatch uses special relocation sections in order to be able to patch
modules that are not yet loaded, as well as apply patches to the kernel
when the addresses of symbols cannot be determined at compile time (for
example, when kaslr is enabled). Livepatch modules must preserve Elf
information such as section indices in order to apply the remaining
relocation sections at the appropriate time (i.e. when the target module
loads).

Signed-off-by: Jessica Yu 
---
 include/linux/module.h |  9 +
 kernel/module.c| 98 --
 2 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 3a19c79..9b46256 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -425,6 +425,14 @@ struct module {

/* Notes attributes */
struct module_notes_attrs *notes_attrs;
+
+   /* Elf information (optionally saved) */
+   Elf_Ehdr *hdr;


I would rename "hdr" to "elf_hdr" to make its purpose clearer.


+   Elf_Shdr *sechdrs;
+   char *secstrings;


Probably a good idea to add underscores to the names ("sec_hdrs" and
"sec_strings") to be consistent with most of the other fields in the
struct.


+   struct {
+   unsigned int sym, str, mod, vers, info, pcpu;
+   } index;


I might be contradicting myself from what I said before.  But I'm
thinking we should put all these fields inside a CONFIG_LIVEPATCH ifdef.
Then below, there could be two versions of copy_module_elf(), the real
one for LIVEPATCH and and an empty one for !LIVEPATCH.  And the same
story for free_module_elf().


I think in the v1 discussion we were leaning more towards making this
generic. We could potentially just have the Elf module fields
available in the generic case, independent of whether CONFIG_LIVEPATCH
is set, whereas the mod->klp field should probably be only available
when LIVEPATCH is set. I think this makes sense since the Elf fields
aren't dependent on livepatch (although livepatch would be the only
user of these fields at the moment). I don't know if there would be
any users in the future that would be interested in using this Elf
information. Thoughts on this?


 #endif

/* The command line arguments (may be mangled).  People like
@@ -461,6 +469,7 @@ struct module {
 #endif

 #ifdef CONFIG_LIVEPATCH
+   bool klp; /* Is this a livepatch module? */
bool klp_alive;
 #endif

diff --git a/kernel/module.c b/kernel/module.c
index 8f051a1..433c2d6 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1984,6 +1984,13 @@ static void unset_module_core_ro_nx(struct module *mod) 
{ }
 static void unset_module_init_ro_nx(struct module *mod) { }
 #endif

+static void free_module_elf(struct module *mod)
+{
+   kfree(mod->hdr);
+   kfree(mod->sechdrs);
+   kfree(mod->secstrings);
+}
+
 void __weak module_memfree(void *module_region)
 {
vfree(module_region);
@@ -2022,6 +2029,9 @@ 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 */
+   free_module_elf(mod);
+
/* Now we can delete it from the lists */
mutex_lock(_mutex);
/* Unlink carefully: kallsyms could be walking list. */
@@ -2137,6 +2147,10 @@ static int simplify_symbols(struct module *mod, const 
struct load_info *info)
   (long)sym[i].st_value);
break;

+   case SHN_LIVEPATCH:
+   /* klp symbols are resolved by livepatch */
+   break;
+
case SHN_UNDEF:
ksym = resolve_symbol_wait(mod, info, name);
/* Ok if resolved.  */
@@ -2185,6 +2199,10 @@ static int apply_relocations(struct module *mod, const 
struct load_info *info)
if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
continue;

+   /* klp 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);
@@ -2393,6 +2411,11 @@ static char elf_type(const Elf_Sym *sym, const struct 
load_info *info)
 {
const Elf_Shdr *sechdrs = info->sechdrs;

+   if (ELF_ST_BIND(sym->st_info) == STB_LIVEPATCH_EXT)
+   return 'K';
+   if (sym->st_shndx == SHN_LIVEPATCH)
+   return 'k';
+
if (ELF_ST_BIND(sym->st_info) == 

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

[Josh Poimboeuf]

On Mon, Nov 30, 2015 at 11:21:15PM -0500, Jessica Yu wrote:
> For livepatch modules, copy Elf section, symbol, and string information
> from the load_info struct in the module loader.
> 
> Livepatch uses special relocation sections in order to be able to patch
> modules that are not yet loaded, as well as apply patches to the kernel
> when the addresses of symbols cannot be determined at compile time (for
> example, when kaslr is enabled). Livepatch modules must preserve Elf
> information such as section indices in order to apply the remaining
> relocation sections at the appropriate time (i.e. when the target module
> loads).
> 
> Signed-off-by: Jessica Yu 
> ---
>  include/linux/module.h |  9 +
>  kernel/module.c| 98 
> --
>  2 files changed, 105 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 3a19c79..9b46256 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -425,6 +425,14 @@ struct module {
>  
>   /* Notes attributes */
>   struct module_notes_attrs *notes_attrs;
> +
> + /* Elf information (optionally saved) */
> + Elf_Ehdr *hdr;

I would rename "hdr" to "elf_hdr" to make its purpose clearer.

> + Elf_Shdr *sechdrs;
> + char *secstrings;

Probably a good idea to add underscores to the names ("sec_hdrs" and
"sec_strings") to be consistent with most of the other fields in the
struct.

> + struct {
> + unsigned int sym, str, mod, vers, info, pcpu;
> + } index;

I might be contradicting myself from what I said before.  But I'm
thinking we should put all these fields inside a CONFIG_LIVEPATCH ifdef.
Then below, there could be two versions of copy_module_elf(), the real
one for LIVEPATCH and and an empty one for !LIVEPATCH.  And the same
story for free_module_elf().

>  #endif
>  
>   /* The command line arguments (may be mangled).  People like
> @@ -461,6 +469,7 @@ struct module {
>  #endif
>  
>  #ifdef CONFIG_LIVEPATCH
> + bool klp; /* Is this a livepatch module? */
>   bool klp_alive;
>  #endif
>  
> diff --git a/kernel/module.c b/kernel/module.c
> index 8f051a1..433c2d6 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1984,6 +1984,13 @@ static void unset_module_core_ro_nx(struct module 
> *mod) { }
>  static void unset_module_init_ro_nx(struct module *mod) { }
>  #endif
>  
> +static void free_module_elf(struct module *mod)
> +{
> + kfree(mod->hdr);
> + kfree(mod->sechdrs);
> + kfree(mod->secstrings);
> +}
> +
>  void __weak module_memfree(void *module_region)
>  {
>   vfree(module_region);
> @@ -2022,6 +2029,9 @@ 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 */
> + free_module_elf(mod);
> +
>   /* Now we can delete it from the lists */
>   mutex_lock(_mutex);
>   /* Unlink carefully: kallsyms could be walking list. */
> @@ -2137,6 +2147,10 @@ static int simplify_symbols(struct module *mod, const 
> struct load_info *info)
>  (long)sym[i].st_value);
>   break;
>  
> + case SHN_LIVEPATCH:
> + /* klp symbols are resolved by livepatch */
> + break;
> +
>   case SHN_UNDEF:
>   ksym = resolve_symbol_wait(mod, info, name);
>   /* Ok if resolved.  */
> @@ -2185,6 +2199,10 @@ static int apply_relocations(struct module *mod, const 
> struct load_info *info)
>   if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
>   continue;
>  
> + /* klp 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);
> @@ -2393,6 +2411,11 @@ static char elf_type(const Elf_Sym *sym, const struct 
> load_info *info)
>  {
>   const Elf_Shdr *sechdrs = info->sechdrs;
>  
> + if (ELF_ST_BIND(sym->st_info) == STB_LIVEPATCH_EXT)
> + return 'K';
> + if (sym->st_shndx == SHN_LIVEPATCH)
> + return 'k';
> +
>   if (ELF_ST_BIND(sym->st_info) == STB_WEAK) {
>   if (ELF_ST_TYPE(sym->st_info) == STT_OBJECT)
>   return 'v';
> @@ -2475,7 +2498,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 || mod->klp ||
>   is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum)) {
>   

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

[Josh Poimboeuf]

On Mon, Nov 30, 2015 at 11:21:15PM -0500, Jessica Yu wrote:
> For livepatch modules, copy Elf section, symbol, and string information
> from the load_info struct in the module loader.
> 
> Livepatch uses special relocation sections in order to be able to patch
> modules that are not yet loaded, as well as apply patches to the kernel
> when the addresses of symbols cannot be determined at compile time (for
> example, when kaslr is enabled). Livepatch modules must preserve Elf
> information such as section indices in order to apply the remaining
> relocation sections at the appropriate time (i.e. when the target module
> loads).
> 
> Signed-off-by: Jessica Yu 
> ---
>  include/linux/module.h |  9 +
>  kernel/module.c| 98 
> --
>  2 files changed, 105 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 3a19c79..9b46256 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -425,6 +425,14 @@ struct module {
>  
>   /* Notes attributes */
>   struct module_notes_attrs *notes_attrs;
> +
> + /* Elf information (optionally saved) */
> + Elf_Ehdr *hdr;

I would rename "hdr" to "elf_hdr" to make its purpose clearer.

> + Elf_Shdr *sechdrs;
> + char *secstrings;

Probably a good idea to add underscores to the names ("sec_hdrs" and
"sec_strings") to be consistent with most of the other fields in the
struct.

> + struct {
> + unsigned int sym, str, mod, vers, info, pcpu;
> + } index;

I might be contradicting myself from what I said before.  But I'm
thinking we should put all these fields inside a CONFIG_LIVEPATCH ifdef.
Then below, there could be two versions of copy_module_elf(), the real
one for LIVEPATCH and and an empty one for !LIVEPATCH.  And the same
story for free_module_elf().

>  #endif
>  
>   /* The command line arguments (may be mangled).  People like
> @@ -461,6 +469,7 @@ struct module {
>  #endif
>  
>  #ifdef CONFIG_LIVEPATCH
> + bool klp; /* Is this a livepatch module? */
>   bool klp_alive;
>  #endif
>  
> diff --git a/kernel/module.c b/kernel/module.c
> index 8f051a1..433c2d6 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1984,6 +1984,13 @@ static void unset_module_core_ro_nx(struct module 
> *mod) { }
>  static void unset_module_init_ro_nx(struct module *mod) { }
>  #endif
>  
> +static void free_module_elf(struct module *mod)
> +{
> + kfree(mod->hdr);
> + kfree(mod->sechdrs);
> + kfree(mod->secstrings);
> +}
> +
>  void __weak module_memfree(void *module_region)
>  {
>   vfree(module_region);
> @@ -2022,6 +2029,9 @@ 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 */
> + free_module_elf(mod);
> +
>   /* Now we can delete it from the lists */
>   mutex_lock(_mutex);
>   /* Unlink carefully: kallsyms could be walking list. */
> @@ -2137,6 +2147,10 @@ static int simplify_symbols(struct module *mod, const 
> struct load_info *info)
>  (long)sym[i].st_value);
>   break;
>  
> + case SHN_LIVEPATCH:
> + /* klp symbols are resolved by livepatch */
> + break;
> +
>   case SHN_UNDEF:
>   ksym = resolve_symbol_wait(mod, info, name);
>   /* Ok if resolved.  */
> @@ -2185,6 +2199,10 @@ static int apply_relocations(struct module *mod, const 
> struct load_info *info)
>   if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
>   continue;
>  
> + /* klp 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);
> @@ -2393,6 +2411,11 @@ static char elf_type(const Elf_Sym *sym, const struct 
> load_info *info)
>  {
>   const Elf_Shdr *sechdrs = info->sechdrs;
>  
> + if (ELF_ST_BIND(sym->st_info) == STB_LIVEPATCH_EXT)
> + return 'K';
> + if (sym->st_shndx == SHN_LIVEPATCH)
> + return 'k';
> +
>   if (ELF_ST_BIND(sym->st_info) == STB_WEAK) {
>   if (ELF_ST_TYPE(sym->st_info) == STT_OBJECT)
>   return 'v';
> @@ -2475,7 +2498,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 || mod->klp ||
>   is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum)) {
> 

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Jessica Yu [30/11/15 23:21 -0500]:

For livepatch modules, copy Elf section, symbol, and string information
from the load_info struct in the module loader.

Livepatch uses special relocation sections in order to be able to patch
modules that are not yet loaded, as well as apply patches to the kernel
when the addresses of symbols cannot be determined at compile time (for
example, when kaslr is enabled). Livepatch modules must preserve Elf
information such as section indices in order to apply the remaining
relocation sections at the appropriate time (i.e. when the target module
loads).

Signed-off-by: Jessica Yu 
---
include/linux/module.h |  9 +
kernel/module.c| 98 --
2 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 3a19c79..9b46256 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -425,6 +425,14 @@ struct module {

/* Notes attributes */
struct module_notes_attrs *notes_attrs;
+
+   /* Elf information (optionally saved) */
+   Elf_Ehdr *hdr;
+   Elf_Shdr *sechdrs;
+   char *secstrings;
+   struct {
+   unsigned int sym, str, mod, vers, info, pcpu;
+   } index;
#endif


This particular patch unforunately breaks !CONFIG_KALLSYMS kernel
builds, as I've just discovered. These fields should move out of the
CONFIG_KALLSYMS block. And..


/* The command line arguments (may be mangled).  People like
@@ -461,6 +469,7 @@ struct module {
#endif

#ifdef CONFIG_LIVEPATCH
+   bool klp; /* Is this a livepatch module? */
bool klp_alive;
#endif

diff --git a/kernel/module.c b/kernel/module.c
index 8f051a1..433c2d6 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1984,6 +1984,13 @@ static void unset_module_core_ro_nx(struct module *mod) 
{ }
static void unset_module_init_ro_nx(struct module *mod) { }
#endif

+static void free_module_elf(struct module *mod)
+{
+   kfree(mod->hdr);
+   kfree(mod->sechdrs);
+   kfree(mod->secstrings);
+}
+
void __weak module_memfree(void *module_region)
{
vfree(module_region);
@@ -2022,6 +2029,9 @@ 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 */
+   free_module_elf(mod);
+
/* Now we can delete it from the lists */
mutex_lock(_mutex);
/* Unlink carefully: kallsyms could be walking list. */
@@ -2137,6 +2147,10 @@ static int simplify_symbols(struct module *mod, const 
struct load_info *info)
   (long)sym[i].st_value);
break;

+   case SHN_LIVEPATCH:
+   /* klp symbols are resolved by livepatch */
+   break;
+
case SHN_UNDEF:
ksym = resolve_symbol_wait(mod, info, name);
/* Ok if resolved.  */
@@ -2185,6 +2199,10 @@ static int apply_relocations(struct module *mod, const 
struct load_info *info)
if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
continue;

+   /* klp 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);
@@ -2393,6 +2411,11 @@ static char elf_type(const Elf_Sym *sym, const struct 
load_info *info)
{
const Elf_Shdr *sechdrs = info->sechdrs;

+   if (ELF_ST_BIND(sym->st_info) == STB_LIVEPATCH_EXT)
+   return 'K';
+   if (sym->st_shndx == SHN_LIVEPATCH)
+   return 'k';
+
if (ELF_ST_BIND(sym->st_info) == STB_WEAK) {
if (ELF_ST_TYPE(sym->st_info) == STT_OBJECT)
return 'v';
@@ -2475,7 +2498,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 || mod->klp ||
is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum)) {
strtab_size += strlen(>strtab[src[i].st_name])+1;
ndst++;
@@ -2517,7 +2540,7 @@ static void add_kallsyms(struct module *mod, const struct 
load_info *info)
mod->core_strtab = s = mod->module_core + info->stroffs;
src = mod->symtab;
for (ndst = i = 0; i < mod->num_symtab; i++) {
-   if (i == 0 ||
+   if (i == 0 || mod->klp ||
is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum)) {
dst[ndst] = src[i];

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Jessica Yu [30/11/15 23:21 -0500]:

For livepatch modules, copy Elf section, symbol, and string information
from the load_info struct in the module loader.

Livepatch uses special relocation sections in order to be able to patch
modules that are not yet loaded, as well as apply patches to the kernel
when the addresses of symbols cannot be determined at compile time (for
example, when kaslr is enabled). Livepatch modules must preserve Elf
information such as section indices in order to apply the remaining
relocation sections at the appropriate time (i.e. when the target module
loads).

Signed-off-by: Jessica Yu 
---
include/linux/module.h |  9 +
kernel/module.c| 98 --
2 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 3a19c79..9b46256 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -425,6 +425,14 @@ struct module {

/* Notes attributes */
struct module_notes_attrs *notes_attrs;
+
+   /* Elf information (optionally saved) */
+   Elf_Ehdr *hdr;
+   Elf_Shdr *sechdrs;
+   char *secstrings;
+   struct {
+   unsigned int sym, str, mod, vers, info, pcpu;
+   } index;
#endif

/* The command line arguments (may be mangled).  People like
@@ -461,6 +469,7 @@ struct module {
#endif

#ifdef CONFIG_LIVEPATCH
+   bool klp; /* Is this a livepatch module? */


Gah. I believe this field should be outside the #ifdef. 


Jessica
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Jessica Yu [30/11/15 23:21 -0500]:

For livepatch modules, copy Elf section, symbol, and string information
from the load_info struct in the module loader.

Livepatch uses special relocation sections in order to be able to patch
modules that are not yet loaded, as well as apply patches to the kernel
when the addresses of symbols cannot be determined at compile time (for
example, when kaslr is enabled). Livepatch modules must preserve Elf
information such as section indices in order to apply the remaining
relocation sections at the appropriate time (i.e. when the target module
loads).

Signed-off-by: Jessica Yu 
---
include/linux/module.h |  9 +
kernel/module.c| 98 --
2 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 3a19c79..9b46256 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -425,6 +425,14 @@ struct module {

/* Notes attributes */
struct module_notes_attrs *notes_attrs;
+
+   /* Elf information (optionally saved) */
+   Elf_Ehdr *hdr;
+   Elf_Shdr *sechdrs;
+   char *secstrings;
+   struct {
+   unsigned int sym, str, mod, vers, info, pcpu;
+   } index;
#endif


This particular patch unforunately breaks !CONFIG_KALLSYMS kernel
builds, as I've just discovered. These fields should move out of the
CONFIG_KALLSYMS block. And..


/* The command line arguments (may be mangled).  People like
@@ -461,6 +469,7 @@ struct module {
#endif

#ifdef CONFIG_LIVEPATCH
+   bool klp; /* Is this a livepatch module? */
bool klp_alive;
#endif

diff --git a/kernel/module.c b/kernel/module.c
index 8f051a1..433c2d6 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1984,6 +1984,13 @@ static void unset_module_core_ro_nx(struct module *mod) 
{ }
static void unset_module_init_ro_nx(struct module *mod) { }
#endif

+static void free_module_elf(struct module *mod)
+{
+   kfree(mod->hdr);
+   kfree(mod->sechdrs);
+   kfree(mod->secstrings);
+}
+
void __weak module_memfree(void *module_region)
{
vfree(module_region);
@@ -2022,6 +2029,9 @@ 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 */
+   free_module_elf(mod);
+
/* Now we can delete it from the lists */
mutex_lock(_mutex);
/* Unlink carefully: kallsyms could be walking list. */
@@ -2137,6 +2147,10 @@ static int simplify_symbols(struct module *mod, const 
struct load_info *info)
   (long)sym[i].st_value);
break;

+   case SHN_LIVEPATCH:
+   /* klp symbols are resolved by livepatch */
+   break;
+
case SHN_UNDEF:
ksym = resolve_symbol_wait(mod, info, name);
/* Ok if resolved.  */
@@ -2185,6 +2199,10 @@ static int apply_relocations(struct module *mod, const 
struct load_info *info)
if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
continue;

+   /* klp 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);
@@ -2393,6 +2411,11 @@ static char elf_type(const Elf_Sym *sym, const struct 
load_info *info)
{
const Elf_Shdr *sechdrs = info->sechdrs;

+   if (ELF_ST_BIND(sym->st_info) == STB_LIVEPATCH_EXT)
+   return 'K';
+   if (sym->st_shndx == SHN_LIVEPATCH)
+   return 'k';
+
if (ELF_ST_BIND(sym->st_info) == STB_WEAK) {
if (ELF_ST_TYPE(sym->st_info) == STT_OBJECT)
return 'v';
@@ -2475,7 +2498,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 || mod->klp ||
is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum)) {
strtab_size += strlen(>strtab[src[i].st_name])+1;
ndst++;
@@ -2517,7 +2540,7 @@ static void add_kallsyms(struct module *mod, const struct 
load_info *info)
mod->core_strtab = s = mod->module_core + info->stroffs;
src = mod->symtab;
for (ndst = i = 0; i < mod->num_symtab; i++) {
-   if (i == 0 ||
+   if (i == 0 || mod->klp ||
is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum)) {
dst[ndst] = 

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Jessica Yu [30/11/15 23:21 -0500]:

For livepatch modules, copy Elf section, symbol, and string information
from the load_info struct in the module loader.

Livepatch uses special relocation sections in order to be able to patch
modules that are not yet loaded, as well as apply patches to the kernel
when the addresses of symbols cannot be determined at compile time (for
example, when kaslr is enabled). Livepatch modules must preserve Elf
information such as section indices in order to apply the remaining
relocation sections at the appropriate time (i.e. when the target module
loads).

Signed-off-by: Jessica Yu 
---
include/linux/module.h |  9 +
kernel/module.c| 98 --
2 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 3a19c79..9b46256 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -425,6 +425,14 @@ struct module {

/* Notes attributes */
struct module_notes_attrs *notes_attrs;
+
+   /* Elf information (optionally saved) */
+   Elf_Ehdr *hdr;
+   Elf_Shdr *sechdrs;
+   char *secstrings;
+   struct {
+   unsigned int sym, str, mod, vers, info, pcpu;
+   } index;
#endif

/* The command line arguments (may be mangled).  People like
@@ -461,6 +469,7 @@ struct module {
#endif

#ifdef CONFIG_LIVEPATCH
+   bool klp; /* Is this a livepatch module? */


Gah. I believe this field should be outside the #ifdef. 


Jessica
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Jessica Yu]

For livepatch modules, copy Elf section, symbol, and string information
from the load_info struct in the module loader.

Livepatch uses special relocation sections in order to be able to patch
modules that are not yet loaded, as well as apply patches to the kernel
when the addresses of symbols cannot be determined at compile time (for
example, when kaslr is enabled). Livepatch modules must preserve Elf
information such as section indices in order to apply the remaining
relocation sections at the appropriate time (i.e. when the target module
loads).

Signed-off-by: Jessica Yu 
---
 include/linux/module.h |  9 +
 kernel/module.c| 98 --
 2 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 3a19c79..9b46256 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -425,6 +425,14 @@ struct module {
 
/* Notes attributes */
struct module_notes_attrs *notes_attrs;
+
+   /* Elf information (optionally saved) */
+   Elf_Ehdr *hdr;
+   Elf_Shdr *sechdrs;
+   char *secstrings;
+   struct {
+   unsigned int sym, str, mod, vers, info, pcpu;
+   } index;
 #endif
 
/* The command line arguments (may be mangled).  People like
@@ -461,6 +469,7 @@ struct module {
 #endif
 
 #ifdef CONFIG_LIVEPATCH
+   bool klp; /* Is this a livepatch module? */
bool klp_alive;
 #endif
 
diff --git a/kernel/module.c b/kernel/module.c
index 8f051a1..433c2d6 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1984,6 +1984,13 @@ static void unset_module_core_ro_nx(struct module *mod) 
{ }
 static void unset_module_init_ro_nx(struct module *mod) { }
 #endif
 
+static void free_module_elf(struct module *mod)
+{
+   kfree(mod->hdr);
+   kfree(mod->sechdrs);
+   kfree(mod->secstrings);
+}
+
 void __weak module_memfree(void *module_region)
 {
vfree(module_region);
@@ -2022,6 +2029,9 @@ 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 */
+   free_module_elf(mod);
+
/* Now we can delete it from the lists */
mutex_lock(_mutex);
/* Unlink carefully: kallsyms could be walking list. */
@@ -2137,6 +2147,10 @@ static int simplify_symbols(struct module *mod, const 
struct load_info *info)
   (long)sym[i].st_value);
break;
 
+   case SHN_LIVEPATCH:
+   /* klp symbols are resolved by livepatch */
+   break;
+
case SHN_UNDEF:
ksym = resolve_symbol_wait(mod, info, name);
/* Ok if resolved.  */
@@ -2185,6 +2199,10 @@ static int apply_relocations(struct module *mod, const 
struct load_info *info)
if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
continue;
 
+   /* klp 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);
@@ -2393,6 +2411,11 @@ static char elf_type(const Elf_Sym *sym, const struct 
load_info *info)
 {
const Elf_Shdr *sechdrs = info->sechdrs;
 
+   if (ELF_ST_BIND(sym->st_info) == STB_LIVEPATCH_EXT)
+   return 'K';
+   if (sym->st_shndx == SHN_LIVEPATCH)
+   return 'k';
+
if (ELF_ST_BIND(sym->st_info) == STB_WEAK) {
if (ELF_ST_TYPE(sym->st_info) == STT_OBJECT)
return 'v';
@@ -2475,7 +2498,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 || mod->klp ||
is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum)) {
strtab_size += strlen(>strtab[src[i].st_name])+1;
ndst++;
@@ -2517,7 +2540,7 @@ static void add_kallsyms(struct module *mod, const struct 
load_info *info)
mod->core_strtab = s = mod->module_core + info->stroffs;
src = mod->symtab;
for (ndst = i = 0; i < mod->num_symtab; i++) {
-   if (i == 0 ||
+   if (i == 0 || mod->klp ||
is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum)) {
dst[ndst] = src[i];
dst[ndst++].st_name = s - mod->core_strtab;
@@ -2638,6 +2661,64 @@ static int elf_header_check(struct load_info *info)
return 0;
 }
 
+/*
+ * copy_module_elf - preserve 

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

[Jessica Yu]

For livepatch modules, copy Elf section, symbol, and string information
from the load_info struct in the module loader.

Livepatch uses special relocation sections in order to be able to patch
modules that are not yet loaded, as well as apply patches to the kernel
when the addresses of symbols cannot be determined at compile time (for
example, when kaslr is enabled). Livepatch modules must preserve Elf
information such as section indices in order to apply the remaining
relocation sections at the appropriate time (i.e. when the target module
loads).

Signed-off-by: Jessica Yu 
---
 include/linux/module.h |  9 +
 kernel/module.c| 98 --
 2 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 3a19c79..9b46256 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -425,6 +425,14 @@ struct module {
 
/* Notes attributes */
struct module_notes_attrs *notes_attrs;
+
+   /* Elf information (optionally saved) */
+   Elf_Ehdr *hdr;
+   Elf_Shdr *sechdrs;
+   char *secstrings;
+   struct {
+   unsigned int sym, str, mod, vers, info, pcpu;
+   } index;
 #endif
 
/* The command line arguments (may be mangled).  People like
@@ -461,6 +469,7 @@ struct module {
 #endif
 
 #ifdef CONFIG_LIVEPATCH
+   bool klp; /* Is this a livepatch module? */
bool klp_alive;
 #endif
 
diff --git a/kernel/module.c b/kernel/module.c
index 8f051a1..433c2d6 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1984,6 +1984,13 @@ static void unset_module_core_ro_nx(struct module *mod) 
{ }
 static void unset_module_init_ro_nx(struct module *mod) { }
 #endif
 
+static void free_module_elf(struct module *mod)
+{
+   kfree(mod->hdr);
+   kfree(mod->sechdrs);
+   kfree(mod->secstrings);
+}
+
 void __weak module_memfree(void *module_region)
 {
vfree(module_region);
@@ -2022,6 +2029,9 @@ 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 */
+   free_module_elf(mod);
+
/* Now we can delete it from the lists */
mutex_lock(_mutex);
/* Unlink carefully: kallsyms could be walking list. */
@@ -2137,6 +2147,10 @@ static int simplify_symbols(struct module *mod, const 
struct load_info *info)
   (long)sym[i].st_value);
break;
 
+   case SHN_LIVEPATCH:
+   /* klp symbols are resolved by livepatch */
+   break;
+
case SHN_UNDEF:
ksym = resolve_symbol_wait(mod, info, name);
/* Ok if resolved.  */
@@ -2185,6 +2199,10 @@ static int apply_relocations(struct module *mod, const 
struct load_info *info)
if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
continue;
 
+   /* klp 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);
@@ -2393,6 +2411,11 @@ static char elf_type(const Elf_Sym *sym, const struct 
load_info *info)
 {
const Elf_Shdr *sechdrs = info->sechdrs;
 
+   if (ELF_ST_BIND(sym->st_info) == STB_LIVEPATCH_EXT)
+   return 'K';
+   if (sym->st_shndx == SHN_LIVEPATCH)
+   return 'k';
+
if (ELF_ST_BIND(sym->st_info) == STB_WEAK) {
if (ELF_ST_TYPE(sym->st_info) == STT_OBJECT)
return 'v';
@@ -2475,7 +2498,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 || mod->klp ||
is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum)) {
strtab_size += strlen(>strtab[src[i].st_name])+1;
ndst++;
@@ -2517,7 +2540,7 @@ static void add_kallsyms(struct module *mod, const struct 
load_info *info)
mod->core_strtab = s = mod->module_core + info->stroffs;
src = mod->symtab;
for (ndst = i = 0; i < mod->num_symtab; i++) {
-   if (i == 0 ||
+   if (i == 0 || mod->klp ||
is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum)) {
dst[ndst] = src[i];
dst[ndst++].st_name = s - mod->core_strtab;
@@ -2638,6 +2661,64 @@ static int elf_header_check(struct load_info *info)
return 0;
 }
 
+/*
+ *