Re: [PATCHv2 2/3] kernel: add support for live patching

[Seth Jennings <sjenning@redhat.com>]

On Tue, Nov 18, 2014 at 03:45:22PM +0100, Miroslav Benes wrote:
> 
> On Sun, 16 Nov 2014, Seth Jennings wrote:
> 
> [...]
> 
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > new file mode 100644
> > index 0000000..8b68fef
> > --- /dev/null
> > +++ b/include/linux/livepatch.h
> > @@ -0,0 +1,68 @@
> > +/*
> > + * livepatch.h - Live Kernel Patching Core
> > + *
> > + * Copyright (C) 2014 Seth Jennings <sjenning@redhat.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#ifndef _LINUX_LIVEPATCH_H_
> > +#define _LINUX_LIVEPATCH_H_
> > +
> > +#include <linux/module.h>
> > +
> 
> I think we need something like 
> 
> #if IS_ENABLED(CONFIG_LIVE_PATCHING)
> 
> here. Otherwise kernel module with live patch itself would be built 
> even with live patching support disabled (as the structures and needed 
> functions are declared).

What do you think of this (already includes s/lp/klp/ change)?

====
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 0143b73..a9821f3 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -21,6 +21,7 @@
 #define _LINUX_LIVEPATCH_H_
 
 #include <linux/module.h>
+#include <asm/livepatch.h>
 
 /* TODO: add kernel-doc for structures once agreed upon */
 
@@ -58,11 +59,20 @@ struct klp_patch {
        struct klp_object *objs;
 };
 
-int klp_register_patch(struct klp_patch *);
-int klp_unregister_patch(struct klp_patch *);
-int klp_enable_patch(struct klp_patch *);
-int klp_disable_patch(struct klp_patch *);
+#ifdef CONFIG_LIVE_PATCHING
 
-#include <asm/livepatch.h>
+extern int klp_register_patch(struct klp_patch *);
+extern int klp_unregister_patch(struct klp_patch *);
+extern int klp_enable_patch(struct klp_patch *);
+extern int klp_disable_patch(struct klp_patch *);
+
+#else /* !CONFIG_LIVE_PATCHING */
+
+static int klp_register_patch(struct klp_patch *k) { return -ENOSYS; }
+static int klp_unregister_patch(struct klp_patch *k) { return -ENOSYS; }
+static int klp_enable_patch(struct klp_patch *k) { return -ENOSYS; }
+static int klp_disable_patch(struct klp_patch *k) { return -ENOSYS; }
+
+#endif
====

This seems to be the way many headers handle this.  Patch modules built
against a kernel that doesn't support live patching will build cleanly,
but will always fail to load.

Seth

> 
> > +/* TODO: add kernel-doc for structures once agreed upon */
> > +
> > +struct lp_func {
> > +	const char *old_name; /* function to be patched */
> > +	void *new_func; /* replacement function in patch module */
> > +	/*
> > +	 * The old_addr field is optional and can be used to resolve
> > +	 * duplicate symbol names in the vmlinux object.  If this
> > +	 * information is not present, the symbol is located by name
> > +	 * with kallsyms. If the name is not unique and old_addr is
> > +	 * not provided, the patch application fails as there is no
> > +	 * way to resolve the ambiguity.
> > +	 */
> > +	unsigned long old_addr;
> > +};
> > +
> > +struct lp_reloc {
> > +	unsigned long dest;
> > +	unsigned long src;
> > +	unsigned long type;
> > +	const char *name;
> > +	int addend;
> > +	int external;
> > +};
> > +
> > +struct lp_object {
> > +	const char *name; /* "vmlinux" or module name */
> > +	struct lp_func *funcs;
> > +	struct lp_reloc *relocs;
> > +};
> > +
> > +struct lp_patch {
> > +	struct module *mod; /* module containing the patch */
> > +	struct lp_object *objs;
> > +};
> > +
> > +int lp_register_patch(struct lp_patch *);
> > +int lp_unregister_patch(struct lp_patch *);
> > +int lp_enable_patch(struct lp_patch *);
> > +int lp_disable_patch(struct lp_patch *);
> > +
> > +#include <asm/livepatch.h>
> 
> and #endif for CONFIG_LIVE_PATCHING here.
> 
> > +
> > +#endif /* _LINUX_LIVEPATCH_H_ */
> 
> Thanks,
> --
> Miroslav Benes
> SUSE Labs