From: Eric W. Biederman <hidden> Date: 2021-06-15 19:32:00
Michael Schmitz [off-list ref] writes:
Hi Eric,
On 15/06/21 4:26 am, Eric W. Biederman wrote:
quoted
Michael Schmitz [off-list ref] writes:
quoted
On second thought, I'm not certain what adding another empty stack frame would
achieve here.
On m68k, 'frame' already is a new stack frame, for running the new thread
in. This new frame does not have any user context at all, and it's explicitly
wiped anyway.
Unless we save all user context on the stack, then push that context to a new
save frame, and somehow point get_signal to look there for IO threads
(essentially what Eric suggested), I don't see how this could work?
I must be missing something.
It is only designed to work well enough so that ptrace will access
something well defined when ptrace accesses io_uring tasks.
The io_uring tasks are special in that they are user process
threads that never run in userspace. So as long as everything
ptrace can read is accessible on that process all is well.
OK, I'm testing a patch that would save extra context in sys_io_uring_setup,
which ought to ensure that for m68k.
I had to update ret_from_kernel_thread to pop that state to get Linus's
change to boot. Apparently kernel_threads exiting needs to be handled.
quoted
Having stared a bit longer at the code I think the short term
fix for both of PTRACE_EVENT_EXIT and io_uring is to guard
them both with CONFIG_HAVE_ARCH_TRACEHOOK.
Which does not work because nios2 which looks susceptible
sets CONFIG_HAVE_ARCH_TRACEHOOK.
A further look shows that there is also PTRACE_EVENT_EXEC that
needs to be handled so execve and execveat need to be wrapped
as well.
Do you happen to know if there is userspace that will run
in qemu-system-m68k that can be used for testing?
Eric
From: Michael Schmitz <schmitzmic@gmail.com> Date: 2021-06-15 20:56:50
Hi Eric,
On 16/06/21 7:30 am, Eric W. Biederman wrote:
quoted
quoted
The io_uring tasks are special in that they are user process
threads that never run in userspace. So as long as everything
ptrace can read is accessible on that process all is well.
OK, I'm testing a patch that would save extra context in sys_io_uring_setup,
which ought to ensure that for m68k.
I had to update ret_from_kernel_thread to pop that state to get Linus's
change to boot. Apparently kernel_threads exiting needs to be handled.
Hadn't yet got to that stage, sorry. Still stress testing stage 1 of my fix (push complete context). I would have thought that this should be sufficient (gives us a complete stack frame for ptrace code to work on)?
But it makes sense that when you push an extra stack frame, you'd need to pop that on exit.
quoted
quoted
Having stared a bit longer at the code I think the short term
fix for both of PTRACE_EVENT_EXIT and io_uring is to guard
them both with CONFIG_HAVE_ARCH_TRACEHOOK.
Which does not work because nios2 which looks susceptible
sets CONFIG_HAVE_ARCH_TRACEHOOK.
A further look shows that there is also PTRACE_EVENT_EXEC that
needs to be handled so execve and execveat need to be wrapped
as well.
Do you happen to know if there is userspace that will run
in qemu-system-m68k that can be used for testing?
I surmise so. I don't use qemu myself - either ARAnyM, or actual hardware. Hardware is limited to 14 MB RAM, which has prevented me from using more than simple regression testing. In particular, I can't test sys_io_uring_setup there.
Adrian uses qemu a lot, and has supplied disk images to work from on occasion. Maybe he's got something recent enough to support sys_io_uring_setup ... I've CC:ed him in, as I'd love to do some more testing as well.
Cheers,
Michael
On Tue, Jun 15, 2021 at 12:32 PM Eric W. Biederman
[off-list ref] wrote:
I had to update ret_from_kernel_thread to pop that state to get Linus's
change to boot. Apparently kernel_threads exiting needs to be handled.
You are very right.
That, btw, seems to be a horrible design mistake, but I think it's how
"kernel_execve()" works - both for the initial "init", but also for
user-mode helper processes.
Both of those cases do "kernel_thread()" to create a new thread, and
then that new kernel thread does kernel_execve() to create the user
space image for that thread. And that act of "execve()" clears
PF_KTHREAD from the thread, and then the final return from the kernel
thread function returns to that new user space.
Or something like that. It's been ages since I looked at that code,
and your patch initially confused the heck out of me because I went
"that can't _possibly_ be needed".
But yes, I think your patch is right.
And I think our horrible "kernel threads return to user space when
done" is absolutely horrifically nasty. Maybe of the clever sort, but
mostly of the historical horror sort.
Or am I mis-rememberting how this ends up working? Did you look at
exactly what it was that returned from kernel threads?
This might be worth commenting on somewhere. But your patch for alpha
looks correct to me. Did you have some test-case to verify ptrace() on
io worker threads?
Linus
From: Finn Thain <fthain@linux-m68k.org> Date: 2021-06-16 00:23:56
On Wed, 16 Jun 2021, Michael Schmitz wrote:
quoted
Do you happen to know if there is userspace that will run in
qemu-system-m68k that can be used for testing?
I surmise so. I don't use qemu myself - either ARAnyM, or actual
hardware. Hardware is limited to 14 MB RAM, which has prevented me from
using more than simple regression testing. In particular, I can't test
sys_io_uring_setup there.
Adrian uses qemu a lot, and has supplied disk images to work from on
occasion. Maybe he's got something recent enough to support
sys_io_uring_setup ... I've CC:ed him in, as I'd love to do some more
testing as well.
Hi Eric,
On Tue, Jun 15, 2021 at 9:32 PM Eric W. Biederman [off-list ref] wrote:
Do you happen to know if there is userspace that will run
in qemu-system-m68k that can be used for testing?
There's a link to an image in Laurent's patch series "[PATCH 0/2]
m68k: Add Virtual M68k Machine"
https://lore.kernel.org/linux-m68k/20210323221430.3735147-1-laurent@vivier.eu/
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Thanks, I'll try that one.
I'll try and implement a few of the solutions Eric came up with for alpha, unless someone beats me to it (Andreas?).
Cheers,
Michael
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2021-06-21 13:55:20
On Tue, Jun 15, 2021 at 02:58:12PM -0700, Linus Torvalds wrote:
And I think our horrible "kernel threads return to user space when
done" is absolutely horrifically nasty. Maybe of the clever sort, but
mostly of the historical horror sort.
How would you prefer to handle that, then? Separate magical path from
kernel_execve() to switch to userland? We used to have something of
that sort, and that had been a real horror...
As it is, it's "kernel thread is spawned at the point similar to
ret_from_fork(), runs the payload (which almost never returns) and
then proceeds out to userland, same way fork(2) would've done."
That way kernel_execve() doesn't have to do anything magical.
Al, digging through the old notes and current call graph...
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2021-06-21 14:16:41
On Mon, Jun 21, 2021 at 01:54:56PM +0000, Al Viro wrote:
On Tue, Jun 15, 2021 at 02:58:12PM -0700, Linus Torvalds wrote:
quoted
And I think our horrible "kernel threads return to user space when
done" is absolutely horrifically nasty. Maybe of the clever sort, but
mostly of the historical horror sort.
How would you prefer to handle that, then? Separate magical path from
kernel_execve() to switch to userland? We used to have something of
that sort, and that had been a real horror...
As it is, it's "kernel thread is spawned at the point similar to
ret_from_fork(), runs the payload (which almost never returns) and
then proceeds out to userland, same way fork(2) would've done."
That way kernel_execve() doesn't have to do anything magical.
Al, digging through the old notes and current call graph...
FWIW, the major assumption back then had been that get_signal(),
signal_delivered() and all associated machinery (including coredumps)
runs *only* from SIGPENDING/NOTIFY_SIGNAL handling.
And "has complete registers on stack" is only a part of that;
there was other fun stuff in the area ;-/ Do we want coredumps for
those, and if we do, will the de_thread stuff work there?
On Mon, Jun 21, 2021 at 6:55 AM Al Viro [off-list ref] wrote:
On Tue, Jun 15, 2021 at 02:58:12PM -0700, Linus Torvalds wrote:
quoted
And I think our horrible "kernel threads return to user space when
done" is absolutely horrifically nasty. Maybe of the clever sort, but
mostly of the historical horror sort.
How would you prefer to handle that, then? Separate magical path from
kernel_execve() to switch to userland? We used to have something of
that sort, and that had been a real horror...
Hmm. Maybe the alternatives would all be worse. The current thing is
clever, and shares the return path with the normal case. It's just
also a bit surprising, in that a kernel thread normally must not
return - with the magical exception of "if it had done a
kernel_execve() at some point, then returning is magically the way you
actually start user mode".
So it all feels very special, and there's not even a comment about it.
I think we only have two users of that thing (the very first 'init',
and user-mode-helpr), So I guess it doesn't really matter.
Linus
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2021-06-21 18:59:17
On Mon, Jun 21, 2021 at 01:54:56PM +0000, Al Viro wrote:
On Tue, Jun 15, 2021 at 02:58:12PM -0700, Linus Torvalds wrote:
quoted
And I think our horrible "kernel threads return to user space when
done" is absolutely horrifically nasty. Maybe of the clever sort, but
mostly of the historical horror sort.
How would you prefer to handle that, then? Separate magical path from
kernel_execve() to switch to userland? We used to have something of
that sort, and that had been a real horror...
As it is, it's "kernel thread is spawned at the point similar to
ret_from_fork(), runs the payload (which almost never returns) and
then proceeds out to userland, same way fork(2) would've done."
That way kernel_execve() doesn't have to do anything magical.
Al, digging through the old notes and current call graph...
There's a large mess around do_exit() - we have a bunch of
callers all over arch/*; if nothing else, I very much doubt that really
want to let tracer play with a thread in the middle of die_if_kernel()
or similar.
We sure as hell do not want to arrange for anything on the kernel
stack in such situations, no matter what's done in exit(2)...
On Mon, Jun 21, 2021 at 11:59 AM Al Viro [off-list ref] wrote:
There's a large mess around do_exit() - we have a bunch of
callers all over arch/*; if nothing else, I very much doubt that really
want to let tracer play with a thread in the middle of die_if_kernel()
or similar.
Right you are.
I'm really beginning to hate ptrace_{event,notify}() and those
PTRACE_EVENT_xyz things.
I don't even know what uses them, honestly. How very annoying.
I guess it's easy enough (famous last words) to move the
ptrace_event() call out of do_exit() and into the actual
exit/exit_group system calls, and the signal handling path. The paths
that actually have proper pt_regs.
Looks like sys_exit() and do_group_exit() would be the two places to
do it (do_group_exit() would handle the signal case and
sys_group_exit()).
Linus
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2021-06-21 19:24:34
On Mon, Jun 21, 2021 at 06:59:01PM +0000, Al Viro wrote:
On Mon, Jun 21, 2021 at 01:54:56PM +0000, Al Viro wrote:
quoted
On Tue, Jun 15, 2021 at 02:58:12PM -0700, Linus Torvalds wrote:
quoted
And I think our horrible "kernel threads return to user space when
done" is absolutely horrifically nasty. Maybe of the clever sort, but
mostly of the historical horror sort.
How would you prefer to handle that, then? Separate magical path from
kernel_execve() to switch to userland? We used to have something of
that sort, and that had been a real horror...
As it is, it's "kernel thread is spawned at the point similar to
ret_from_fork(), runs the payload (which almost never returns) and
then proceeds out to userland, same way fork(2) would've done."
That way kernel_execve() doesn't have to do anything magical.
Al, digging through the old notes and current call graph...
There's a large mess around do_exit() - we have a bunch of
callers all over arch/*; if nothing else, I very much doubt that really
want to let tracer play with a thread in the middle of die_if_kernel()
or similar.
We sure as hell do not want to arrange for anything on the kernel
stack in such situations, no matter what's done in exit(2)...
FWIW, on alpha it's die_if_kernel(), do_entUna() and do_page_fault(),
all in not-from-userland cases. On m68k - die_if_kernel(), do_page_fault()
(both for non-from-userland cases) and something really odd - fpsp040_die().
Exception handling for floating point stuff on 68040? Looks like it has
an open-coded copy_to_user()/copy_from_user(), with faults doing hard
do_exit(SIGSEGV) instead of raising a signal and trying to do something
sane...
I really don't want to try and figure out how painful would it be to
teach that code how to deal with faults - _testing_ anything in that
area sure as hell will be. IIRC, details of recovery from FPU exceptions
on 68040 in the manual left impression of a minefield...
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2021-06-21 19:45:56
On Mon, Jun 21, 2021 at 12:22:06PM -0700, Linus Torvalds wrote:
On Mon, Jun 21, 2021 at 11:59 AM Al Viro [off-list ref] wrote:
quoted
There's a large mess around do_exit() - we have a bunch of
callers all over arch/*; if nothing else, I very much doubt that really
want to let tracer play with a thread in the middle of die_if_kernel()
or similar.
Right you are.
I'm really beginning to hate ptrace_{event,notify}() and those
PTRACE_EVENT_xyz things.
I don't even know what uses them, honestly. How very annoying.
I guess it's easy enough (famous last words) to move the
ptrace_event() call out of do_exit() and into the actual
exit/exit_group system calls, and the signal handling path. The paths
that actually have proper pt_regs.
Looks like sys_exit() and do_group_exit() would be the two places to
do it (do_group_exit() would handle the signal case and
sys_group_exit()).
Maybe... I'm digging through that pile right now, will follow up when
I get a reasonably complete picture. In the meanwhile, do kernel/kthread.c
uses look even remotely sane? Intentional - sure, but it really looks
wrong to use thread exit code as communication channel there...
On Mon, Jun 21, 2021 at 12:45 PM Al Viro [off-list ref] wrote:
quoted
Looks like sys_exit() and do_group_exit() would be the two places to
do it (do_group_exit() would handle the signal case and
sys_group_exit()).
Maybe... I'm digging through that pile right now, will follow up when
I get a reasonably complete picture
We might have another possible way to solve this:
(a) make it the rule that everybody always saves the full (integer)
register set in pt_regs
(b) make m68k just always create that switch-stack for all system
calls (it's really not that big, I think it's like six words or
something)
(c) admit that alpha is broken, but nobody really cares
In the meanwhile, do kernel/kthread.c uses look even remotely sane?
Intentional - sure, but it really looks wrong to use thread exit code
as communication channel there...
I really doubt that it is even "intentional".
I think it's "use some errno as a random exit code" and nobody ever
really thought about it, or thought about how that doesn't really
work. People are used to the error numbers, not thinking about how
do_exit() doesn't take an error number, but a signal number (and an
8-bit positive error code in bits 8-15).
Because no, it's not even remotely sane.
I think the do_exit(-EINTR) could be do_exit(SIGINT) and it would make
more sense. And the -ENOMEM might be SIGBUS, perhaps.
It does look like the usermode-helper code does save the exit code
with things like
kernel_wait(pid, &sub_info->retval);
and I see call_usermodehelper_exec() doing
retval = sub_info->retval;
and treating it as an error code. But I think those have never been
tested with that (bogus) exit code thing from kernel_wait(), because
it wouldn't have worked. It has only ever been tested with the (real)
exit code things like
if (pid < 0) {
sub_info->retval = pid;
which does actually assign a negative error code to it.
So I think that
kernel_wait(pid, &sub_info->retval);
line is buggy, and should be something like
int wstatus;
kernel_wait(pid, &wstatus);
sub_info->retval = WEXITSTATUS(wstatus) ? -EINVAL : 0;
or something.
Linus
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2021-06-21 23:23:24
On Mon, Jun 21, 2021 at 04:14:36PM -0700, Linus Torvalds wrote:
On Mon, Jun 21, 2021 at 12:45 PM Al Viro [off-list ref] wrote:
quoted
quoted
Looks like sys_exit() and do_group_exit() would be the two places to
do it (do_group_exit() would handle the signal case and
sys_group_exit()).
Maybe... I'm digging through that pile right now, will follow up when
I get a reasonably complete picture
We might have another possible way to solve this:
(a) make it the rule that everybody always saves the full (integer)
register set in pt_regs
(b) make m68k just always create that switch-stack for all system
calls (it's really not that big, I think it's like six words or
something)
(c) admit that alpha is broken, but nobody really cares
How would it help e.g. oopsen on the way out of timer interrupts?
IMO we simply shouldn't allow ptrace access if the tracee is in that kind
of state, on any architecture...
From: Michael Schmitz <schmitzmic@gmail.com> Date: 2021-06-21 23:25:12
Hi Al,
On 22/06/21 7:24 am, Al Viro wrote:
quoted
There's a large mess around do_exit() - we have a bunch of
callers all over arch/*; if nothing else, I very much doubt that really
want to let tracer play with a thread in the middle of die_if_kernel()
or similar.
We sure as hell do not want to arrange for anything on the kernel
stack in such situations, no matter what's done in exit(2)...
FWIW, on alpha it's die_if_kernel(), do_entUna() and do_page_fault(),
all in not-from-userland cases. On m68k - die_if_kernel(), do_page_fault()
(both for non-from-userland cases) and something really odd - fpsp040_die().
Exception handling for floating point stuff on 68040? Looks like it has
Exception handling for emulated floating point instructions, really - exceptions happening when excecuting FPU instructions on hardware will do the normal exception processing.
an open-coded copy_to_user()/copy_from_user(), with faults doing hard
do_exit(SIGSEGV) instead of raising a signal and trying to do something
sane...
Yes, that's what it does. Not pretty ... though all that using m68k copy_to_user()/copy_from_user() would change is returning how many bytes could not copied. In contrast to the ifpsp060 code, we could not pass on that return status to callers of copyin/copyout in fpsp040, so I don't see what sane thing could be done if a fault happens.
(I'd expect the MMU would have raised a bus error and resolved the problem by a page fault if possible, before we ever get to this point?)
I really don't want to try and figure out how painful would it be to
teach that code how to deal with faults - _testing_ anything in that
area sure as hell will be. IIRC, details of recovery from FPU exceptions
on 68040 in the manual left impression of a minefield...
This is only about faults when moving data from/to user space. FPU exceptions are handled elsewhere in the code. So we at least don't have to deal with that particular minefield.
Teaching the fpsp040 code to deal with access faults looks horrible indeed... let's not go there.
Cheers,
Michael
On Mon, Jun 21, 2021 at 4:23 PM Al Viro [off-list ref] wrote:
How would it help e.g. oopsen on the way out of timer interrupts?
IMO we simply shouldn't allow ptrace access if the tracee is in that kind
of state, on any architecture...
Yeah no, we can't do the "wait for ptrace" when the exit is due to an
oops. Although honestly, we have other cases like that where do_exit()
isn't 100% robust if you kill something in an interrupt. Like all the
locks it leaves locked etc.
So do_exit() from a timer interrupt is going to cause problems
regardless. I agree it's probably a good idea to try to avoid causing
even more with the odd ptrace thing, but I don't think ptrace_event is
some really "fundamental" problem at that point - it's just one detail
among many many.
So I was more thinking of the debug patch for m68k to catch all the
_regular_ cases, and all the other random cases of ptrace_event() or
ptrace_notify().
Although maybe we've really caught them all. The exit case was clearly
missing, and the thread fork case was scrogged. There are patches for
the known problems. The patches I really don't like are the
verification ones to find any unknown ones..
Linus
From: Michael Schmitz <schmitzmic@gmail.com> Date: 2021-06-22 00:01:21
Hi Linus,
On 22/06/21 11:14 am, Linus Torvalds wrote:
On Mon, Jun 21, 2021 at 12:45 PM Al Viro [off-list ref] wrote:
quoted
quoted
Looks like sys_exit() and do_group_exit() would be the two places to
do it (do_group_exit() would handle the signal case and
sys_group_exit()).
Maybe... I'm digging through that pile right now, will follow up when
I get a reasonably complete picture
We might have another possible way to solve this:
(a) make it the rule that everybody always saves the full (integer)
register set in pt_regs
(b) make m68k just always create that switch-stack for all system
calls (it's really not that big, I think it's like six words or
something)
Correct - six words for registers, one for the return address. Probably still a win compared to setting and clearing flag bits all over the place in an attempt to catch any as yet undetected unsafe cases of ptrace_stop.
I'll have to see how much of a performance impact I can see (not that I can even remotely measure that accurately - it's more of a 'does it now feel real sluggish' thing).
Cheers,
Michael
(c) admit that alpha is broken, but nobody really cares
quoted
In the meanwhile, do kernel/kthread.c uses look even remotely sane?
Intentional - sure, but it really looks wrong to use thread exit code
as communication channel there...
I really doubt that it is even "intentional".
I think it's "use some errno as a random exit code" and nobody ever
really thought about it, or thought about how that doesn't really
work. People are used to the error numbers, not thinking about how
do_exit() doesn't take an error number, but a signal number (and an
8-bit positive error code in bits 8-15).
Because no, it's not even remotely sane.
I think the do_exit(-EINTR) could be do_exit(SIGINT) and it would make
more sense. And the -ENOMEM might be SIGBUS, perhaps.
It does look like the usermode-helper code does save the exit code
with things like
kernel_wait(pid, &sub_info->retval);
and I see call_usermodehelper_exec() doing
retval = sub_info->retval;
and treating it as an error code. But I think those have never been
tested with that (bogus) exit code thing from kernel_wait(), because
it wouldn't have worked. It has only ever been tested with the (real)
exit code things like
if (pid < 0) {
sub_info->retval = pid;
which does actually assign a negative error code to it.
So I think that
kernel_wait(pid, &sub_info->retval);
line is buggy, and should be something like
int wstatus;
kernel_wait(pid, &wstatus);
sub_info->retval = WEXITSTATUS(wstatus) ? -EINVAL : 0;
or something.
Linus
From: Michael Schmitz <schmitzmic@gmail.com> Date: 2021-06-22 20:04:25
Hi Linus,
On 22/06/21 11:14 am, Linus Torvalds wrote:
On Mon, Jun 21, 2021 at 12:45 PM Al Viro [off-list ref] wrote:
quoted
quoted
Looks like sys_exit() and do_group_exit() would be the two places to
do it (do_group_exit() would handle the signal case and
sys_group_exit()).
Maybe... I'm digging through that pile right now, will follow up when
I get a reasonably complete picture
We might have another possible way to solve this:
(a) make it the rule that everybody always saves the full (integer)
register set in pt_regs
(b) make m68k just always create that switch-stack for all system
calls (it's really not that big, I think it's like six words or
something)
Turns out that is harder than it looked at first glance (at least for me).
All syscalls that _do_ save the switch stack are currently called through wrappers which pull the syscall arguments out of the saved pt_regs on the stack (pushing the switch stack after the SAVE_ALL saved stuff buries the syscall arguments on the stack, see comment about m68k_clone(). We'd have to push the switch stack _first_ when entering system_call to leave the syscall arguments in place, but that will require further changes to the syscall exit path (currently shared with the interrupt exit path). Not to mention the register offset calculations in arch/m68k/kernel/ptrace.c, and perhaps a few other dependencies that don't come to mind immediately.
We have both pt_regs and switch_stack in uapi/asm/ptrace.h, but the ordering of the two is only mentioned in a comment. Can we reorder them on the stack, as long as we don't change the struct definitions proper?
This will take a little more time to work out and test - certainly not before the weekend. I'll send a corrected version of my debug patch before that.
Cheers,
Michael
(c) admit that alpha is broken, but nobody really cares
quoted
In the meanwhile, do kernel/kthread.c uses look even remotely sane?
Intentional - sure, but it really looks wrong to use thread exit code
as communication channel there...
I really doubt that it is even "intentional".
I think it's "use some errno as a random exit code" and nobody ever
really thought about it, or thought about how that doesn't really
work. People are used to the error numbers, not thinking about how
do_exit() doesn't take an error number, but a signal number (and an
8-bit positive error code in bits 8-15).
Because no, it's not even remotely sane.
I think the do_exit(-EINTR) could be do_exit(SIGINT) and it would make
more sense. And the -ENOMEM might be SIGBUS, perhaps.
It does look like the usermode-helper code does save the exit code
with things like
kernel_wait(pid, &sub_info->retval);
and I see call_usermodehelper_exec() doing
retval = sub_info->retval;
and treating it as an error code. But I think those have never been
tested with that (bogus) exit code thing from kernel_wait(), because
it wouldn't have worked. It has only ever been tested with the (real)
exit code things like
if (pid < 0) {
sub_info->retval = pid;
which does actually assign a negative error code to it.
So I think that
kernel_wait(pid, &sub_info->retval);
line is buggy, and should be something like
int wstatus;
kernel_wait(pid, &wstatus);
sub_info->retval = WEXITSTATUS(wstatus) ? -EINVAL : 0;
or something.
Linus
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2021-06-22 20:19:11
On Wed, Jun 23, 2021 at 08:04:11AM +1200, Michael Schmitz wrote:
All syscalls that _do_ save the switch stack are currently called through
wrappers which pull the syscall arguments out of the saved pt_regs on the
stack (pushing the switch stack after the SAVE_ALL saved stuff buries the
syscall arguments on the stack, see comment about m68k_clone(). We'd have to
push the switch stack _first_ when entering system_call to leave the syscall
arguments in place, but that will require further changes to the syscall
exit path (currently shared with the interrupt exit path). Not to mention
the register offset calculations in arch/m68k/kernel/ptrace.c, and perhaps a
few other dependencies that don't come to mind immediately.
We have both pt_regs and switch_stack in uapi/asm/ptrace.h, but the ordering
of the two is only mentioned in a comment. Can we reorder them on the stack,
as long as we don't change the struct definitions proper?
This will take a little more time to work out and test - certainly not
before the weekend. I'll send a corrected version of my debug patch before
that.
This is insane, *especially* on m68k where you have the mess with different
frame layouts and associated ->stkadj crap (see mangle_kernel_stack() for
the (very) full barfbag).
From: Michael Schmitz <schmitzmic@gmail.com> Date: 2021-06-22 21:57:39
Hi Al,
On 23/06/21 8:18 am, Al Viro wrote:
On Wed, Jun 23, 2021 at 08:04:11AM +1200, Michael Schmitz wrote:
quoted
All syscalls that _do_ save the switch stack are currently called through
wrappers which pull the syscall arguments out of the saved pt_regs on the
stack (pushing the switch stack after the SAVE_ALL saved stuff buries the
syscall arguments on the stack, see comment about m68k_clone(). We'd have to
push the switch stack _first_ when entering system_call to leave the syscall
arguments in place, but that will require further changes to the syscall
exit path (currently shared with the interrupt exit path). Not to mention
the register offset calculations in arch/m68k/kernel/ptrace.c, and perhaps a
few other dependencies that don't come to mind immediately.
We have both pt_regs and switch_stack in uapi/asm/ptrace.h, but the ordering
of the two is only mentioned in a comment. Can we reorder them on the stack,
as long as we don't change the struct definitions proper?
This will take a little more time to work out and test - certainly not
before the weekend. I'll send a corrected version of my debug patch before
that.
This is insane, *especially* on m68k where you have the mess with different
frame layouts and associated ->stkadj crap (see mangle_kernel_stack() for
the (very) full barfbag).
Indeed - that's one of the uses of pt_regs and switch_stack that I hadn't yet seen.
So it's either leave the stack layout in system calls unchanged (aside from the ones that need the extra registers) and protect against accidental misuse of registers that weren't saved, with the overhead of playing with thread_info->status bits, or tackle the mess of redoing the stack layout to save all registers, always (did I already mention that I'd need a _lot_ of help from someone more conversant with m68k assembly coding for that option?).
Which one of these two barf bags is the fuller one?
Cheers,
Michael