Re: module: save load_info for livepatch modules

On Wed 2015-11-11 23:44:08, Jessica Yu wrote:

I see. It looks safe at this point. But still I wonder if it would be
possible to calculate this later in the livepatch code. It will allow
to potentially use the info structure also by other subsystem.

BTW: Where is "sh_addr" value used, please? I see it used only
in the third patch as info->sechdrs[relindex].sh_addr. But it is
an array. I am not sure if it is the same variable.

quoted

quoted

+	mod->info = kzalloc(sizeof(*info), GFP_KERNEL);
+	memcpy(mod->info, info, sizeof(*info));
+
+}

It is strange that this funtion is defined in livepatch/core.c
but declared in module.h. I would move the definition to
module.c.

Right, I was trying to keep all the livepatch-related functions
together in livepatch/core.c. but I can move it to module.c if it
makes more sense/Rusty doesn't object to it :-)

Sure. I think that we could use some generic name, e.g. copy_module_info().

quoted

quoted

static int __init klp_init(void)
{
	int ret;

diff --git a/kernel/module.c b/kernel/module.c
index 8f051a1..8ae3ca5 100644
--- a/kernel/module.c
+++ b/kernel/module.c

@@ -2137,6 +2123,11 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)

			       (long)sym[i].st_value);
			break;

+#ifdef CONFIG_LIVEPATCH
+		case SHN_LIVEPATCH:
+			break;
+#endif

IMHO, even a kernel compiled without CONFIG_LIVEPATCH should handle livepatch
modules with grace. It means to reject loading.

I think even right now, without considering this patchset, we don't
reject modules "gracefully" when we load a livepatch module without
CONFIG_LIVEPATCH. The module loader will complain and reject the
livepatch module, saying something like "Unknown symbol
klp_register_patch." This behavior is the same with or without
this patch series applied. If we want to add a bit more logic to
gracefully reject patch modules, perhaps that should be a different
patch altogether, as I think it is unrelated to the goal of this one :-)

Yup, the module load would fail anyway because of the missing symbol.
But I think that we should fail on the first error occurence.

In each case, IMHO, we should not do the "default:" action for this
section even when complied without CONFIG_LIVEPATCH.

quoted

quoted

		case SHN_UNDEF:
			ksym = resolve_symbol_wait(mod, info, name);
			/* Ok if resolved.  */

@@ -2185,6 +2176,11 @@ static int apply_relocations(struct module *mod, const struct load_info *info)

		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
			continue;

+#ifdef CONFIG_LIVEPATCH
+		if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
+			continue;
+#endif

I guess that if we do not trigger the error above, and do
not have the check here, we will try to call apply_relocate() below.
I guess that it will fail. If we are lucky it will print "Unknown
relocation". I think that we could do better.

quoted

quoted

+
		if (info->sechdrs[i].sh_type == SHT_REL)
			err = apply_relocate(info->sechdrs, info->strtab,
					     info->index.sym, i, mod);

@@ -3530,8 +3526,20 @@ static int load_module(struct load_info *info, const char __user *uargs,

	if (err < 0)
		goto bug_cleanup;

+#ifdef CONFIG_LIVEPATCH
+	/*
+	 * 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 (get_modinfo((struct load_info *)info, "livepatch"))
+		klp_prepare_patch_module(mod, info);
+	else
+		free_copy(info);
+#else

I would move this #else one line above and get rid of the
double free_copy(info); But it is a matter of taste.

Maybe I'm missing something, but I think we do need the double
free_copy(), because in the CONFIG_LIVEPATCH case, we still want to
call free_copy() for non-livepatch modules. And we want to avoid
calling free_copy() for livepatch modules (hence the extra else).

Ah, this was just a cosmetic change. I meant to use something like:

#ifdef CONFIG_LIVEPATCH
	/*
	 * 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 (get_modinfo((struct load_info *)info, "livepatch"))
		klp_prepare_patch_module(mod, info);
	else
#endif
		/* Get rid of temporary copy. */
		free_copy(info);


It is a matter of taste. Maybe, your variant was better in the end.

Thank you,
Petr