Re: [PATCH v4 15/16] module: Move where we mark modules RO,X

Re: [PATCH v4 15/16] module: Move where we mark modules RO,X

[Josh Poimboeuf]

On Wed, Oct 23, 2019 at 01:48:35PM +0200, Peter Zijlstra wrote:
> Now sadly that commit missed all the useful information, luckily I could
> find the patch in my LKML folder, more sad, that thread still didn't
> contain the actual useful information, for that I was directed to
> github:
> 
>   https://github.com/dynup/kpatch/issues/580
> 
> Now, someone is owning me a beer for having to look at github for this.

Deal.  And you probably deserve a few more for fixing our crap.

The github thing is supposed to be temporary, at least in theory we'll
eventually have all klp patch module building code in the kernel tree.

> That finally explained that what happens is that the RELA was trying to
> fix up the paravirt indirect call to 'local_irq_disable', which
> apply_paravirt() will have overwritten with 'CLI; NOP'. This then
> obviously goes *bang*.
> 
> This then raises a number of questions:
> 
>  1) why is that RELA (that obviously does not depend on any module)
>     applied so late?

Good question.  The 'pv_ops' symbol is exported by the core kernel, so I
can't see any reason why we'd need to apply that rela late.  In theory,
kpatch-build isn't supposed to convert that to a klp rela.  Maybe
something went wrong in the patch creation code.

I'm also questioning why we even need to apply the parainstructions
section late.  Maybe we can remove that apply_paravirt() call
altogether, along with .klp.arch.parainstruction sections.

I'll need to look into it...

>  2) why can't we unconditionally skip RELA's to paravirt sites?

We could, but I don't think it's needed if we fix #1.

>  3) Is there ever a possible module-dependent RELA to a paravirt /
>     alternative site?

Good question...

> Now, for 1), I would propose '.klp.rela.${mod}' sections only contain
> RELAs that depend on symbols in ${mod} (or modules in general).

That was already the goal, but we've apparently failed at that.

> We can fix up RELAs that depend on core kernel early without problems.
> Let them be in the normal .rela sections and be fixed up on loading
> the patch-module as per usual.

If such symbols aren't exported, then they still need to be in
.klp.rela.vmlinux sections, since normal relas won't work.

> This should also deal with 2, paravirt should always have RELAs into the
> core kernel.
> 
> Then for 3) we only have alternatives left, and I _think_ it unlikely to
> be the case, but I'll have to have a hard look at that.

I'm not sure about alternatives, but maybe we can enforce such
limitations with tooling and/or kernel checks.

-- 
Josh

Re: [PATCH v4 15/16] module: Move where we mark modules RO,X

[Peter Zijlstra]

On Wed, Oct 23, 2019 at 12:00:25PM -0500, Josh Poimboeuf wrote:

> > This then raises a number of questions:
> > 
> >  1) why is that RELA (that obviously does not depend on any module)
> >     applied so late?
> 
> Good question.  The 'pv_ops' symbol is exported by the core kernel, so I
> can't see any reason why we'd need to apply that rela late.  In theory,
> kpatch-build isn't supposed to convert that to a klp rela.  Maybe
> something went wrong in the patch creation code.
> 
> I'm also questioning why we even need to apply the parainstructions
> section late.  Maybe we can remove that apply_paravirt() call
> altogether, along with .klp.arch.parainstruction sections.
> 
> I'll need to look into it...

Right, that really should be able to run early. Esp. after commit

  11e86dc7f274 ("x86/paravirt: Detect over-sized patching bugs in paravirt_patch_call()")

paravirt patching is unconditional. We _never_ run with the indirect
call except very early boot, but modules should have them patched way
before their init section runs.

We rely on this for spectre-v2 and friends.

> >  3) Is there ever a possible module-dependent RELA to a paravirt /
> >     alternative site?
> 
> Good question...

> > Then for 3) we only have alternatives left, and I _think_ it unlikely to
> > be the case, but I'll have to have a hard look at that.
> 
> I'm not sure about alternatives, but maybe we can enforce such
> limitations with tooling and/or kernel checks.

Right, so on IRC you implied you might have some additional details on
how alternatives were affected; did you manage to dig that up?

Re: [PATCH v4 15/16] module: Move where we mark modules RO,X

[Petr Mladek]

On Thu 2019-10-24 15:16:34, Peter Zijlstra wrote:
> On Wed, Oct 23, 2019 at 12:00:25PM -0500, Josh Poimboeuf wrote:
> 
> > > This then raises a number of questions:
> > > 
> > >  1) why is that RELA (that obviously does not depend on any module)
> > >     applied so late?
> > 
> > Good question.  The 'pv_ops' symbol is exported by the core kernel, so I
> > can't see any reason why we'd need to apply that rela late.  In theory,
> > kpatch-build isn't supposed to convert that to a klp rela.  Maybe
> > something went wrong in the patch creation code.
> > 
> > I'm also questioning why we even need to apply the parainstructions
> > section late.  Maybe we can remove that apply_paravirt() call
> > altogether, along with .klp.arch.parainstruction sections.

Hmm, the original bug report against livepatching was actually about
paravirt ops, see below.


> > I'll need to look into it...
> 
> Right, that really should be able to run early. Esp. after commit
> 
>   11e86dc7f274 ("x86/paravirt: Detect over-sized patching bugs in paravirt_patch_call()")
> 
> paravirt patching is unconditional. We _never_ run with the indirect
> call except very early boot, but modules should have them patched way
> before their init section runs.
> 
> We rely on this for spectre-v2 and friends.

Livepatching has the same requirement. The module code has to be fully
livepatched before the module gets actually used. It means before
mod->init() is called and before the module is moved into
MODULE_STATE_LIVE state.


> > >  3) Is there ever a possible module-dependent RELA to a paravirt /
> > >     alternative site?
> > 
> > Good question...
> 
> > > Then for 3) we only have alternatives left, and I _think_ it unlikely to
> > > be the case, but I'll have to have a hard look at that.
> > 
> > I'm not sure about alternatives, but maybe we can enforce such
> > limitations with tooling and/or kernel checks.
> 
> Right, so on IRC you implied you might have some additional details on
> how alternatives were affected; did you manage to dig that up?

I am not sure what Josh had in mind. But the problem with livepatches,
paravort ops, and alternatives was described in the related patchset, see
https://lkml.kernel.org/r/1471481911-5003-1-git-send-email-jeyu@redhat.com

The original bug report is
https://lkml.kernel.org/r/20160329120518.GA21252@canonical.com

Best Regards,
Petr

Re: [PATCH v4 15/16] module: Move where we mark modules RO,X

[Peter Zijlstra]

On Fri, Oct 25, 2019 at 08:44:56AM +0200, Petr Mladek wrote:
> On Thu 2019-10-24 15:16:34, Peter Zijlstra wrote:
> > On Wed, Oct 23, 2019 at 12:00:25PM -0500, Josh Poimboeuf wrote:
> > 
> > > > This then raises a number of questions:
> > > > 
> > > >  1) why is that RELA (that obviously does not depend on any module)
> > > >     applied so late?
> > > 
> > > Good question.  The 'pv_ops' symbol is exported by the core kernel, so I
> > > can't see any reason why we'd need to apply that rela late.  In theory,
> > > kpatch-build isn't supposed to convert that to a klp rela.  Maybe
> > > something went wrong in the patch creation code.
> > > 
> > > I'm also questioning why we even need to apply the parainstructions
> > > section late.  Maybe we can remove that apply_paravirt() call
> > > altogether, along with .klp.arch.parainstruction sections.
> 
> Hmm, the original bug report against livepatching was actually about
> paravirt ops, see below.

Yes, I found that.

> > > I'm not sure about alternatives, but maybe we can enforce such
> > > limitations with tooling and/or kernel checks.
> > 
> > Right, so on IRC you implied you might have some additional details on
> > how alternatives were affected; did you manage to dig that up?
> 
> I am not sure what Josh had in mind. But the problem with livepatches,
> paravort ops, and alternatives was described in the related patchset, see
> https://lkml.kernel.org/r/1471481911-5003-1-git-send-email-jeyu@redhat.com

Yes, and my complaint there is that that thread is void of useful
content.

> The original bug report is
> https://lkml.kernel.org/r/20160329120518.GA21252@canonical.com

I found the github (*groan*) link in the thread above.

From all that I could only make that the paravirt stuff is just doing it
wrong (see earlier emails, core-kernel RELAs really should be applied at
the time of patch-module load, there's no excuse for them to be delayed
to the .klp.rela. section) at which point paravirt will also magically
work.

But none of that explains why apply_alternatives() is also delayed.

So I'm very tempted to just revert that patchset for doing it all
wrong.

Re: [PATCH v4 15/16] module: Move where we mark modules RO,X

[Peter Zijlstra]

On Fri, Oct 25, 2019 at 10:43:00AM +0200, Peter Zijlstra wrote:

> But none of that explains why apply_alternatives() is also delayed.
> 
> So I'm very tempted to just revert that patchset for doing it all
> wrong.

And I've done just that. This includes Josh's validation patch, the
revert and my klp_appy_relocations_add() patches with the removal of
module_disable_ro().

Josh, can you test or give me clue on how to test? I need to run a few
errands today, but I'll try and have a poke either tonight or tomorrow.

  git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/rwx

After this it is waiting for Mark's argh64 patches to land:

  https://lkml.kernel.org/r/20191021163426.9408-5-mark.rutland@arm.com

And then we can go and delete module_disable_ro() entirely -- hooray!

Re: [PATCH v4 15/16] module: Move where we mark modules RO,X

[Josh Poimboeuf]

On Fri, Oct 25, 2019 at 12:06:12PM +0200, Peter Zijlstra wrote:
> On Fri, Oct 25, 2019 at 10:43:00AM +0200, Peter Zijlstra wrote:
> 
> > But none of that explains why apply_alternatives() is also delayed.
> > 
> > So I'm very tempted to just revert that patchset for doing it all
> > wrong.
> 
> And I've done just that. This includes Josh's validation patch, the
> revert and my klp_appy_relocations_add() patches with the removal of
> module_disable_ro().
> 
> Josh, can you test or give me clue on how to test? I need to run a few
> errands today, but I'll try and have a poke either tonight or tomorrow.
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/rwx

Thanks.  I'll work on hacking up kpatch-build to support this, and then
I'll need to run it through a lot of testing to make sure this was a
good idea.

-- 
Josh

Re: [PATCH v4 15/16] module: Move where we mark modules RO,X

[Josh Poimboeuf]

On Fri, Oct 25, 2019 at 12:06:12PM +0200, Peter Zijlstra wrote:
> On Fri, Oct 25, 2019 at 10:43:00AM +0200, Peter Zijlstra wrote:
> 
> > But none of that explains why apply_alternatives() is also delayed.
> > 
> > So I'm very tempted to just revert that patchset for doing it all
> > wrong.
> 
> And I've done just that. This includes Josh's validation patch, the
> revert and my klp_appy_relocations_add() patches with the removal of
> module_disable_ro().
> 
> Josh, can you test or give me clue on how to test? I need to run a few
> errands today, but I'll try and have a poke either tonight or tomorrow.
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/rwx

I looked at this today.  A few potential tweaks:

- The new klp_apply_relocate_add() interface isn't needed.  Instead
  apply_relocate_add() can use the module state to decide whether to
  text_poke().

- For robustness I think we need to apply vmlinux-specific klp
  relocations at the same time as normal relocations.

Rough untested changes below.  I still need to finish changing
kpatch-build and then I'll need to do a LOT of testing.

I can take over the livepatch-specific patches if you want.  Or however
you want to do it.

diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
index 7fc519b9b4e0..6a70213854f0 100644
--- a/arch/s390/kernel/module.c
+++ b/arch/s390/kernel/module.c
@@ -451,14 +451,11 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
 		       unsigned int symindex, unsigned int relsec,
 		       struct module *me)
 {
-	return __apply_relocate_add(sechdrs, strtab, symindex, relsec, me, memwrite);
-}
+	int ret;
+	bool early = me->state != MODULE_STATE_LIVE;
 
-int klp_apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
-		       unsigned int symindex, unsigned int relsec,
-		       struct module *me)
-{
-	return __apply_relocate_add(sechdrs, strtab, symindex, relsec, me, s390_kernel_write);
+	return __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
+				    early ? memwrite : s390_kernel_write);
 }
 
 int module_finalize(const Elf_Ehdr *hdr,
diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
index 5eee618a98c5..30174798ff79 100644
--- a/arch/x86/kernel/module.c
+++ b/arch/x86/kernel/module.c
@@ -222,20 +222,14 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 		   unsigned int symindex,
 		   unsigned int relsec,
 		   struct module *me)
-{
-	return __apply_relocate_add(sechdrs, strtab, symindex, relsec, me, memcpy);
-}
-
-int klp_apply_relocate_add(Elf64_Shdr *sechdrs,
-		   const char *strtab,
-		   unsigned int symindex,
-		   unsigned int relsec,
-		   struct module *me)
 {
 	int ret;
+	bool early = me->state != MODULE_STATE_LIVE;
+
+	ret = __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
+				   early ? memcpy : text_poke);
 
-	ret = __apply_relocate_add(sechdrs, strtab, symindex, relsec, me, text_poke);
-	if (!ret)
+	if (!ret && !early)
 		text_poke_sync();
 
 	return ret;
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index cc18f945bdb2..b00170696db2 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -214,12 +214,7 @@ void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
 void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor);
 void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor);
 
-
-extern int klp_apply_relocate_add(Elf64_Shdr *sechdrs,
-			      const char *strtab,
-			      unsigned int symindex,
-			      unsigned int relsec,
-			      struct module *me);
+int klp_write_relocations(struct module *mod, struct klp_object *obj);
 
 #else /* !CONFIG_LIVEPATCH */
 
@@ -229,6 +224,12 @@ static inline bool klp_patch_pending(struct task_struct *task) { return false; }
 static inline void klp_update_patch_state(struct task_struct *task) {}
 static inline void klp_copy_process(struct task_struct *child) {}
 
+static inline int klp_write_relocations(struct module *mod,
+					struct klp_object *obj)
+{
+	return 0;
+}
+
 #endif /* CONFIG_LIVEPATCH */
 
 #endif /* _LINUX_LIVEPATCH_H_ */
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 30395302a273..52eb91d0ee8d 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -256,27 +256,60 @@ static int klp_resolve_symbols(Elf_Shdr *relasec, struct module *pmod)
 	return 0;
 }
 
-int __weak klp_apply_relocate_add(Elf64_Shdr *sechdrs,
-			      const char *strtab,
-			      unsigned int symindex,
-			      unsigned int relsec,
-			      struct module *me)
-{
-	return apply_relocate_add(sechdrs, strtab, symindex, relsec, me);
-}
-
-static int klp_write_object_relocations(struct module *pmod,
-					struct klp_object *obj)
+/*
+ * This function is called for both vmlinux-specific and module-specific klp
+ * relocation sections:
+ *
+ * 1) When the klp module itself loads, the module code calls this function
+ *    to write vmlinux-specific klp relocations.  These relocations allow the
+ *    patched code/data to reference unexported vmlinux symbols.  They're
+ *    written as early as possible to ensure that other module init code
+ *    (.e.g., jump_label_apply_nops) can access any non-exported vmlinux
+ *    symbols which might be referenced by the klp module's special sections.
+ *
+ * 2) When a to-be-patched module loads (or is already loaded when the
+ *    klp module loads), klp code calls this function to write klp relocations
+ *    which are specific to the module.  These relocations allow the patched
+ *    code/data to reference module symbols, both unexported and exported.
+ *    They also enable late module patching, which means the to-be-patched
+ *    module may be loaded *after* the klp module.
+ *
+ *    The following restrictions apply to module-specific relocation sections:
+ *
+ *    a) References to vmlinux symbols are not allowed.  Otherwise there might
+ *       be module init ordering issues, and crashes might occur in some of the
+ *       other kernel patching components like paravirt patching or jump
+ *       labels.  All references to vmlinux symbols should use either normal
+ *       relas (for exported symbols) or vmlinux-specific klp relas (for
+ *       unexported symbols).  This restriction is enforced in
+ *       klp_resolve_symbols().
+ *
+ *    b) Relocations to special sections like __jump_table and .altinstructions
+ *       aren't allowed.  In other words, there should never be a
+ *       .klp.rela.{module}.__jump_table section.  This will definitely cause
+ *       initialization ordering issues, as such special sections are processed
+ *       during the loading of the klp module itself, *not* the to-be-patched
+ *       module.  This means that e.g., it's not currently possible to patch a
+ *       module function which uses a static key jump label, if you want to
+ *       have the replacement function also use the same static key.  In this
+ *       case, a non-static interface like static_key_enabled() can be used in
+ *       the new function instead.
+ *
+ *       On the other hand, a .klp.rela.vmlinux.__jump_table section is fine,
+ *       as it can be resolved early enough during the load of the klp module,
+ *       as described above.
+ */
+int klp_write_relocations(struct module *pmod, struct klp_object *obj)
 {
 	int i, cnt, ret = 0;
 	const char *objname, *secname;
 	char sec_objname[MODULE_NAME_LEN];
 	Elf_Shdr *sec;
 
-	if (WARN_ON(!klp_is_object_loaded(obj)))
+	if (WARN_ON(obj && !klp_is_object_loaded(obj)))
 		return -EINVAL;
 
-	objname = klp_is_module(obj) ? obj->name : "vmlinux";
+	objname = obj ? obj->name : "vmlinux";
 
 	/* For each klp relocation section */
 	for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
@@ -305,7 +338,7 @@ static int klp_write_object_relocations(struct module *pmod,
 		if (ret)
 			break;
 
-		ret = klp_apply_relocate_add(pmod->klp_info->sechdrs,
+		ret = apply_relocate_add(pmod->klp_info->sechdrs,
 					 pmod->core_kallsyms.strtab,
 					 pmod->klp_info->symndx, i, pmod);
 		if (ret)
@@ -733,16 +766,25 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 	struct klp_func *func;
 	int ret;
 
-	mutex_lock(&text_mutex);
+	if (klp_is_module(obj)) {
+
+		/*
+		 * Only write module-specific relocations here.
+		 * vmlinux-specific relocations were already written during the
+		 * loading of the klp module.
+		 */
+
+		mutex_lock(&text_mutex);
+
+		ret = klp_write_relocations(patch->mod, obj);
+		if (ret) {
+			mutex_unlock(&text_mutex);
+			return ret;
+		}
 
-	ret = klp_write_object_relocations(patch->mod, obj);
-	if (ret) {
 		mutex_unlock(&text_mutex);
-		return ret;
 	}
 
-	mutex_unlock(&text_mutex);
-
 	klp_for_each_func(obj, func) {
 		ret = klp_find_object_symbol(obj->name, func->old_name,
 					     func->old_sympos,
diff --git a/kernel/module.c b/kernel/module.c
index fe5bd382759c..ff4347385f05 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2327,11 +2327,9 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
 		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
 			continue;
 
-		/* Livepatch relocation sections are applied by livepatch */
 		if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
-			continue;
-
-		if (info->sechdrs[i].sh_type == SHT_REL)
+			err = klp_write_relocations(mod, NULL);
+		else if (info->sechdrs[i].sh_type == SHT_REL)
 			err = apply_relocate(info->sechdrs, info->strtab,
 					     info->index.sym, i, mod);
 		else if (info->sechdrs[i].sh_type == SHT_RELA)
@@ -3812,18 +3810,24 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	/* Set up MODINFO_ATTR fields */
 	setup_modinfo(mod, info);
 
+	if (is_livepatch_module(mod)) {
+		err = copy_module_elf(mod, info);
+		if (err < 0)
+			goto free_modinfo;
+	}
+
 	/* Fix up syms, so that st_value is a pointer to location. */
 	err = simplify_symbols(mod, info);
 	if (err < 0)
-		goto free_modinfo;
+		goto free_elf_copy;
 
 	err = apply_relocations(mod, info);
 	if (err < 0)
-		goto free_modinfo;
+		goto free_elf_copy;
 
 	err = post_relocation(mod, info);
 	if (err < 0)
-		goto free_modinfo;
+		goto free_elf_copy;
 
 	flush_module_icache(mod);
 
@@ -3866,12 +3870,6 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	if (err < 0)
 		goto coming_cleanup;
 
-	if (is_livepatch_module(mod)) {
-		err = copy_module_elf(mod, info);
-		if (err < 0)
-			goto sysfs_cleanup;
-	}
-
 	/* Get rid of temporary copy. */
 	free_copy(info);
 
@@ -3880,8 +3878,6 @@ static int load_module(struct load_info *info, const char __user *uargs,
 
 	return do_init_module(mod);
 
- sysfs_cleanup:
-	mod_sysfs_teardown(mod);
  coming_cleanup:
 	mod->state = MODULE_STATE_GOING;
 	destroy_params(mod->kp, mod->num_kp);
@@ -3901,6 +3897,8 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	kfree(mod->args);
  free_arch_cleanup:
 	module_arch_cleanup(mod);
+ free_elf_copy:
+	free_module_elf(mod);
  free_modinfo:
 	free_modinfo(mod);
  free_unload:

Re: [PATCH v4 15/16] module: Move where we mark modules RO,X

[Peter Zijlstra]

On Fri, Oct 25, 2019 at 08:17:41PM -0500, Josh Poimboeuf wrote:

> + *    The following restrictions apply to module-specific relocation sections:
> + *
> + *    a) References to vmlinux symbols are not allowed.  Otherwise there might
> + *       be module init ordering issues, and crashes might occur in some of the
> + *       other kernel patching components like paravirt patching or jump
> + *       labels.  All references to vmlinux symbols should use either normal
> + *       relas (for exported symbols) or vmlinux-specific klp relas (for
> + *       unexported symbols).  This restriction is enforced in
> + *       klp_resolve_symbols().

Right.

> + *    b) Relocations to special sections like __jump_table and .altinstructions
> + *       aren't allowed.  In other words, there should never be a
> + *       .klp.rela.{module}.__jump_table section.  This will definitely cause
> + *       initialization ordering issues, as such special sections are processed
> + *       during the loading of the klp module itself, *not* the to-be-patched
> + *       module.  This means that e.g., it's not currently possible to patch a
> + *       module function which uses a static key jump label, if you want to
> + *       have the replacement function also use the same static key.  In this
> + *       case, a non-static interface like static_key_enabled() can be used in
> + *       the new function instead.

Idem for .static_call_sites I suppose..

Is there any enforcement on this? I'm thinking it should be possible to
detect the presence of these sections and yell a bit.

OTOH, it should be possible to actually handle this, but let's do that
later.

> + *       On the other hand, a .klp.rela.vmlinux.__jump_table section is fine,
> + *       as it can be resolved early enough during the load of the klp module,
> + *       as described above.
> + */

> diff --git a/kernel/module.c b/kernel/module.c
> index fe5bd382759c..ff4347385f05 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2327,11 +2327,9 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
>  		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
>  			continue;
>  
> -		/* Livepatch relocation sections are applied by livepatch */
>  		if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
> -			continue;
> -
> -		if (info->sechdrs[i].sh_type == SHT_REL)
> +			err = klp_write_relocations(mod, NULL);
> +		else if (info->sechdrs[i].sh_type == SHT_REL)
>  			err = apply_relocate(info->sechdrs, info->strtab,
>  					     info->index.sym, i, mod);
>  		else if (info->sechdrs[i].sh_type == SHT_RELA)

Like here, we can yell and error if .klp.rela.{mod}.__jump_table
sections are encountered.


Other than that, this should work I suppose.

Re: [PATCH v4 15/16] module: Move where we mark modules RO,X

[Peter Zijlstra]

On Fri, Oct 25, 2019 at 08:17:41PM -0500, Josh Poimboeuf wrote:

> I can take over the livepatch-specific patches if you want.  Or however
> you want to do it.

Sure, feel free to take and route the livepatch patches. Then I'll wait
until those and the ARM64 patches land before I'll pick this up again.

Re: [PATCH v4 15/16] module: Move where we mark modules RO,X

[Peter Zijlstra]

On Fri, Oct 25, 2019 at 08:44:56AM +0200, Petr Mladek wrote:
> On Thu 2019-10-24 15:16:34, Peter Zijlstra wrote:

> > Right, that really should be able to run early. Esp. after commit
> > 
> >   11e86dc7f274 ("x86/paravirt: Detect over-sized patching bugs in paravirt_patch_call()")
> > 
> > paravirt patching is unconditional. We _never_ run with the indirect
> > call except very early boot, but modules should have them patched way
> > before their init section runs.
> > 
> > We rely on this for spectre-v2 and friends.
> 
> Livepatching has the same requirement. The module code has to be fully
> livepatched before the module gets actually used.

Right, and that is just saying that all paravirt RELAs (pv_ops) can
basically be deleted from modules.

Which avoids the reported problem in yet another way.

> It means before mod->init() is called and before the module is moved
> into MODULE_STATE_LIVE state.

Funny thing, currently ftrace is running code before all that. It runs
code before klp_module_coming(), before jump_label patching.

My other patch in this thread fixes that.