m68k: Improved switch stack handling

Add 'status' field to thread_info struct to hold syscall trace status info.

Set flag bit in thread_info->status at syscall trace entry, clear flag bit on trace exit.

Set another flag bit on entering syscall where the full stack frame has been saved. These flags can be checked whenever a syscall calls ptrace_stop().

Check flag bits in get_reg()/put_reg() and prevent access to registers that are saved on the switch stack, in case the syscall did not actually save these registers on the switch stack.

Tested on ARAnyM only - boots and survives running strace on a binary, nothing fancy.

CC Eric W. Biederman <[email protected]>

CC Linus Torvalds <[email protected]>

CC Andreas Schwab <[email protected]>

Signed-off-by Michael Schmitz <[email protected]>

Hi Michael,

Quoted 15 lines · Michael Schmitz

Add 'status' field to thread_info struct to hold syscall trace status info.

Set flag bit in thread_info->status at syscall trace entry, clear flag bit on trace exit.

Set another flag bit on entering syscall where the full stack frame has been saved. These flags can be checked whenever a syscall calls ptrace_stop().

Check flag bits in get_reg()/put_reg() and prevent access to registers that are saved on the switch stack, in case the syscall did not actually save these registers on the switch stack.

Quoted 7 lines

Tested on ARAnyM only - boots and survives running strace on a binary, nothing fancy.

CC Eric W. Biederman <[email protected]>

CC Linus Torvalds <[email protected]>

CC Andreas Schwab <[email protected]>

Signed-off-by Michael Schmitz <[email protected]>

Thanks for your patch!

arch/m68k/kernel/entry.S line 55

Quoted 8 lines

arch/m68k/kernel/entry.S +2 -0

@@ -51,75 +51,115 @@
 
  .text
  ENTRY(__sys_fork)
+       movel   %curptr@(TASK_STACK),%a1
+       orb     #TIS_SWITCH_STACK, %a1@(TINFO_STATUS+3)

This doesn't work on Coldfire:

arch/m68k/kernel/entry.S:55: Error: invalid instruction for this architecture; needs 68000 or higher (68000 [68ec000, 68hc000, 68hc001, 68008, 68302, 68306, 68307, 68322, 68356], 68010, 68020 [68k, 68ec020], 68030 [68ec030], 68040 [68ec040], 68060 [68ec060], cpu32 [68330, 68331, 68332,
68333, 68334, 68336, 68340, 68341, 68349, 68360], fidoa [fido]) --
statement `orb #(1<<1),%a1@(16+3)' ignored

arch/m68k/kernel/entry.S line 60

Quoted 5 lines

arch/m68k/kernel/entry.S +2 -0

         SAVE_SWITCH_STACK
         jbsr    sys_fork
         lea     %sp@(24),%sp
+       movel   %curptr@(TASK_STACK),%a1
+       andb    #TIS_NO_SWITCH_STACK, %a1@(TINFO_STATUS+3)

arch/m68k/kernel/entry.S:60: Error: invalid instruction for this architecture; needs 68000 or higher (68000 [68ec000, 68hc000, 68hc001, 68008, 68302, 68306, 68307, 68322, 68356], 68010, 68020 [68k, 68ec020], 68030 [68ec030], 68040 [68ec040], 68060 [68ec060], cpu32 [68330, 68331, 68332, 68333, 68334, 68336, 68340, 68341, 68349, 68360], fidoa [fido]) -- statement `andb #(~((1<<1))),%a1@(16+3)' ignored

arch/m68k/kernel/entry.S line 61

Quoted 1 line

arch/m68k/kernel/entry.S +0 -0

         rts

Gr{oetje,eeting}s,

Geert

Just out of interest - what would be the correct way to set/clear a single bit on Coldfire? Add/subtract the 1<<bit value?

Quoted 3 lines · Michael Schmitz

Just out of interest - what would be the correct way to set/clear a single bit on Coldfire? Add/subtract the 1<<bit value?

I think BSET/BCLR are the way to go.

Or, alternatively, put the constant in a register, and use a longword memory access. The arithmetic ops don't do immediates _and_ memory operands in Coldfire, and they don't do byte ops.

Or something like that.

m68k version of Eric's patch series 'alpha/ptrace: Improved switch_stack handling'.

Registers d6, d7, a3-a6 are not saved on the stack by default on every syscall entry by the m68k kernel. A separate switch stack frame is pushed to save those registers as needed. This leaves the majority of syscalls with only a subset of registers on the stack, and access to unsaved registers in those would expose or modify random stack addresses.

Patch 1 and 2 add a switch stack for all syscalls that were found to need one to allow ptrace access to all registers outside of syscall entry/exit tracing, as well as kernel worker threads. This ought to protect against accidents.

Patch 3 adds safety checks and debug output to m68k get_reg() and put_reg() functions. Any unsafe register access during process tracing will be prevented and reported.

Suggestions for optimizations or improvements welcome!

Cheers,

Michael

Link https://lore.kernel.org/r/<87pmwlek8d.fsf_-_@disp2133> open here

I have been digging into this some more and I have found one place that I am having a challenge dealing with.

In arch/m68k/fpsp040/skeleton.S there is an assembly version of copy_from_user that calls fpsp040_die when the bytes can not be read.

Now fpsp040_die is just:

/*
* This function is called if an error occur while accessing
* user-space from the fpsp040 code.
*/
asmlinkage void fpsp040_die(void)

{
	do_exit(SIGSEGV);
}

The problem here is the instruction emulation performed in the fpsp040 code performs a very minimal saving of registers. I don't think even the normal system call entry point registers that are saved are present at that point.

Is there any chance you can help me figure out how to get a stack frame with all of the registers present before fpsp040_die is called?

Eric

Eric,

Quoted 17 lines · Eric W. Biederman

I have been digging into this some more and I have found one place that I am having a challenge dealing with.

In arch/m68k/fpsp040/skeleton.S there is an assembly version of copy_from_user that calls fpsp040_die when the bytes can not be read.

Now fpsp040_die is just:

/*
* This function is called if an error occur while accessing
* user-space from the fpsp040 code.
*/
asmlinkage void fpsp040_die(void)

{
    do_exit(SIGSEGV);
}

Quoted 7 lines

The problem here is the instruction emulation performed in the fpsp040 code performs a very minimal saving of registers. I don't think even the normal system call entry point registers that are saved are present at that point.

Is there any chance you can help me figure out how to get a stack frame with all of the registers present before fpsp040_die is called?

I suppose adding the following code (untested) to entry.S:

ENTRY(fpsp040_die)
SAVE_ALL_INT
jbsr fpsp040_die_c
jra ret_from_exception

along with renaming above C entry point to fpsp040_die_c would add the basic saved registers, but these would not necessarily reflect the state of the processor when the fpsp040 trap was called. Is that what you're after?

Am 16.07.2021 um 11:10 schrieb Michael Schmitz:

Quoted 35 lines

Eric,

Quoted 17 lines · Eric W. Biederman

I have been digging into this some more and I have found one place that I am having a challenge dealing with.

In arch/m68k/fpsp040/skeleton.S there is an assembly version of copy_from_user that calls fpsp040_die when the bytes can not be read.

Now fpsp040_die is just:

/*
* This function is called if an error occur while accessing
* user-space from the fpsp040 code.
*/
asmlinkage void fpsp040_die(void)

{
    do_exit(SIGSEGV);
}

Quoted 7 lines

The problem here is the instruction emulation performed in the fpsp040 code performs a very minimal saving of registers. I don't think even the normal system call entry point registers that are saved are present at that point.

Is there any chance you can help me figure out how to get a stack frame with all of the registers present before fpsp040_die is called?

I suppose adding the following code (untested) to entry.S:

ENTRY(fpsp040_die)
SAVE_ALL_INT
jbsr fpsp040_die_c
jra ret_from_exception

Quoted 4 lines

along with renaming above C entry point to fpsp040_die_c would add the basic saved registers, but these would not necessarily reflect the state of the processor when the fpsp040 trap was called. Is that what you're after?

I should have looked more closely at skeleton.S - most FPU exceptions handled there call trap_c the same way as is done for generic traps, i.e. SAVE_ALL_INT before, ret_from_exception after.

Instead of adding code to entry.S, much better to add it in skeleton.S. I'll try to come up with a way to test this code path (calling fpsp040_die from the dz exception hander seems much the easiest way) to make sure this doesn't have side effects.

Does do_exit() ever return?

Quoted 4 lines

I should have looked more closely at skeleton.S - most FPU exceptions handled there call trap_c the same way as is done for generic traps, i.e. SAVE_ALL_INT before, ret_from_exception after.

Quoted 6 lines

Instead of adding code to entry.S, much better to add it in skeleton.S. I'll try to come up with a way to test this code path (calling fpsp040_die from the dz exception hander seems much the easiest way) to make sure this doesn't have side effects.

Does do_exit() ever return?

No. The function do_exit never returns.

If it is not too much difficulty I would be in favor of having the code do force_sigsegv(SIGSEGV), instead of calling do_exit directly.

Looking at that code I have not been able to figure out the call paths that get into skeleton.S. I am not certain saving all of the registers on an the exceptions that reach there make sense. In practice I suspect

taking an exception is much more expensive than saving the registers so it might not make any difference. But this definitely looks like code that is performance sensitive.

My sense when I was reading through skeleton.S was just one or two registers were saved before the instruction emulation was called.

Hi Eric,

Am 18.07.2021 um 06:52 schrieb Eric W. Biederman:

Quoted 4 lines

Quoted 4 lines

I should have looked more closely at skeleton.S - most FPU exceptions handled there call trap_c the same way as is done for generic traps, i.e. SAVE_ALL_INT before, ret_from_exception after.

Quoted 8 lines

Quoted 6 lines

Instead of adding code to entry.S, much better to add it in skeleton.S. I'll try to come up with a way to test this code path (calling fpsp040_die from the dz exception hander seems much the easiest way) to make sure this doesn't have side effects.

Does do_exit() ever return?

No. The function do_exit never returns.

Fine - nothing to worry about as regards restoring the stack pointer correctly then.

Quoted 2 lines

If it is not too much difficulty I would be in favor of having the code do force_sigsegv(SIGSEGV), instead of calling do_exit directly.

That _would_ force a return, right? The exception handling in skeleton.S won't be set up for that.

Quoted 4 lines

Looking at that code I have not been able to figure out the call paths that get into skeleton.S. I am not certain saving all of the registers on an the exceptions that reach there make sense. In practice I suspect

The registers are saved only so trap_c has a stack frame to work with. In that sense, adding a stack frame before calling fpsp040_die is no different.

Quoted 4 lines

taking an exception is much more expensive than saving the registers so it
might not make any difference. But this definitely looks like code that is performance sensitive.

We're only planning to add a stack frame save before calling out of the user access exception handler, right? I doubt that will be called very often.

Quoted 2 lines

My sense when I was reading through skeleton.S was just one or two registers were saved before the instruction emulation was called.

skeleton.S only contains the entry points for code to handle FPU exceptions, from what I've seen (plus the user space access code).

Wherever that exception handling requires calling into the C exception handler (trap_c), a stack frame is added.

Cheers,

Michael

Quoted 1 line

arch/m68k/fpsp040/skeleton.S in_ea: · line 514

Quoted 6 lines · Michael Schmitz

arch/m68k/fpsp040/skeleton.S +6 -0

+	addql   #8,%sp
+	addql   #8,%sp
+	addql   #8,%sp
+	addql   #8,%sp
+	addql   #8,%sp
+	addql   #4,%sp

aka lea 44(%sp),%sp

Somewhere in entry.S is

addql #8,%sp
addql #4,%sp

- is that faster than

lea 12(%sp),%sp ?

Quoted 8 lines

Somewhere in entry.S is

addql #8,%sp
addql #4,%sp

- is that faster than

lea 12(%sp),%sp ?

On the 68040 the timing can depend on the other instructions around it. Each of those addql instructions is listed as 1 and 1 for fetch/execute, while that lea is listed as 2 and 1L+1 meaning that it could potentially be faster depending on the behavior of the instruction that preceded it thorough the execute stage. That one free cycle if the stage is busy (due to the 1L) could make it effectively faster since the first addql would have to wait that extra cycle in that case.

On the 68060, it looks like the lea version is the clear winner, although the timing description is obviously much more complicated and thus I might have missed something. From a quick look, it seems that lea takes the same time as just the first addql.

On CPU32, the lea version loses due to the extra 3 cycles from the addressing mode, even though the base cycles of lea are the same as for addql (2 cycles each). The lea might be even worse if it can't take advantage of overlapping the surrounding instructions (1 cycle before and 1 after).

Those are the only ones I already have the documentation in my hands. I haven't checked older classic cores or coldfire, but it does seem like it is specific to each chip which is faster.

Obviously both versions would be the same size (2 words).

Am 18.07.2021 um 08:09 schrieb Michael Schmitz:

Quoted 16 lines

Hi Eric,

Am 18.07.2021 um 06:52 schrieb Eric W. Biederman:

Quoted 4 lines

Quoted 4 lines

I should have looked more closely at skeleton.S - most FPU exceptions handled there call trap_c the same way as is done for generic traps, i.e. SAVE_ALL_INT before, ret_from_exception after.

Quoted 8 lines

Quoted 6 lines

Instead of adding code to entry.S, much better to add it in skeleton.S. I'll try to come up with a way to test this code path (calling fpsp040_die from the dz exception hander seems much the easiest way) to make sure this doesn't have side effects.

Does do_exit() ever return?

No. The function do_exit never returns.

Quoted 8 lines

Fine - nothing to worry about as regards restoring the stack pointer correctly then.

Quoted 2 lines

If it is not too much difficulty I would be in favor of having the code do force_sigsegv(SIGSEGV), instead of calling do_exit directly.

That _would_ force a return, right? The exception handling in skeleton.S won't be set up for that.

See attached patch - note that when you change fpsp040_die() to call force_sig(SIGSEGV), the access exception handler will return to whatever function in fpsp040 attempted the user space access, and continue that operation with quite likely bogus data. That may well force another FPU trap before the SIGSEGV is delivered (will force_sig() immediately force a trap, or wait until returning to user space?).

Compile tested - haven't found an easy way to execute that code path yet.

Cheers,

Michael

Quoted 24 lines

Quoted 4 lines

Looking at that code I have not been able to figure out the call paths that get into skeleton.S. I am not certain saving all of the registers on an the exceptions that reach there make sense. In practice I suspect

The registers are saved only so trap_c has a stack frame to work with. In that sense, adding a stack frame before calling fpsp040_die is no different.

Quoted 4 lines

taking an exception is much more expensive than saving the registers so it
might not make any difference. But this definitely looks like code that is performance sensitive.

We're only planning to add a stack frame save before calling out of the user access exception handler, right? I doubt that will be called very often.

Quoted 2 lines

My sense when I was reading through skeleton.S was just one or two registers were saved before the instruction emulation was called.

skeleton.S only contains the entry points for code to handle FPU exceptions, from what I've seen (plus the user space access code).

Quoted 8 lines

Wherever that exception handling requires calling into the C exception handler (trap_c), a stack frame is added.

Cheers,

Michael

Quoted 1 line

From 1e9be9238fb88dc0b87a7ffdd48068f944d8626c Mon Sep 17 00:00:00 2001
From: Michael Schmitz <[email protected]>
Date: Sun, 18 Jul 2021 10:31:42 +1200
Subject: [PATCH] m68k/fpsp040 - save full stack frame before calling
fpsp040_die

The FPSP040 floating point support code does not know how to handle user space access faults gracefully, and just calls do_exit(SIGSEGV) indirectly on these faults to abort.

do_exit() may stop if traced, and needs a full stack frame available to avoid exposing kernel data.

Add the current stack frame before calling do_exit() from the fpsp040 user access exception handler. Unwind the stack frame and return to caller once done, in case do_exit() is replaced by force_sig() later on.

CC Eric W. Biederman <[email protected]>

Signed-off-by Michael Schmitz <[email protected]>

---
 arch/m68k/fpsp040/skeleton.S | 10 ++++++++++
 1 file changed, 10 insertions(+)

arch/m68k/fpsp040/skeleton.S +0 -0

index a8f4161..6c92d38 100644

arch/m68k/fpsp040/skeleton.S +0 -0

 	.section __ex_table,#alloc
 	.align	4

arch/m68k/fpsp040/skeleton.S in_ea: · line 507

Quoted 12 lines

arch/m68k/fpsp040/skeleton.S +3 -0

index a8f4161..6c92d38 100644
@@ -502,7 +502,17 @@ in_ea:
  	.section .fixup,#alloc,#execinstr
  	.even
  1:
+
+	SAVE_ALL_INT
+	SAVE_SWITCH_STACK

         ^^^^^^^^^^

I don't think this saves the registers in the well known fixed location on the stack because some registers are saved at the exception entry point.

Without being saved at the well known fixed location if some process stops in PTRACE_EVENT_EXIT in do_exit we likely get some complete gibberish.

That is probably safe.

arch/m68k/fpsp040/skeleton.S in_ea: · line 515

Quoted 8 lines

arch/m68k/fpsp040/skeleton.S +7 -0

  	jbra	fpsp040_die
+	addql   #8,%sp
+	addql   #8,%sp
+	addql   #8,%sp
+	addql   #8,%sp
+	addql   #8,%sp
+	addql   #4,%sp
+	rts

Especially as everything after jumping to fpsp040_die does not execute.

Eric

Quoted 3 lines

arch/m68k/fpsp040/skeleton.S +0 -0

  	.section __ex_table,#alloc
  	.align	4

Hi Eric,

Quoted 14 lines · Eric W. Biederman

arch/m68k/fpsp040/skeleton.S in_ea: · line 507

Quoted 12 lines

arch/m68k/fpsp040/skeleton.S +3 -0

index a8f4161..6c92d38 100644
@@ -502,7 +502,17 @@ in_ea:
  	.section .fixup,#alloc,#execinstr
  	.even
  1:
+
+	SAVE_ALL_INT
+	SAVE_SWITCH_STACK

         ^^^^^^^^^^

Quoted 3 lines

I don't think this saves the registers in the well known fixed location on the stack because some registers are saved at the exception entry point.

The FPU exception entry points are not using the exception entry code in head.S. These entry points are stored in the exception vector table directly. No saving of a syscall stack frame happens there. The FPU places its exception frame on the stack, and that is what the FPU exception handlers use.

(If these have to call out to the generic exception handlers again, they will build a minimal stack frame, see code in skeleton.S.)

Calling fpsp040_die() is no different from calling a syscall that may need to have access to the full stack frame. The 'fixed location' is just 'on the stack before calling  fpsp040_die()', again this is no different from calling e.g. sys_fork() which does not take a pointer to the begin of the stack frame as an argument.

I must admit I never looked at how do_exit() figures out where the stack frame containing the saved registers is stored, I just assumed it unwinds the stack up to the point where the caller syscall was made, and works from there. The same strategy ought to work here.

Quoted 6 lines

Without being saved at the well known fixed location if some process stops in PTRACE_EVENT_EXIT in do_exit we likely get some complete gibberish.

That is probably safe.

Quoted 9 lines

arch/m68k/fpsp040/skeleton.S in_ea: · line 515

Quoted 8 lines

arch/m68k/fpsp040/skeleton.S +7 -0

  	jbra	fpsp040_die
+	addql   #8,%sp
+	addql   #8,%sp
+	addql   #8,%sp
+	addql   #8,%sp
+	addql   #8,%sp
+	addql   #4,%sp
+	rts

Especially as everything after jumping to fpsp040_die does not execute.

Unless we change fpsp040_die() to call force_sig(SIGSEGV).

Quoted 35 lines · Michael Schmitz

Hi Eric,

Quoted 14 lines · Eric W. Biederman

arch/m68k/fpsp040/skeleton.S in_ea: · line 507

Quoted 12 lines

arch/m68k/fpsp040/skeleton.S +3 -0

index a8f4161..6c92d38 100644
@@ -502,7 +502,17 @@ in_ea:
  	.section .fixup,#alloc,#execinstr
  	.even
  1:
+
+	SAVE_ALL_INT
+	SAVE_SWITCH_STACK

         ^^^^^^^^^^

Quoted 3 lines

I don't think this saves the registers in the well known fixed location on the stack because some registers are saved at the exception entry point.

The FPU exception entry points are not using the exception entry code in head.S. These entry points are stored in the exception vector table directly. No saving of a syscall stack frame happens there. The FPU places its exception frame on the stack, and that is what the FPU exception handlers use.

(If these have to call out to the generic exception handlers again, they will build a minimal stack frame, see code in skeleton.S.)

Calling fpsp040_die() is no different from calling a syscall that may need to have access to the full stack frame. The 'fixed location' is just 'on the stack before calling fpsp040_die()', again this is no different from calling e.g. sys_fork() which does not take a pointer to the begin of the stack frame as an argument.

Quoted 4 lines

I must admit I never looked at how do_exit() figures out where the stack frame containing the saved registers is stored, I just assumed it unwinds the stack up to the point where the caller syscall was made, and works from there. The same strategy ought to work here.

For do_exit the part we need to be careful with is PTRACE_EVENT_EXIT, which means it is ptrace that we need to look at.

For m68k the code in put_reg and get_reg finds the registers by looking at task->thread.esp0.

I was expecting m68k to use the same technique as alpha which expects a fixed offset from task_stack_page(task).

So your code will work if you add code to update task->thread.esp0 which is also known as THREAD_ESP0 in entry.S

Quoted 17 lines

Quoted 16 lines

Without being saved at the well known fixed location if some process stops in PTRACE_EVENT_EXIT in do_exit we likely get some complete gibberish.

That is probably safe.

arch/m68k/fpsp040/skeleton.S in_ea: · line 515

Quoted 8 lines

arch/m68k/fpsp040/skeleton.S +7 -0

  	jbra	fpsp040_die
+	addql   #8,%sp
+	addql   #8,%sp
+	addql   #8,%sp
+	addql   #8,%sp
+	addql   #8,%sp
+	addql   #4,%sp
+	rts

Especially as everything after jumping to fpsp040_die does not execute.

Quoted 1 line

Unless we change fpsp040_die() to call force_sig(SIGSEGV).

Yes. I think we would probably need to have it also call get_signal and all of that, because I don't think the very light call path for that exception includes testing if signals are pending.

The way the code is structured it is actively incorrect to return from fpsp040_die, as the code does not know what to do if it reads a byte from userspace and there is nothing there.

So instead of handling -EFAULT like most pieces of kernel code the code just immediately calls do_exit, and does not even attempt to handle the error.

That is not my favorite strategy at all, but I suspect it isn't worth it, or safe to update the skeleton.S to handle errors. Especially as we have not even figured out how to test that code yet.

Eric

Hi Eric,

Am 23.07.2021 um 02:49 schrieb Eric W. Biederman:

Quoted 42 lines

Quoted 35 lines · Michael Schmitz

Hi Eric,

Quoted 14 lines · Eric W. Biederman

arch/m68k/fpsp040/skeleton.S in_ea: · line 507

Quoted 12 lines

arch/m68k/fpsp040/skeleton.S +3 -0

index a8f4161..6c92d38 100644
@@ -502,7 +502,17 @@ in_ea:
  	.section .fixup,#alloc,#execinstr
  	.even
  1:
+
+	SAVE_ALL_INT
+	SAVE_SWITCH_STACK

         ^^^^^^^^^^

Quoted 3 lines

I don't think this saves the registers in the well known fixed location on the stack because some registers are saved at the exception entry point.

The FPU exception entry points are not using the exception entry code in head.S. These entry points are stored in the exception vector table directly. No saving of a syscall stack frame happens there. The FPU places its exception frame on the stack, and that is what the FPU exception handlers use.

(If these have to call out to the generic exception handlers again, they will build a minimal stack frame, see code in skeleton.S.)

Calling fpsp040_die() is no different from calling a syscall that may need to have access to the full stack frame. The 'fixed location' is just 'on the stack before calling fpsp040_die()', again this is no different from calling e.g. sys_fork() which does not take a pointer to the begin of the stack frame as an argument.

Quoted 4 lines

I must admit I never looked at how do_exit() figures out where the stack frame containing the saved registers is stored, I just assumed it unwinds the stack up to the point where the caller syscall was made, and works from there. The same strategy ought to work here.

Quoted 5 lines

For do_exit the part we need to be careful with is PTRACE_EVENT_EXIT, which means it is ptrace that we need to look at.

For m68k the code in put_reg and get_reg finds the registers by looking at task->thread.esp0.

Thanks, that's what I was missing here.

Quoted 6 lines

I was expecting m68k to use the same technique as alpha which expects a fixed offset from task_stack_page(task).

So your code will work if you add code to update task->thread.esp0 which is also known as THREAD_ESP0 in entry.S

Shoving

movel %sp,%curptr@(TASK_THREAD+THREAD_ESP0)

in between the SAVE_ALL_INT and SAVE_SWITCH_STACK ought to do the trick there.

Quoted 17 lines

Quoted 17 lines

Quoted 16 lines

Without being saved at the well known fixed location if some process stops in PTRACE_EVENT_EXIT in do_exit we likely get some complete gibberish.

That is probably safe.

arch/m68k/fpsp040/skeleton.S in_ea: · line 515

Quoted 8 lines

arch/m68k/fpsp040/skeleton.S +7 -0

  	jbra	fpsp040_die
+	addql   #8,%sp
+	addql   #8,%sp
+	addql   #8,%sp
+	addql   #8,%sp
+	addql   #8,%sp
+	addql   #4,%sp
+	rts

Especially as everything after jumping to fpsp040_die does not execute.

Quoted 5 lines

Quoted 1 line

Unless we change fpsp040_die() to call force_sig(SIGSEGV).

Yes. I think we would probably need to have it also call get_signal and all of that, because I don't think the very light call path for that exception includes testing if signals are pending.

As far as I can see, there is a test for pending signals:

ENTRY(ret_from_exception)
.Lret_from_exception:

        btst    #5,%sp@(PT_OFF_SR)      | check if returning to kernel
        bnes    1f                      | if so, skip resched, signals
        | only allow interrupts when we are really the last one on the
        | kernel stack, otherwise stack overflow can occur during
        | heavy interrupt load

andw #ALLOWINT,%sr

resume_userspace:

        movel   %curptr@(TASK_STACK),%a1
        moveb   %a1@(TINFO_FLAGS+3),%d0	| bits 0-7 of TINFO_FLAGS
        jne     exit_work		| any bit set? -> exit_work

1: RESTORE_ALL

exit_work:
| save top of frame
movel %sp,%curptr@(TASK_THREAD+THREAD_ESP0)
lslb #1,%d0 | shift out TIF_NEED_RESCHED
jne do_signal_return | any remaining bit
(signal/notify_resume)? -> do_signal_return
pea resume_userspace
jra schedule

As long as TIF_NOTIFY_SIGNAL or TIF_SIGPENDING are set, do_signal_return will be called.

Quoted 3 lines

The way the code is structured it is actively incorrect to return from fpsp040_die, as the code does not know what to do if it reads a byte from userspace and there is nothing there.

Correct - my hope is that upon return from the FPU exception (that continued after a dodgy read or write), we get the signal delivered and will die then.

Quoted 7 lines

So instead of handling -EFAULT like most pieces of kernel code the code just immediately calls do_exit, and does not even attempt to handle the error.

That is not my favorite strategy at all, but I suspect it isn't worth it, or safe to update the skeleton.S to handle errors. Especially as we have not even figured out how to test that code yet.

That's bothering me more than a little, but I need to find out whether the emulator even handles FPU exceptions correctly ...

Quoted 32 lines

Quoted 5 lines

Quoted 1 line

Unless we change fpsp040_die() to call force_sig(SIGSEGV).

Yes. I think we would probably need to have it also call get_signal and all of that, because I don't think the very light call path for that exception includes testing if signals are pending.

As far as I can see, there is a test for pending signals:

ENTRY(ret_from_exception)
.Lret_from_exception:

        btst    #5,%sp@(PT_OFF_SR)      | check if returning to kernel
        bnes    1f                      | if so, skip resched, signals
        | only allow interrupts when we are really the last one on the
        | kernel stack, otherwise stack overflow can occur during
        | heavy interrupt load

andw #ALLOWINT,%sr

resume_userspace:

        movel   %curptr@(TASK_STACK),%a1
        moveb   %a1@(TINFO_FLAGS+3),%d0	| bits 0-7 of TINFO_FLAGS
        jne     exit_work		| any bit set? -> exit_work

1: RESTORE_ALL

exit_work:
| save top of frame
movel %sp,%curptr@(TASK_THREAD+THREAD_ESP0)
lslb #1,%d0 | shift out TIF_NEED_RESCHED
jne do_signal_return | any remaining bit
(signal/notify_resume)? -> do_signal_return
pea resume_userspace
jra schedule

Quoted 2 lines

As long as TIF_NOTIFY_SIGNAL or TIF_SIGPENDING are set, do_signal_return will be called.

I was going to say I don't think so, as my tracing of the code lead in a couple of different directions. Upon closer inspection all those paths either lead to fpsp_done or more directly to ret_from_exception.

For anyone else who might want to trace the code, or for myself later on when I forget. As best as I can figure the hardware exception vector table is setup in: arch/m68k/kernel/vector.c

For the vectors in question it appears to be this chunk of code:

	if (CPU_IS_040 && !FPU_IS_EMU) {
		/* set up FPSP entry points */
		asmlinkage void dz_vec(void) asm ("dz");
		asmlinkage void inex_vec(void) asm ("inex");
		asmlinkage void ovfl_vec(void) asm ("ovfl");
		asmlinkage void unfl_vec(void) asm ("unfl");
		asmlinkage void snan_vec(void) asm ("snan");
		asmlinkage void operr_vec(void) asm ("operr");
		asmlinkage void bsun_vec(void) asm ("bsun");
		asmlinkage void fline_vec(void) asm ("fline");
		asmlinkage void unsupp_vec(void) asm ("unsupp");

		vectors[VEC_FPDIVZ] = dz_vec;
		vectors[VEC_FPIR] = inex_vec;

		vectors[VEC_FPOVER] = ovfl_vec;
		vectors[VEC_FPUNDER] = unfl_vec;
		vectors[VEC_FPNAN] = snan_vec;
		vectors[VEC_FPOE] = operr_vec;
		vectors[VEC_FPBRUC] = bsun_vec;
		vectors[VEC_LINE11] = fline_vec;
		vectors[VEC_FPUNSUP] = unsupp_vec;
	}

Which leads me to call traces that look like this:

hw
fline

    fpsp_fline
       mem_read
          user_read
             copyin
               in_ea
                  <page-fault>
                     fpsp040_die

If that mem_read returns it can be followed by
not_mvcr
real_fline
ret_from_exception

Or it can be followed by

       fix_con
          uni_2
             gen_except
                 do_clean
                    finish_up
                       fpsp_done
                          ret_from_exception

Quoted 4 lines

Quoted 3 lines

The way the code is structured it is actively incorrect to return from fpsp040_die, as the code does not know what to do if it reads a byte from userspace and there is nothing there.

Quoted 3 lines

Correct - my hope is that upon return from the FPU exception (that continued after a dodgy read or write), we get the signal delivered and will die then.

Yes. That does look like a good strategy.

I am wondering if there are values we can return that will make the path out of the exit routine more deterministic.

I have played with that a little bit today, but it doesn't look like I am going to have time to put together any kind of real patch today.

Simply modifying fpsp040_die to call force_sigsegv(SIGSEGV) should be enough to trigger a signal (no call stack work needed if we remove do_exit). The tricky bit is what value do we want to fake when we
can not read anything from userspace. For a write fault we should just
be able to skip the write entirely.

In both cases we probably should break out of the loop prematurely. But I don't know if that is necessary.

The lazy strategy would be to copy the ifpsp060 code and simply oops the kernel if the read or write of userspace gets a page fault.

Quoted 10 lines

Quoted 7 lines

So instead of handling -EFAULT like most pieces of kernel code the code just immediately calls do_exit, and does not even attempt to handle the error.

That is not my favorite strategy at all, but I suspect it isn't worth it, or safe to update the skeleton.S to handle errors. Especially as we have not even figured out how to test that code yet.

That's bothering me more than a little, but I need to find out whether the emulator even handles FPU exceptions correctly ...

As a fallback plan we can following the lead of ifpsp060/os.S and simply not catch the kernel triggered page fault, and let arch/m68k/mm/fault.c:send_fault_sig() return a kernel oops. It is not ideal as it allows userspace to trigger a kernel oops, but it does at least keep the kernel in a consistent state.

arch/m68k/fpsp040/skeleton.S +0 -13

index a8f41615d94a..4c6c4b07ef38 100644
@@ -479,7 +479,6 @@ copyout:
 |	movec	%d1,%DFC		| set dfc for user data space
 moreout:
 	moveb	(%a0)+,%d1	| fetch supervisor byte
-out_ea:
 	movesb	%d1,(%a1)+	| write user byte
 	dbf	%d0,moreout
 	rts

@@ -493,21 +492,9 @@ copyin:
 |	SFC is already set
 |	movec	%d1,%SFC		| set sfc for user space
 morein:
-in_ea:
 	movesb	(%a0)+,%d1	| fetch user byte
 	moveb	%d1,(%a1)+	| write supervisor byte
 	dbf	%d0,morein
 	rts
 
-	.section .fixup,#alloc,#execinstr
-	.even
-1:
-	jbra	fpsp040_die
-
-	.section __ex_table,#alloc
-	.align	4
-
-	.long	in_ea,1b
-	.long	out_ea,1b
-
 	|end

arch/m68k/kernel/traps.c +0 -9

index e2a6f3556211..3ec6ae1bdaf9 100644
@@ -1144,15 +1144,6 @@ asmlinkage void set_esp0(unsigned long ssp)
 	current->thread.esp0 = ssp;
 }
 
-/*
- * This function is called if an error occur while accessing
- * user-space from the fpsp040 code.
- */
-asmlinkage void fpsp040_die(void)
-{
-	do_exit(SIGSEGV);
-}
-
 #ifdef CONFIG_M68KFPU_EMU
 asmlinkage void fpemu_signal(int signal, int code, void *addr)
 {

Eric

According to my understanding, you can't get a F-line exception on 68040.

Quoted 3 lines

According to my understanding, you can't get a F-line exception on 68040.

The F-line exeception vector is used for all FPU illegal and unimplemented insns.

Am 25.07.2021 um 00:05 schrieb Andreas Schwab:

Quoted 5 lines · Michael Schmitz

Quoted 3 lines

According to my understanding, you can't get a F-line exception on 68040.

Quoted 2 lines

The F-line exception vector is used for all FPU illegal and unimplemented insns.

Thanks - now from my reading of the fpsp040 code (which has mislead me in the past), it would seem that operations like sin() and exp() ought to raise that exception then. I don't see that in ARAnyM.

Quoted 10 lines · Michael Schmitz

Am 25.07.2021 um 00:05 schrieb Andreas Schwab:

Quoted 6 lines · Michael Schmitz

Quoted 3 lines

According to my understanding, you can't get a F-line exception on 68040.

The F-line exception vector is used for all FPU illegal and unimplemented insns.

Thanks - now from my reading of the fpsp040 code (which has mislead me in the past), it would seem that operations like sin() and exp() ought to raise that exception then. I don't see that in ARAnyM.

Yes, according to the 68040 user's manual, unimplemented and illegal F-line instructions trigger the standard F-line exception vector (11) but have separate stack frame formats so the fpsp040 code gets some extra data. The CPU does a bunch of the prep work so that part doesn't need to be emulated in software.

The ARAnyM docs appear to claim a strange combination that wouldn't exist in hardware by implementing a full 68882 instead of the limited subset found on a real 68040. Strangely, that might have been easier to implement. However, it would also completely bypass any use of fpsp040.

Brad Boyer
[email protected]

arch/m68k/fpsp040/skeleton.S in_ea: · line 506

Quoted 11 lines · Eric W. Biederman

arch/m68k/fpsp040/skeleton.S +2 -1

index a8f41615d94a..ec767523c012 100644
@@ -502,7 +502,8 @@ in_ea:
 	.section .fixup,#alloc,#execinstr
 	.even
 1:
-	jbra	fpsp040_die
+	bsrl	fpsp040_die
+	jmp	.Lnotkern

That should be jbra instead of jmp.

I could not find a reference mentioning jbra. Do I need to look in the gas source or do you know if there is a better source?

In the fpsp040 code when copyin or copyout fails call force_sigsegv(SIGSEGV) instead of do_exit(SIGSEGV).

This solves a couple of problems. Because do_exit embeds the ptrace stop PTRACE_EVENT_EXIT a complete stack frame needs to be present for that to work correctly. There is always the information needed for a ptrace stop where get_signal is called. So exiting with a signal solves the ptrace issue.

Further exiting with a signal ensures that all of the threads in a process are killed not just the thread that malfunctioned. Which avoids confusing userspace.

To make force_sigsegv(SIGSEGV) work in fpsp040_die modify the code to save all of the registers and jump to ret_from_exception (which ultimately calls get_signal) after fpsp040_die returns.

Signed-off-by "Eric W. Biederman" <[email protected]>

---

Can someone please check my m68k assembly changes?

I think I have them correct, and the code assembles but I don't understand the fine points of when the different branch instructions should be used.

  arch/m68k/fpsp040/skeleton.S | 3 ++-
  arch/m68k/kernel/traps.c     | 2 +-
  2 files changed, 3 insertions(+), 2 deletions(-)

arch/m68k/fpsp040/skeleton.S +2 -1

index a8f41615d94a..ec767523c012 100644
@@ -502,7 +502,8 @@ in_ea:
  	.section .fixup,#alloc,#execinstr
  	.even
  1:
-	jbra	fpsp040_die
+	bsrl	fpsp040_die
+	jmp	.Lnotkern
 
  	.section __ex_table,#alloc
  	.align	4

arch/m68k/kernel/traps.c +1 -1

index 9e1261462bcc..5b19fcdcd69e 100644
@@ -1150,7 +1150,7 @@ asmlinkage void set_esp0(unsigned long ssp)
   */
  asmlinkage void fpsp040_die(void)
  {
-	do_exit(SIGSEGV);
+	force_sigsegv(SIGSEGV);
  }
 
  #ifdef CONFIG_M68KFPU_EMU

In the fpsp040 code when copyin or copyout fails call force_sigsegv(SIGSEGV) instead of do_exit(SIGSEGV).

This solves a couple of problems. Because do_exit embeds the ptrace stop PTRACE_EVENT_EXIT a complete stack frame needs to be present for that to work correctly. There is always the information needed for a ptrace stop where get_signal is called. So exiting with a signal solves the ptrace issue.

Further exiting with a signal ensures that all of the threads in a process are killed not just the thread that malfunctioned. Which avoids confusing userspace.

To make force_sigsegv(SIGSEGV) work in fpsp040_die modify the code to save all of the registers and jump to ret_from_exception (which ultimately calls get_signal) after fpsp040_die returns.

v2: Updated the branches to use gas's pseudo ops that automatically
calculate the best branch instruction to use for the purpose.

v1: Link: https://lkml.kernel.org/r/87a6m8kgtx.fsf_-_@disp2133

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 arch/m68k/fpsp040/skeleton.S | 3 ++-
 arch/m68k/kernel/traps.c     | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

arch/m68k/fpsp040/skeleton.S +2 -1

index a8f41615d94a..439395aa6fb4 100644
@@ -502,7 +502,8 @@ in_ea:
 	.section .fixup,#alloc,#execinstr
 	.even
 1:
-	jbra	fpsp040_die
+	jbsr	fpsp040_die
+	jbra	.Lnotkern
 
 	.section __ex_table,#alloc
 	.align	4

arch/m68k/kernel/traps.c +1 -1

index 9e1261462bcc..5b19fcdcd69e 100644
@@ -1150,7 +1150,7 @@ asmlinkage void set_esp0(unsigned long ssp)
  */
 asmlinkage void fpsp040_die(void)
 {
-	do_exit(SIGSEGV);
+	force_sigsegv(SIGSEGV);
 }
 
 #ifdef CONFIG_M68KFPU_EMU

In the fpsp040 code when copyin or copyout fails call force_sigsegv(SIGSEGV) instead of do_exit(SIGSEGV).

This solves a couple of problems. Because do_exit embeds the ptrace stop PTRACE_EVENT_EXIT a complete stack frame needs to be present for that to work correctly. There is always the information needed for a ptrace stop where get_signal is called. So exiting with a signal solves the ptrace issue.

Further exiting with a signal ensures that all of the threads in a process are killed not just the thread that malfunctioned. Which avoids confusing userspace.

To make force_sigsegv(SIGSEGV) work in fpsp040_die modify the code to save all of the registers and jump to ret_from_exception (which ultimately calls get_signal) after fpsp040_die returns.

v2: Updated the branches to use gas's pseudo ops that automatically
calculate the best branch instruction to use for the purpose.

v1: Link: https://lkml.kernel.org/r/87a6m8kgtx.fsf_-_@disp2133

Signed-off-by "Eric W. Biederman" <[email protected]>

I don't see that in ARAnyM.