Re: [PATCH v2] m68k: save extra registers on more syscall entry points

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Linus,

Am 19.06.2021 um 05:17 schrieb Linus Torvalds:
> On Thu, Jun 17, 2021 at 6:27 PM Michael Schmitz <schmitzmic@gmail.com> wrote:
>>
>> I'd need specific test cases to exercise io_uring_setup in
>> particular, to see whether stack offsets for pt_regs and the
>> switch stack have been messed up.
>
> I don't think doing this for io_uring_setup() will help any - the
> problem is not in that system call thread itself, it's purely in the
> kernel thread that it then starts.
>
> And the fact that io_uring_setup() has the full stack frame won't then
> help that kernel thread, for exactly the same reason that was true on
> alpha: copy_thread() will actually _create_ the full stack, but when
> we switch to it (through switch_to() -> resume()), the resume code in
> arch/m68k/kernel/entry.S will switch to that stack, and then do
> RESTORE_SWITCH_STACK which will consume it again.

Now I see ... I had glossed over the point in resume() where the stack 
pointer was switched to the new task.

> So I think m68k should do the same thing as Eric's patch for alpha: do
> the full stack for exit and exit_group, and for kernel thread creation
> - or at least PF_IO_WORKER), do an extra stack frame on the kernel
> stack, so that even after resume() we'll still have another copy of
> the frame.

Is your patch to copy_thread() to add the extra stack frame still needed?

(Eric added switch stack save/restore around the worker thread call in 
ret_from_exception, which pushes back the missing stack contents as far 
as I understand ... )

There seems to be no expectation for kernel worker threads to have valid 
saved user context, just space on the stack for the tracer to read. I'll 
drop saving full context for io_uring_setup in v3.

> The alternative would be to do what x86 does: see __switch_to_asm().
> Instead of doing that normal kernel entry/exit stack (with
> SAVE_SWITCH_STACK and RESTORE_SWITCH_STACK), x86 has it's own very
> special "only for task switching" stack frame thing, and leaves the
> pt_regs etc entirely alone.
>
> Of course, that "only for task switching" is _kind_of_ what the whole
> SAVE_SWITCH_STACK is for - it's part of the name after all - but the
> difference is that on alpha and m68k, it's also (and primarily) the
> "full state" stack frame, used not just for task switching, but for
> signal handling state and for ptrace too.
>
> So in theory, it would be good to split this up:
>
>  (a) have the signal handling and ptrace stack be one thing (maybe
> rename the "SWITCH" part of the operations to something else, like
> "EXTRA" or "SIGNAL" or whatever)

We can rename the macros, but we also expose struct switch_stack in 
uapi/asm/ptrace.h - hard to rename that one.

>  (b) make a separate "for task switching only" stack frame, which is
> used by that switch_to() -> resume() sequence, and that copy_thread()
> has a "struct inactive_task_frame" thing for..

That's more than a bit beyond my m68k assembly level, sorry. Maybe 
Andreas can help if we decide to go that way?

Cheers,

	Michael

>
> That way, the pt_regs/extra_regs stack frame that copy_thread()
> creates wouldn't then be eaten up by the task switch.
>
> But while that sounds like the right thing to do, it would be a rather
> bigger change. I'm not entirely sure it's worth it.
>
> Eric, comments?
>
>                 Linus
>