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

Message

Jiaxun Yang Feb. 2, 2024, 12:30 p.m. UTC

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/[e-mail]

To: Oleg Nesterov <[e-mail]>

To: Thomas Bogendoerfer <[e-mail]>

To: Andrew Morton <[e-mail]>
To: Ben Hutchings <[e-mail]>

Cc:  <[e-mail]>
Cc:  <[e-mail]>

Cc:  <[e-mail]>

Cc:  <[e-mail]>

Signed-off-by: Jiaxun Yang <[e-mail]>
---
Changes in v2:
- Reduce diffstat by implemente fallback macro in linux/ptrace.h (linus)
- Link to v1: https://lore.kernel.org/r/[e-mail]

---
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,

Comments

Linus Torvalds Feb. 2, 2024, 6:39 p.m. UTC | #1

On Fri, 2 Feb 2024 at 04:30, Jiaxun Yang <[e-mail]> 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

Jiaxun Yang Feb. 2, 2024, 9:31 p.m. UTC | #2

在 2024/2/2 18:39, Linus Torvalds 写道:
> On Fri, 2 Feb 2024 at 04:30, Jiaxun Yang <[e-mail]> 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 Feb. 3, 2024, 1:56 p.m. UTC | #3

在 2024/2/2 18:39, Linus Torvalds 写道:
> On Fri, 2 Feb 2024 at 04:30, Jiaxun Yang <[e-mail]> 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

Linus Torvalds Feb. 4, 2024, 7:41 a.m. UTC | #4

On Sat, 3 Feb 2024 at 13:56, Jiaxun Yang <[e-mail]> 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

Jiaxun Yang Feb. 4, 2024, 11:03 a.m. UTC | #5

在 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

Linus Torvalds Feb. 4, 2024, 11:45 a.m. UTC | #6

On Sun, 4 Feb 2024 at 11:03, Jiaxun Yang <[e-mail]> 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

Jiaxun Yang Feb. 8, 2024, 9:20 a.m. UTC | #7

在 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/[e-mail]
>
> To: Oleg Nesterov <[e-mail]>
>
> To: Thomas Bogendoerfer <[e-mail]>
>
> To: Andrew Morton <[e-mail]>
> To: Ben Hutchings <[e-mail]>
>
> Cc:  <[e-mail]>
> Cc:  <[e-mail]>
>
> Cc:  <[e-mail]>
>
> Cc:  <[e-mail]>
>
> Signed-off-by: Jiaxun Yang <[e-mail]>
> ---
> Changes in v2:
> - Reduce diffstat by implemente fallback macro in linux/ptrace.h (linus)
> - Link to v1: https://lore.kernel.org/r/[e-mail]
>
> ---
> 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,

Xi Ruoyao Feb. 8, 2024, 9:23 a.m. UTC | #8

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: [e-mail] Greg will pick
it up once it's in Linus tree.

Jiaxun Yang Feb. 8, 2024, 9:31 a.m. UTC | #9

在 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: [e-mail] 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
>

Thomas Bogendoerfer Feb. 12, 2024, 10:10 p.m. UTC | #10

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/[e-mail]
> 
> To: Oleg Nesterov <[e-mail]>
> 
> To: Thomas Bogendoerfer <[e-mail]>
> 
> To: Andrew Morton <[e-mail]>
> To: Ben Hutchings <[e-mail]>
> 
> Cc:  <[e-mail]>
> Cc:  <[e-mail]>
> 
> Cc:  <[e-mail]>
> 
> Cc:  <[e-mail]>
> 
> Signed-off-by: Jiaxun Yang <[e-mail]>
> ---
> Changes in v2:
> - Reduce diffstat by implemente fallback macro in linux/ptrace.h (linus)
> - Link to v1: https://lore.kernel.org/r/[e-mail]
> 
> ---
> 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.