Hi Linus,
Am 19.06.2021 um 05:17 schrieb Linus Torvalds:
On Thu, Jun 17, 2021 at 6:27 PM Michael Schmitz <schmitzmic@xxxxxxxxx> 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
