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 <jeyu@xxxxxxxxxx>
> > >>---
> > >> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/