Michael Schmitz [off-list ref] writes:

Hi Eric,

On 21/07/21 8:32 am, Eric W. Biederman wrote:

quoted

quoted

diff --git a/arch/m68k/fpsp040/skeleton.S b/arch/m68k/fpsp040/skeleton.S
index a8f4161..6c92d38 100644
--- a/arch/m68k/fpsp040/skeleton.S
+++ b/arch/m68k/fpsp040/skeleton.S

@@ -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.

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.

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

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

  	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).

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