| Message ID | 20181113130730.44844-3-namit@vmware.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/alternative: text_poke() fixes | expand |
On 11/13/18 5:07 AM, Nadav Amit wrote: > There is no apparent reason not to use text_poke_early() while we are > during early-init and we do not patch code that might be on the stack > (i.e., we'll return to the middle of the patched code). This appears to > be the case of jump-labels, so do so. > > This is required for the next patches that would set a temporary mm for > patching, which is initialized after some static-keys are > enabled/disabled. > > Cc: Andy Lutomirski <luto@kernel.org> > Cc: Kees Cook <keescook@chromium.org> > Cc: Dave Hansen <dave.hansen@intel.com> > Cc: Masami Hiramatsu <mhiramat@kernel.org> > Co-Developed-by: Peter Zijlstra <peterz@infradead.org> > Signed-off-by: Nadav Amit <namit@vmware.com> > --- > arch/x86/kernel/jump_label.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c > index aac0c1f7e354..ed5fe274a7d8 100644 > --- a/arch/x86/kernel/jump_label.c > +++ b/arch/x86/kernel/jump_label.c > @@ -52,7 +52,12 @@ static void __ref __jump_label_transform(struct jump_entry *entry, > jmp.offset = jump_entry_target(entry) - > (jump_entry_code(entry) + JUMP_LABEL_NOP_SIZE); > > - if (early_boot_irqs_disabled) > + /* > + * As long as we're UP and not yet marked RO, we can use > + * text_poke_early; SYSTEM_BOOTING guarantees both, as we switch to > + * SYSTEM_SCHEDULING before going either. > + */ > + if (system_state == SYSTEM_BOOTING) > poker = text_poke_early; > > if (type == JUMP_LABEL_JMP) { > Can't we make this test in text_poke() directly, please? -hpa
On Tue, Nov 20, 2018 at 10:10:14AM -0800, H. Peter Anvin wrote: > On 11/13/18 5:07 AM, Nadav Amit wrote: > > There is no apparent reason not to use text_poke_early() while we are > > during early-init and we do not patch code that might be on the stack > > (i.e., we'll return to the middle of the patched code). This appears to > > be the case of jump-labels, so do so. > > > > This is required for the next patches that would set a temporary mm for > > patching, which is initialized after some static-keys are > > enabled/disabled. > > > > Cc: Andy Lutomirski <luto@kernel.org> > > Cc: Kees Cook <keescook@chromium.org> > > Cc: Dave Hansen <dave.hansen@intel.com> > > Cc: Masami Hiramatsu <mhiramat@kernel.org> > > Co-Developed-by: Peter Zijlstra <peterz@infradead.org> > > Signed-off-by: Nadav Amit <namit@vmware.com> > > --- > > arch/x86/kernel/jump_label.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c > > index aac0c1f7e354..ed5fe274a7d8 100644 > > --- a/arch/x86/kernel/jump_label.c > > +++ b/arch/x86/kernel/jump_label.c > > @@ -52,7 +52,12 @@ static void __ref __jump_label_transform(struct jump_entry *entry, > > jmp.offset = jump_entry_target(entry) - > > (jump_entry_code(entry) + JUMP_LABEL_NOP_SIZE); > > > > - if (early_boot_irqs_disabled) > > + /* > > + * As long as we're UP and not yet marked RO, we can use > > + * text_poke_early; SYSTEM_BOOTING guarantees both, as we switch to > > + * SYSTEM_SCHEDULING before going either. > > + */ > > + if (system_state == SYSTEM_BOOTING) > > poker = text_poke_early; > > > > if (type == JUMP_LABEL_JMP) { > > > > Can't we make this test in text_poke() directly, please? He does that in 9/10 iirc.
On 11/20/18 10:18 AM, Peter Zijlstra wrote: >> >> Can't we make this test in text_poke() directly, please? > > He does that in 9/10 iirc. > No, in 9/10 he does that change locally for the jump_label, but there is absolutely no reason not to do that test in text_poke() proper, and simply use text_poke() everywhere in the kernel. -hpa
> On Nov 20, 2018, at 10:23 AM, H. Peter Anvin <hpa@zytor.com> wrote: > > On 11/20/18 10:18 AM, Peter Zijlstra wrote: >>> Can't we make this test in text_poke() directly, please? >> >> He does that in 9/10 iirc. > > No, in 9/10 he does that change locally for the jump_label, but there is > absolutely no reason not to do that test in text_poke() proper, and > simply use text_poke() everywhere in the kernel. The decision in __jump_label_transform() is between text_poke_early() and text_poke_bp() ( and not text_poke() ). Moveover, text_poke_early() is also used when a module is loaded - a decision which based on the “init" argument. For these two reasons, I don’t see how the change you proposed can be performed.
diff --git a/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c index aac0c1f7e354..ed5fe274a7d8 100644 --- a/arch/x86/kernel/jump_label.c +++ b/arch/x86/kernel/jump_label.c @@ -52,7 +52,12 @@ static void __ref __jump_label_transform(struct jump_entry *entry, jmp.offset = jump_entry_target(entry) - (jump_entry_code(entry) + JUMP_LABEL_NOP_SIZE); - if (early_boot_irqs_disabled) + /* + * As long as we're UP and not yet marked RO, we can use + * text_poke_early; SYSTEM_BOOTING guarantees both, as we switch to + * SYSTEM_SCHEDULING before going either. + */ + if (system_state == SYSTEM_BOOTING) poker = text_poke_early; if (type == JUMP_LABEL_JMP) {
There is no apparent reason not to use text_poke_early() while we are during early-init and we do not patch code that might be on the stack (i.e., we'll return to the middle of the patched code). This appears to be the case of jump-labels, so do so. This is required for the next patches that would set a temporary mm for patching, which is initialized after some static-keys are enabled/disabled. Cc: Andy Lutomirski <luto@kernel.org> Cc: Kees Cook <keescook@chromium.org> Cc: Dave Hansen <dave.hansen@intel.com> Cc: Masami Hiramatsu <mhiramat@kernel.org> Co-Developed-by: Peter Zijlstra <peterz@infradead.org> Signed-off-by: Nadav Amit <namit@vmware.com> --- arch/x86/kernel/jump_label.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)