x86/ftrace: Cure boot time W+X mapping

[PATCH 4/5] x86/ftrace: Remove SYSTEM_BOOTING exceptions

Posted by Peter Zijlstra 3 years, 5 months ago

Now that text_poke is available before ftrace, remove the
SYSTEM_BOOTING exceptions.

Specifically, this cures a W+X case during boot.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/alternative.c |   10 ----------
 arch/x86/kernel/ftrace.c      |    3 +--
 2 files changed, 1 insertion(+), 12 deletions(-)

--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -1681,11 +1681,6 @@ void __ref text_poke_queue(void *addr, c
 {
 	struct text_poke_loc *tp;
 
-	if (unlikely(system_state == SYSTEM_BOOTING)) {
-		text_poke_early(addr, opcode, len);
-		return;
-	}
-
 	text_poke_flush(addr);
 
 	tp = &tp_vec[tp_vec_nr++];
@@ -1707,11 +1702,6 @@ void __ref text_poke_bp(void *addr, cons
 {
 	struct text_poke_loc tp;
 
-	if (unlikely(system_state == SYSTEM_BOOTING)) {
-		text_poke_early(addr, opcode, len);
-		return;
-	}
-
 	text_poke_loc_init(&tp, addr, opcode, len, emulate);
 	text_poke_bp_batch(&tp, 1);
 }
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -415,8 +415,7 @@ create_trampoline(struct ftrace_ops *ops
 
 	set_vm_flush_reset_perms(trampoline);
 
-	if (likely(system_state != SYSTEM_BOOTING))
-		set_memory_ro((unsigned long)trampoline, npages);
+	set_memory_ro((unsigned long)trampoline, npages);
 	set_memory_x((unsigned long)trampoline, npages);
 	return (unsigned long)trampoline;
 fail:

Re: [PATCH 4/5] x86/ftrace: Remove SYSTEM_BOOTING exceptions

Posted by Steven Rostedt 3 years, 5 months ago

On Tue, 25 Oct 2022 22:07:00 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> Now that text_poke is available before ftrace, remove the
> SYSTEM_BOOTING exceptions.
> 
> Specifically, this cures a W+X case during boot.

We have W+X all over the place (the entire kernel text). And I don't think
we really want this.

This will slow down boots in general, as it will cause all static_branches
to use this memory page logic. And I don't think we really want to do
that at boot up when we don't need to.

I would change this to:

	if (unlikely(system_state == SYSTEM_BOOTING) &&
	    core_kernel_text((unsigned long)addr)) {

This way we still do memcpy() on all core kernel text which is still
writable. It was the ftrace allocated trampoline that caused issues, not
the locations that were being updated.

-- Steve



> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/kernel/alternative.c |   10 ----------
>  arch/x86/kernel/ftrace.c      |    3 +--
>  2 files changed, 1 insertion(+), 12 deletions(-)
> 
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -1681,11 +1681,6 @@ void __ref text_poke_queue(void *addr, c
>  {
>  	struct text_poke_loc *tp;
>  
> -	if (unlikely(system_state == SYSTEM_BOOTING)) {
> -		text_poke_early(addr, opcode, len);
> -		return;
> -	}
> -
>  	text_poke_flush(addr);
>  
>  	tp = &tp_vec[tp_vec_nr++];
> @@ -1707,11 +1702,6 @@ void __ref text_poke_bp(void *addr, cons
>  {
>  	struct text_poke_loc tp;
>  
> -	if (unlikely(system_state == SYSTEM_BOOTING)) {
> -		text_poke_early(addr, opcode, len);
> -		return;
> -	}
> -
>  	text_poke_loc_init(&tp, addr, opcode, len, emulate);
>  	text_poke_bp_batch(&tp, 1);
>  }
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -415,8 +415,7 @@ create_trampoline(struct ftrace_ops *ops
>  
>  	set_vm_flush_reset_perms(trampoline);
>  
> -	if (likely(system_state != SYSTEM_BOOTING))
> -		set_memory_ro((unsigned long)trampoline, npages);
> +	set_memory_ro((unsigned long)trampoline, npages);
>  	set_memory_x((unsigned long)trampoline, npages);
>  	return (unsigned long)trampoline;
>  fail:
>

Re: [PATCH 4/5] x86/ftrace: Remove SYSTEM_BOOTING exceptions

Posted by Peter Zijlstra 3 years, 5 months ago

On Tue, Oct 25, 2022 at 04:59:56PM -0400, Steven Rostedt wrote:
> On Tue, 25 Oct 2022 22:07:00 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > Now that text_poke is available before ftrace, remove the
> > SYSTEM_BOOTING exceptions.
> > 
> > Specifically, this cures a W+X case during boot.
> 
> We have W+X all over the place (the entire kernel text). And I don't think
> we really want this.
> 
> This will slow down boots in general, as it will cause all static_branches
> to use this memory page logic. And I don't think we really want to do
> that at boot up when we don't need to.

Both static_call and jump_label explicitly call text_poke_early() when
appropriate.

> I would change this to:
> 
> 	if (unlikely(system_state == SYSTEM_BOOTING) &&
> 	    core_kernel_text((unsigned long)addr)) {
> 
> This way we still do memcpy() on all core kernel text which is still
> writable. It was the ftrace allocated trampoline that caused issues, not
> the locations that were being updated.

I would suggest changing ftrace to call text_poke_early() when
appropriate if it matters (it already does a little of that); doing a
boot test with and without my patch 4 on shows no noticable overhead
over being horribly slow either way.

[tip: x86/mm] x86/ftrace: Remove SYSTEM_BOOTING exceptions

Posted by tip-bot2 for Peter Zijlstra 3 years, 3 months ago

The following commit has been merged into the x86/mm branch of tip:

Commit-ID:     eb7d389d5b2b3c453332abc41c3eea73290cc006
Gitweb:        https://git.kernel.org/tip/eb7d389d5b2b3c453332abc41c3eea73290cc006
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Tue, 25 Oct 2022 21:39:47 +02:00
Committer:     Dave Hansen <dave.hansen@linux.intel.com>
CommitterDate: Thu, 15 Dec 2022 10:37:26 -08:00

x86/ftrace: Remove SYSTEM_BOOTING exceptions

Now that text_poke is available before ftrace, remove the
SYSTEM_BOOTING exceptions.

Specifically, this cures a W+X case during boot.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20221025201057.945960823@infradead.org
---
 arch/x86/kernel/alternative.c | 10 ----------
 arch/x86/kernel/ftrace.c      |  3 +--
 2 files changed, 1 insertion(+), 12 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 5cadcea..e240351 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -1681,11 +1681,6 @@ void __ref text_poke_queue(void *addr, const void *opcode, size_t len, const voi
 {
 	struct text_poke_loc *tp;
 
-	if (unlikely(system_state == SYSTEM_BOOTING)) {
-		text_poke_early(addr, opcode, len);
-		return;
-	}
-
 	text_poke_flush(addr);
 
 	tp = &tp_vec[tp_vec_nr++];
@@ -1707,11 +1702,6 @@ void __ref text_poke_bp(void *addr, const void *opcode, size_t len, const void *
 {
 	struct text_poke_loc tp;
 
-	if (unlikely(system_state == SYSTEM_BOOTING)) {
-		text_poke_early(addr, opcode, len);
-		return;
-	}
-
 	text_poke_loc_init(&tp, addr, opcode, len, emulate);
 	text_poke_bp_batch(&tp, 1);
 }
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index bd16500..43628b8 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -415,8 +415,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
 
 	set_vm_flush_reset_perms(trampoline);
 
-	if (likely(system_state != SYSTEM_BOOTING))
-		set_memory_ro((unsigned long)trampoline, npages);
+	set_memory_ro((unsigned long)trampoline, npages);
 	set_memory_x((unsigned long)trampoline, npages);
 	return (unsigned long)trampoline;
 fail:

[tip: x86/mm] x86/ftrace: Remove SYSTEM_BOOTING exceptions

Posted by tip-bot2 for Peter Zijlstra 3 years, 5 months ago

The following commit has been merged into the x86/mm branch of tip:

Commit-ID:     52a56f20bb7c34ed4b48466ad2d443165fad942f
Gitweb:        https://git.kernel.org/tip/52a56f20bb7c34ed4b48466ad2d443165fad942f
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Tue, 25 Oct 2022 21:39:47 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Tue, 01 Nov 2022 13:43:58 +01:00

x86/ftrace: Remove SYSTEM_BOOTING exceptions

Now that text_poke is available before ftrace, remove the
SYSTEM_BOOTING exceptions.

Specifically, this cures a W+X case during boot.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20221025201057.945960823@infradead.org
---
 arch/x86/kernel/alternative.c | 10 ----------
 arch/x86/kernel/ftrace.c      |  3 +--
 2 files changed, 1 insertion(+), 12 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 5cadcea..e240351 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -1681,11 +1681,6 @@ void __ref text_poke_queue(void *addr, const void *opcode, size_t len, const voi
 {
 	struct text_poke_loc *tp;
 
-	if (unlikely(system_state == SYSTEM_BOOTING)) {
-		text_poke_early(addr, opcode, len);
-		return;
-	}
-
 	text_poke_flush(addr);
 
 	tp = &tp_vec[tp_vec_nr++];
@@ -1707,11 +1702,6 @@ void __ref text_poke_bp(void *addr, const void *opcode, size_t len, const void *
 {
 	struct text_poke_loc tp;
 
-	if (unlikely(system_state == SYSTEM_BOOTING)) {
-		text_poke_early(addr, opcode, len);
-		return;
-	}
-
 	text_poke_loc_init(&tp, addr, opcode, len, emulate);
 	text_poke_bp_batch(&tp, 1);
 }
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index bd16500..43628b8 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -415,8 +415,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
 
 	set_vm_flush_reset_perms(trampoline);
 
-	if (likely(system_state != SYSTEM_BOOTING))
-		set_memory_ro((unsigned long)trampoline, npages);
+	set_memory_ro((unsigned long)trampoline, npages);
 	set_memory_x((unsigned long)trampoline, npages);
 	return (unsigned long)trampoline;
 fail: