1. Patchew
  2. linux
  3. View series

[PATCH] x86/static_call: Fix __static_call_fixup()

Peter Zijlstra posted 1 patch 2 years, 4 months ago
There is a newer version of this series
arch/x86/kernel/static_call.c | 10 ++++++++++
1 file changed, 10 insertions(+)
[PATCH] x86/static_call: Fix __static_call_fixup()
Posted by Peter Zijlstra 2 years, 4 months ago 

Christian reported spurious module crashes after some of Song's module
memory layout patches.

Turns out that if the very last instruction on the very last page of the
module is a 'JMP __x86_return_thunk' then __static_call_fixup() will
trip a fault and dies.

And while the module rework made this slightly more likely to happen,
it's always been possible.

Fixes: ee88d363d156 ("x86,static_call: Use alternative RET encoding")
Reported-by: Christian Bricart <christian@bricart.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/static_call.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/x86/kernel/static_call.c b/arch/x86/kernel/static_call.c
index b70670a98597..2e67512d7104 100644
--- a/arch/x86/kernel/static_call.c
+++ b/arch/x86/kernel/static_call.c
@@ -186,6 +186,16 @@ EXPORT_SYMBOL_GPL(arch_static_call_transform);
  */
 bool __static_call_fixup(void *tramp, u8 op, void *dest)
 {
+	/*
+	 * Not all .return_sites are a static_call trampoline (most are not).
+	 * Check if the next 3 bytes are still kernel text, if not, then this
+	 * definitely is not a trampoline and we need not worry further.
+	 *
+	 * This avoids the memcmp() below tripping over pagefaults etc..
+	 */
+	if (!kernel_text_address(tramp+7))
+		return false;
+
 	if (memcmp(tramp+5, tramp_ud, 3)) {
 		/* Not a trampoline site, not our problem. */
 		return false;
Re: [PATCH] x86/static_call: Fix __static_call_fixup()
Posted by Steven Rostedt 2 years, 4 months ago 
On Wed, 16 Aug 2023 01:08:09 +0200
Peter Zijlstra <peterz@infradead.org> wrote:


> diff --git a/arch/x86/kernel/static_call.c b/arch/x86/kernel/static_call.c
> index b70670a98597..2e67512d7104 100644
> --- a/arch/x86/kernel/static_call.c
> +++ b/arch/x86/kernel/static_call.c
> @@ -186,6 +186,16 @@ EXPORT_SYMBOL_GPL(arch_static_call_transform);
>   */
>  bool __static_call_fixup(void *tramp, u8 op, void *dest)
>  {
> +	/*
> +	 * Not all .return_sites are a static_call trampoline (most are not).
> +	 * Check if the next 3 bytes are still kernel text, if not, then this
> +	 * definitely is not a trampoline and we need not worry further.
> +	 *
> +	 * This avoids the memcmp() below tripping over pagefaults etc..
> +	 */
> +	if (!kernel_text_address(tramp+7))

The comment says "next 3 bytes" and the test is "tramp+7". Why the magic 7 number?

If the tramp is 5 bytes, shouldn't it be +8?

-- Steve


> +		return false;
> +
>  	if (memcmp(tramp+5, tramp_ud, 3)) {
>  		/* Not a trampoline site, not our problem. */
>  		return false;
Re: [PATCH] x86/static_call: Fix __static_call_fixup()
Posted by Peter Zijlstra 2 years, 4 months ago 
On Tue, Aug 15, 2023 at 08:41:12PM -0400, Steven Rostedt wrote:
> On Wed, 16 Aug 2023 01:08:09 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> 
> > diff --git a/arch/x86/kernel/static_call.c b/arch/x86/kernel/static_call.c
> > index b70670a98597..2e67512d7104 100644
> > --- a/arch/x86/kernel/static_call.c
> > +++ b/arch/x86/kernel/static_call.c
> > @@ -186,6 +186,16 @@ EXPORT_SYMBOL_GPL(arch_static_call_transform);
> >   */
> >  bool __static_call_fixup(void *tramp, u8 op, void *dest)
> >  {
> > +	/*
> > +	 * Not all .return_sites are a static_call trampoline (most are not).
> > +	 * Check if the next 3 bytes are still kernel text, if not, then this
> > +	 * definitely is not a trampoline and we need not worry further.
> > +	 *
> > +	 * This avoids the memcmp() below tripping over pagefaults etc..
> > +	 */
> > +	if (!kernel_text_address(tramp+7))
> 
> The comment says "next 3 bytes" and the test is "tramp+7". Why the magic 7 number?
> 
> If the tramp is 5 bytes, shouldn't it be +8?

0 based, 7 is the last of the 8 bytes. +8 would be one beyond.
Re: [PATCH] x86/static_call: Fix __static_call_fixup()
Posted by Josh Poimboeuf 2 years, 4 months ago 
On Wed, Aug 16, 2023 at 01:08:09AM +0200, Peter Zijlstra wrote:
>  bool __static_call_fixup(void *tramp, u8 op, void *dest)
>  {
> +	/*
> +	 * Not all .return_sites are a static_call trampoline (most are not).
> +	 * Check if the next 3 bytes are still kernel text, if not, then this

s/3/7 ?

> +	 * definitely is not a trampoline and we need not worry further.
> +	 *
> +	 * This avoids the memcmp() below tripping over pagefaults etc..
> +	 */
> +	if (!kernel_text_address(tramp+7))
> +		return false;
> +
>  	if (memcmp(tramp+5, tramp_ud, 3)) {
>  		/* Not a trampoline site, not our problem. */
>  		return false;

kernel_text_address() can be quite heavyweight to call in a loop during
module loading.  Maybe that doesn't matter much.  But it would be a lot
faster to only call kernel_text_address() if tramp+7 is on the next
page.

-- 
Josh
Re: [PATCH] x86/static_call: Fix __static_call_fixup()
Posted by Peter Zijlstra 2 years, 4 months ago 
On Tue, Aug 15, 2023 at 05:10:32PM -0700, Josh Poimboeuf wrote:
> On Wed, Aug 16, 2023 at 01:08:09AM +0200, Peter Zijlstra wrote:
> >  bool __static_call_fixup(void *tramp, u8 op, void *dest)
> >  {
> > +	/*
> > +	 * Not all .return_sites are a static_call trampoline (most are not).
> > +	 * Check if the next 3 bytes are still kernel text, if not, then this
> 
> s/3/7 ?

Right, so what I meant was the 3 bytes after the return, which is 5+3,
but yeah, that can be said better.

> 
> > +	 * definitely is not a trampoline and we need not worry further.
> > +	 *
> > +	 * This avoids the memcmp() below tripping over pagefaults etc..
> > +	 */
> > +	if (!kernel_text_address(tramp+7))
> > +		return false;
> > +
> >  	if (memcmp(tramp+5, tramp_ud, 3)) {
> >  		/* Not a trampoline site, not our problem. */
> >  		return false;
> 
> kernel_text_address() can be quite heavyweight to call in a loop during
> module loading.  Maybe that doesn't matter much.  But it would be a lot
> faster to only call kernel_text_address() if tramp+7 is on the next
> page.

Oh, right, in those few configs where it doesn't use the tree. Sure can
do.
Re: [PATCH] x86/static_call: Fix __static_call_fixup()
Posted by Peter Zijlstra 2 years, 4 months ago 
On Wed, Aug 16, 2023 at 01:08:10AM +0200, Peter Zijlstra wrote:
> 
> Christian reported spurious module crashes after some of Song's module

To clarify: module-load.

Obviously I shouldn't be writing Changelogs after 1am :-)

> memory layout patches.
> 
> Turns out that if the very last instruction on the very last page of the
> module is a 'JMP __x86_return_thunk' then __static_call_fixup() will
> trip a fault and dies.
> 
> And while the module rework made this slightly more likely to happen,
> it's always been possible.
> 
> Fixes: ee88d363d156 ("x86,static_call: Use alternative RET encoding")
> Reported-by: Christian Bricart <christian@bricart.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/kernel/static_call.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/x86/kernel/static_call.c b/arch/x86/kernel/static_call.c
> index b70670a98597..2e67512d7104 100644
> --- a/arch/x86/kernel/static_call.c
> +++ b/arch/x86/kernel/static_call.c
> @@ -186,6 +186,16 @@ EXPORT_SYMBOL_GPL(arch_static_call_transform);
>   */
>  bool __static_call_fixup(void *tramp, u8 op, void *dest)
>  {
> +	/*
> +	 * Not all .return_sites are a static_call trampoline (most are not).
> +	 * Check if the next 3 bytes are still kernel text, if not, then this
> +	 * definitely is not a trampoline and we need not worry further.
> +	 *
> +	 * This avoids the memcmp() below tripping over pagefaults etc..
> +	 */
> +	if (!kernel_text_address(tramp+7))
> +		return false;
> +
>  	if (memcmp(tramp+5, tramp_ud, 3)) {
>  		/* Not a trampoline site, not our problem. */
>  		return false;
[PATCH v2] x86/static_call: Fix __static_call_fixup()
Posted by Peter Zijlstra 2 years, 4 months ago 

Christian reported spurious module load crashes after some of Song's
module memory layout patches.

Turns out that if the very last instruction on the very last page of the
module is a 'JMP __x86_return_thunk' then __static_call_fixup() will
trip a fault and die.

And while the module rework made this slightly more likely to happen,
it's always been possible.

Fixes: ee88d363d156 ("x86,static_call: Use alternative RET encoding")
Reported-by: Christian Bricart <christian@bricart.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/static_call.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

--- a/arch/x86/kernel/static_call.c
+++ b/arch/x86/kernel/static_call.c
@@ -186,6 +186,19 @@ EXPORT_SYMBOL_GPL(arch_static_call_trans
  */
 bool __static_call_fixup(void *tramp, u8 op, void *dest)
 {
+	unsigned long addr = (unsigned long)tramp;
+	/*
+	 * Not all .return_sites are a static_call trampoline (most are not).
+	 * Check if the 3 bytes after the return are still kernel text, if not,
+	 * then this definitely is not a trampoline and we need not worry
+	 * further.
+	 *
+	 * This avoids the memcmp() below tripping over pagefaults etc..
+	 */
+	if (((addr >> PAGE_SHIFT) != ((addr + 7) >> PAGE_SHIFT)) &&
+	    !kernel_text_address(addr + 7))
+		return false;
+
 	if (memcmp(tramp+5, tramp_ud, 3)) {
 		/* Not a trampoline site, not our problem. */
 		return false;
Re: [PATCH v2] x86/static_call: Fix __static_call_fixup()
Posted by Josh Poimboeuf 2 years, 4 months ago 
On Wed, Aug 16, 2023 at 12:44:19PM +0200, Peter Zijlstra wrote:
> 
> Christian reported spurious module load crashes after some of Song's
> module memory layout patches.
> 
> Turns out that if the very last instruction on the very last page of the
> module is a 'JMP __x86_return_thunk' then __static_call_fixup() will
> trip a fault and die.
> 
> And while the module rework made this slightly more likely to happen,
> it's always been possible.
> 
> Fixes: ee88d363d156 ("x86,static_call: Use alternative RET encoding")
> Reported-by: Christian Bricart <christian@bricart.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Acked-by: Josh Poimboeuf <jpoimboe@kernel.org>

-- 
Josh
[tip: x86/urgent] x86/static_call: Fix __static_call_fixup()
Posted by tip-bot2 for Peter Zijlstra 2 years, 4 months ago 
The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     54097309620ef0dc2d7083783dc521c6a5fef957
Gitweb:        https://git.kernel.org/tip/54097309620ef0dc2d7083783dc521c6a5fef957
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Wed, 16 Aug 2023 12:44:19 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Thu, 17 Aug 2023 13:24:09 +02:00

x86/static_call: Fix __static_call_fixup()

Christian reported spurious module load crashes after some of Song's
module memory layout patches.

Turns out that if the very last instruction on the very last page of the
module is a 'JMP __x86_return_thunk' then __static_call_fixup() will
trip a fault and die.

And while the module rework made this slightly more likely to happen,
it's always been possible.

Fixes: ee88d363d156 ("x86,static_call: Use alternative RET encoding")
Reported-by: Christian Bricart <christian@bricart.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Josh Poimboeuf <jpoimboe@kernel.org>
Link: https://lkml.kernel.org/r/20230816104419.GA982867@hirez.programming.kicks-ass.net
---
 arch/x86/kernel/static_call.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/x86/kernel/static_call.c b/arch/x86/kernel/static_call.c
index b70670a..77a9316 100644
--- a/arch/x86/kernel/static_call.c
+++ b/arch/x86/kernel/static_call.c
@@ -186,6 +186,19 @@ EXPORT_SYMBOL_GPL(arch_static_call_transform);
  */
 bool __static_call_fixup(void *tramp, u8 op, void *dest)
 {
+	unsigned long addr = (unsigned long)tramp;
+	/*
+	 * Not all .return_sites are a static_call trampoline (most are not).
+	 * Check if the 3 bytes after the return are still kernel text, if not,
+	 * then this definitely is not a trampoline and we need not worry
+	 * further.
+	 *
+	 * This avoids the memcmp() below tripping over pagefaults etc..
+	 */
+	if (((addr >> PAGE_SHIFT) != ((addr + 7) >> PAGE_SHIFT)) &&
+	    !kernel_text_address(addr + 7))
+		return false;
+
 	if (memcmp(tramp+5, tramp_ud, 3)) {
 		/* Not a trampoline site, not our problem. */
 		return false;

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.