[PATCH v2 0/3] Handle delay slot for extable lookup

[PATCH v2 0/3] Handle delay slot for extable lookup

[Jiaxun Yang]

Hi all,

This series fixed extable handling for architecture delay slot (MIPS).

Please see previous discussions at [1].

There are some other places in kernel not handling delay slots properly,
such as uprobe and kgdb, I'll sort them later.

Thanks!

[1]: https://lore.kernel.org/lkml/75e9fd7b08562ad9b456a5bdaacb7cc220311cc9.camel@xry111.site

To: Oleg Nesterov <oleg@redhat.com>

To: Thomas Bogendoerfer <tsbogend@alpha.franken.de>

To: Andrew Morton <akpm@linux-foundation.org>
To: Ben Hutchings <ben@decadent.org.uk>

Cc:  <linux-arch@vger.kernel.org>
Cc:  <linux-kernel@vger.kernel.org>

Cc:  <linux-mips@vger.kernel.org>

Cc:  <linux-mm@kvack.org>

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
Changes in v2:
- Reduce diffstat by implemente fallback macro in linux/ptrace.h (linus)
- Link to v1: https://lore.kernel.org/r/20240201-exception_ip-v1-0-aa26ab3ee0b5@flygoat.com

---
Jiaxun Yang (3):
      ptrace: Introduce exception_ip arch hook
      MIPS: Clear Cause.BD in instruction_pointer_set
      mm/memory: Use exception ip to search exception tables

 arch/mips/include/asm/ptrace.h | 3 +++
 arch/mips/kernel/ptrace.c      | 7 +++++++
 include/linux/ptrace.h         | 4 ++++
 mm/memory.c                    | 4 ++--
 4 files changed, 16 insertions(+), 2 deletions(-)
---
base-commit: 06f658aadff0e483ee4f807b0b46c9e5cba62bfa
change-id: 20240131-exception_ip-194e4ad0e6ca

Best regards,
-- 
Jiaxun Yang <jiaxun.yang@flygoat.com>

[PATCH v2 1/3] ptrace: Introduce exception_ip arch hook

[Jiaxun Yang]

On architectures with delay slot, architecture level instruction
pointer (or program counter) in pt_regs may differ from where
exception was triggered.

Introduce exception_ip hook to invoke architecture code and determine
actual instruction pointer to the exception.

Link: https://lore.kernel.org/lkml/00d1b813-c55f-4365-8d81-d70258e10b16@app.fastmail.com/
Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 arch/mips/include/asm/ptrace.h | 2 ++
 arch/mips/kernel/ptrace.c      | 7 +++++++
 include/linux/ptrace.h         | 4 ++++
 3 files changed, 13 insertions(+)

diff --git a/arch/mips/include/asm/ptrace.h b/arch/mips/include/asm/ptrace.h
index daf3cf244ea9..701a233583c2 100644
--- a/arch/mips/include/asm/ptrace.h
+++ b/arch/mips/include/asm/ptrace.h
@@ -154,6 +154,8 @@ static inline long regs_return_value(struct pt_regs *regs)
 }
 
 #define instruction_pointer(regs) ((regs)->cp0_epc)
+extern unsigned long exception_ip(struct pt_regs *regs);
+#define exception_ip(regs) exception_ip(regs)
 #define profile_pc(regs) instruction_pointer(regs)
 
 extern asmlinkage long syscall_trace_enter(struct pt_regs *regs, long syscall);
diff --git a/arch/mips/kernel/ptrace.c b/arch/mips/kernel/ptrace.c
index d9df543f7e2c..59288c13b581 100644
--- a/arch/mips/kernel/ptrace.c
+++ b/arch/mips/kernel/ptrace.c
@@ -31,6 +31,7 @@
 #include <linux/seccomp.h>
 #include <linux/ftrace.h>
 
+#include <asm/branch.h>
 #include <asm/byteorder.h>
 #include <asm/cpu.h>
 #include <asm/cpu-info.h>
@@ -48,6 +49,12 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/syscalls.h>
 
+unsigned long exception_ip(struct pt_regs *regs)
+{
+	return exception_epc(regs);
+}
+EXPORT_SYMBOL(exception_ip);
+
 /*
  * Called by kernel/ptrace.c when detaching..
  *
diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index eaaef3ffec22..90507d4afcd6 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -393,6 +393,10 @@ static inline void user_single_step_report(struct pt_regs *regs)
 #define current_user_stack_pointer() user_stack_pointer(current_pt_regs())
 #endif
 
+#ifndef exception_ip
+#define exception_ip(x) instruction_pointer(x)
+#endif
+
 extern int task_current_syscall(struct task_struct *target, struct syscall_info *info);
 
 extern void sigaction_compat_abi(struct k_sigaction *act, struct k_sigaction *oact);

-- 
2.43.0

[PATCH v2 2/3] MIPS: Clear Cause.BD in instruction_pointer_set

[Jiaxun Yang]

Clear Cause.BD after we use instruction_pointer_set to override
EPC.

This can prevent exception_epc check against instruction code at
new return address.
It won't be considered as "in delay slot" after epc being overridden
anyway.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 arch/mips/include/asm/ptrace.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/mips/include/asm/ptrace.h b/arch/mips/include/asm/ptrace.h
index 701a233583c2..d14d0e37ad02 100644
--- a/arch/mips/include/asm/ptrace.h
+++ b/arch/mips/include/asm/ptrace.h
@@ -60,6 +60,7 @@ static inline void instruction_pointer_set(struct pt_regs *regs,
                                            unsigned long val)
 {
 	regs->cp0_epc = val;
+	regs->cp0_cause &= ~CAUSEF_BD;
 }
 
 /* Query offset/name of register from its name/offset */

-- 
2.43.0

[PATCH v2 3/3] mm/memory: Use exception ip to search exception tables

[Jiaxun Yang]

On architectures with delay slot, instruction_pointer() may differ
from where exception was triggered.

Use exception_ip we just introduced to search exception tables to
get rid of the problem.

Fixes: 4bce37a68ff8 ("mips/mm: Convert to using lock_mm_and_find_vma()")
Reported-by: Xi Ruoyao <xry111@xry111.site>
Link: https://lore.kernel.org/r/75e9fd7b08562ad9b456a5bdaacb7cc220311cc9.camel@xry111.site/
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 mm/memory.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 8d14ba440929..49433612444a 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5481,7 +5481,7 @@ static inline bool get_mmap_lock_carefully(struct mm_struct *mm, struct pt_regs
 		return true;
 
 	if (regs && !user_mode(regs)) {
-		unsigned long ip = instruction_pointer(regs);
+		unsigned long ip = exception_ip(regs);
 		if (!search_exception_tables(ip))
 			return false;
 	}
@@ -5506,7 +5506,7 @@ static inline bool upgrade_mmap_lock_carefully(struct mm_struct *mm, struct pt_r
 {
 	mmap_read_unlock(mm);
 	if (regs && !user_mode(regs)) {
-		unsigned long ip = instruction_pointer(regs);
+		unsigned long ip = exception_ip(regs);
 		if (!search_exception_tables(ip))
 			return false;
 	}

-- 
2.43.0

Re: [PATCH v2 0/3] Handle delay slot for extable lookup

[Linus Torvalds]

On Fri, 2 Feb 2024 at 04:30, Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
>
>       ptrace: Introduce exception_ip arch hook
>       MIPS: Clear Cause.BD in instruction_pointer_set
>       mm/memory: Use exception ip to search exception tables

Just to clarify: does that second patch fix the problem that
__isa_exception_epc() does a __get_user()?

Because that mm/memory.c use of "exception_ip()" most definitely
cannot take a page fault.

So if MIPS cannot do that whole exception IP thing without potentially
touching user space, I do worry that we might just have to add a

   #ifndef __MIPS__

around this all.

Possibly somehow limited to just the microMIPS/MIPS16e case in Kconfig instead?

            Linus

Re: [PATCH v2 0/3] Handle delay slot for extable lookup

[Jiaxun Yang]

在 2024/2/2 18:39, Linus Torvalds 写道:
> On Fri, 2 Feb 2024 at 04:30, Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
>>        ptrace: Introduce exception_ip arch hook
>>        MIPS: Clear Cause.BD in instruction_pointer_set
>>        mm/memory: Use exception ip to search exception tables
> Just to clarify: does that second patch fix the problem that
> __isa_exception_epc() does a __get_user()?

No it only covers an obvious case when I was playing around exception_ip 
with kgdb.
There are still potential cases __isa_exception_epc may touch user space.

> Because that mm/memory.c use of "exception_ip()" most definitely
> cannot take a page fault.
>
> So if MIPS cannot do that whole exception IP thing without potentially
> touching user space, I do worry that we might just have to add a
>
>     #ifndef __MIPS__
>
> around this all.

It is possible to perform exception_ip() without touching user space by 
saving "BadInstr" in pt_regs
at exception entries. For newer hardware it's as simple as saving an 
extra CP0 register, on older hardware
we may have to read it from user space.

+ Thomas (MIPS maintainer), what's your opinion?

Thanks
>
> Possibly somehow limited to just the microMIPS/MIPS16e case in Kconfig instead?
>
>              Linus

-- 
---
Jiaxun Yang

Re: [PATCH v2 0/3] Handle delay slot for extable lookup

[Jiaxun Yang]

在 2024/2/2 18:39, Linus Torvalds 写道:
> On Fri, 2 Feb 2024 at 04:30, Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
>>        ptrace: Introduce exception_ip arch hook
>>        MIPS: Clear Cause.BD in instruction_pointer_set
>>        mm/memory: Use exception ip to search exception tables
> Just to clarify: does that second patch fix the problem that
> __isa_exception_epc() does a __get_user()?
>
> Because that mm/memory.c use of "exception_ip()" most definitely
> cannot take a page fault.

I reconsidered this issue today and I believe that exception_ip() usage 
here won't trigger
page_fault anyway.

Given that exception_ip is guarded by !user_mode(regs), EPC must points 
to a kernel
text address. There is no way accessing kernel text will generate such 
exception..

Thanks
>
> So if MIPS cannot do that whole exception IP thing without potentially
> touching user space, I do worry that we might just have to add a
>
>     #ifndef __MIPS__
>
> around this all.
>
> Possibly somehow limited to just the microMIPS/MIPS16e case in Kconfig instead?
>
>              Linus

-- 
---
Jiaxun Yang

Re: [PATCH v2 0/3] Handle delay slot for extable lookup

[Linus Torvalds]

On Sat, 3 Feb 2024 at 13:56, Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
>
> Given that exception_ip is guarded by !user_mode(regs), EPC must points
> to a kernel text address. There is no way accessing kernel text will generate such
> exception..

Sadly, that's not actually likely true.

The thing is, the only reason for the code in
get_mmap_lock_carefully() is for kernel bugs. IOW, some kernel bug
with a wild pointer, and we do not want to deadlock on the mm
semaphore, we want to get back to the fault handler and it should
report an oops.

... and one of the "wild pointers" might in fact be the instruction
pointer, in case we've branched through a NULL function pointer or
similar. IOW, the exception *source* might be the instruction pointer
itself.

So I realy think that MIPS needs to have some kind of "safe kernel
exception pointer" helper. One that is guaranteed not to fault when
evaluating the exception pointer.

Assuming the kernel itself is never built with MIPS16e instructions,
maybe that's a safe assumption thanks to the "get_isa16_mode()" check
of EPC. But all of this makes me nervous.

              Linus

Re: [PATCH v2 0/3] Handle delay slot for extable lookup

[Jiaxun Yang]

在 2024/2/4 07:41, Linus Torvalds 写道:
[...]
> The thing is, the only reason for the code in
> get_mmap_lock_carefully() is for kernel bugs. IOW, some kernel bug
> with a wild pointer, and we do not want to deadlock on the mm
> semaphore, we want to get back to the fault handler and it should
> report an oops.
>
> ... and one of the "wild pointers" might in fact be the instruction
> pointer, in case we've branched through a NULL function pointer or
> similar. IOW, the exception *source* might be the instruction pointer
> itself.

Well this is the tricky part of my assumption.
In `exception_epc()` `__isa_exception_epc()` stuff is only called if we 
are in delay slot.
It is impossible for a invalid instruction_pointer to be considered as 
"in delay slot"
by hardware.

I'd agree that sounds fragile though.

Thanks
>
> So I realy think that MIPS needs to have some kind of "safe kernel
> exception pointer" helper. One that is guaranteed not to fault when
> evaluating the exception pointer.
>
> Assuming the kernel itself is never built with MIPS16e instructions,
> maybe that's a safe assumption thanks to the "get_isa16_mode()" check
> of EPC. But all of this makes me nervous.
>
>                Linus

-- 
---
Jiaxun Yang

Re: [PATCH v2 0/3] Handle delay slot for extable lookup

[Linus Torvalds]

On Sun, 4 Feb 2024 at 11:03, Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
>
> Well this is the tricky part of my assumption.
> In `exception_epc()` `__isa_exception_epc()` stuff is only called if we
> are in delay slot.
> It is impossible for a invalid instruction_pointer to be considered as
> "in delay slot" by hardware.

Ok, I guess I'm convinced this is all safe. Not great, and not exactly
giving me the warm fuzzies, but not buggy.

         Linus

Re: [PATCH v2 0/3] Handle delay slot for extable lookup

[Jiaxun Yang]

在 2024/2/2 12:30, Jiaxun Yang 写道:
> Hi all,
>
> This series fixed extable handling for architecture delay slot (MIPS).
>
> Please see previous discussions at [1].
>
> There are some other places in kernel not handling delay slots properly,
> such as uprobe and kgdb, I'll sort them later.

A gentle ping :-)

This series fixes a regression, perhaps it should go through fixes tree.

Thanks
- Jiaxun

>
> Thanks!
>
> [1]: https://lore.kernel.org/lkml/75e9fd7b08562ad9b456a5bdaacb7cc220311cc9.camel@xry111.site
>
> To: Oleg Nesterov <oleg@redhat.com>
>
> To: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
>
> To: Andrew Morton <akpm@linux-foundation.org>
> To: Ben Hutchings <ben@decadent.org.uk>
>
> Cc:  <linux-arch@vger.kernel.org>
> Cc:  <linux-kernel@vger.kernel.org>
>
> Cc:  <linux-mips@vger.kernel.org>
>
> Cc:  <linux-mm@kvack.org>
>
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
> Changes in v2:
> - Reduce diffstat by implemente fallback macro in linux/ptrace.h (linus)
> - Link to v1: https://lore.kernel.org/r/20240201-exception_ip-v1-0-aa26ab3ee0b5@flygoat.com
>
> ---
> Jiaxun Yang (3):
>        ptrace: Introduce exception_ip arch hook
>        MIPS: Clear Cause.BD in instruction_pointer_set
>        mm/memory: Use exception ip to search exception tables
>
>   arch/mips/include/asm/ptrace.h | 3 +++
>   arch/mips/kernel/ptrace.c      | 7 +++++++
>   include/linux/ptrace.h         | 4 ++++
>   mm/memory.c                    | 4 ++--
>   4 files changed, 16 insertions(+), 2 deletions(-)
> ---
> base-commit: 06f658aadff0e483ee4f807b0b46c9e5cba62bfa
> change-id: 20240131-exception_ip-194e4ad0e6ca
>
> Best regards,

-- 
---
Jiaxun Yang

Re: [PATCH v2 0/3] Handle delay slot for extable lookup

[Xi Ruoyao]

On Thu, 2024-02-08 at 09:20 +0000, Jiaxun Yang wrote:
> 
> 
> 在 2024/2/2 12:30, Jiaxun Yang 写道:
> > Hi all,
> > 
> > This series fixed extable handling for architecture delay slot (MIPS).
> > 
> > Please see previous discussions at [1].
> > 
> > There are some other places in kernel not handling delay slots properly,
> > such as uprobe and kgdb, I'll sort them later.
> 
> A gentle ping :-)
> 
> This series fixes a regression, perhaps it should go through fixes tree.

If you have Fixes: {sha} and Cc: stable@vger.kernel.org Greg will pick
it up once it's in Linus tree.

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH v2 0/3] Handle delay slot for extable lookup

[Jiaxun Yang]

在 2024/2/8 09:23, Xi Ruoyao 写道:
> On Thu, 2024-02-08 at 09:20 +0000, Jiaxun Yang wrote:
>>
>> 在 2024/2/2 12:30, Jiaxun Yang 写道:
>>> Hi all,
>>>
>>> This series fixed extable handling for architecture delay slot (MIPS).
>>>
>>> Please see previous discussions at [1].
>>>
>>> There are some other places in kernel not handling delay slots properly,
>>> such as uprobe and kgdb, I'll sort them later.
>> A gentle ping :-)
>>
>> This series fixes a regression, perhaps it should go through fixes tree.
> If you have Fixes: {sha} and Cc: stable@vger.kernel.org Greg will pick
> it up once it's in Linus tree.

In this case I'd like to backport it manually, as there is only one 
patch in this series
actually fixing the problem.

Thanks
- Jiaxun
>

-- 
---
Jiaxun Yang

Re: [PATCH v2 0/3] Handle delay slot for extable lookup

[Thomas Bogendoerfer]

On Fri, Feb 02, 2024 at 12:30:25PM +0000, Jiaxun Yang wrote:
> Hi all,
> 
> This series fixed extable handling for architecture delay slot (MIPS).
> 
> Please see previous discussions at [1].
> 
> There are some other places in kernel not handling delay slots properly,
> such as uprobe and kgdb, I'll sort them later.
> 
> Thanks!
> 
> [1]: https://lore.kernel.org/lkml/75e9fd7b08562ad9b456a5bdaacb7cc220311cc9.camel@xry111.site
> 
> To: Oleg Nesterov <oleg@redhat.com>
> 
> To: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> 
> To: Andrew Morton <akpm@linux-foundation.org>
> To: Ben Hutchings <ben@decadent.org.uk>
> 
> Cc:  <linux-arch@vger.kernel.org>
> Cc:  <linux-kernel@vger.kernel.org>
> 
> Cc:  <linux-mips@vger.kernel.org>
> 
> Cc:  <linux-mm@kvack.org>
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
> Changes in v2:
> - Reduce diffstat by implemente fallback macro in linux/ptrace.h (linus)
> - Link to v1: https://lore.kernel.org/r/20240201-exception_ip-v1-0-aa26ab3ee0b5@flygoat.com
> 
> ---
> Jiaxun Yang (3):
>       ptrace: Introduce exception_ip arch hook
>       MIPS: Clear Cause.BD in instruction_pointer_set
>       mm/memory: Use exception ip to search exception tables
> 
>  arch/mips/include/asm/ptrace.h | 3 +++
>  arch/mips/kernel/ptrace.c      | 7 +++++++
>  include/linux/ptrace.h         | 4 ++++
>  mm/memory.c                    | 4 ++--
>  4 files changed, 16 insertions(+), 2 deletions(-)

series applied to mips-fixes.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]