Skip to content

Conversation

@pussuw
Copy link
Contributor

@pussuw pussuw commented Jul 31, 2024

Summary

When executing fork() via a system call, the parent's stack gets corrupted
by the child, as during exception return the child loads the parent's
stack pointer from the context save area.

This happens because the full parent stack (including what has been pushed
during the system call) is copied to the child. What should be copied, is
only the user stack of the parent (the kernel stack is not interesting).

Fix this by only copying the parent's user stack to the child; and make
the child return directly to userspace (not via dispatch_syscall).

Impact

Fixes fork() when LIB_SYSCALL=y

Testing

rv-virt:pnsh64

@pussuw
Copy link
Contributor Author

pussuw commented Jul 31, 2024

Forgot to save FPU, fill fix it asap

@pussuw
Copy link
Contributor Author

pussuw commented Jul 31, 2024

This is now tested to work in protected mode, just like before, and SHOULD work with kernel stack as well, but could not test it yet.

I will still test it but vfork() has some kind of a bug for kernel mode which prevents doing this. I'll look into it tomorrow.

@yf13
Copy link
Contributor

yf13 commented Aug 1, 2024

I tried this locally but vfork_test() got illegal instruction exception:

user_main: vfork() test
riscv_exception: EXCEPTION: Illegal instruction. MCAUSE: 00000002, EPC: 80203a40, MTVAL: 00000000
riscv_exception: PANIC!!! Exception = 00000002

@pussuw
Copy link
Contributor Author

pussuw commented Aug 1, 2024

Yes I noticed late last night there is an error; the child returns to userspace with kernel privileges. I did a fix but did not have time to push.

FPU state is also not copied to the child. The patch is incomplete.

@pussuw pussuw force-pushed the rv_fix_fork_syscall branch from f8fd331 to e8c9576 Compare August 1, 2024 07:33
@pussuw
Copy link
Contributor Author

pussuw commented Aug 1, 2024

@yf13 thanks for testing, I fixed the aforementioned issues; the child now returns to user space with user privileges and FPU registers are also copied to the child.

@yf13
Copy link
Contributor

yf13 commented Aug 2, 2024

@pussuw, thanks for the updates, it works here. Also I added two in-place comemnts.

@pussuw
Copy link
Contributor Author

pussuw commented Aug 2, 2024

@pussuw, thanks for the updates, it works here. Also I added two in-place comemnts.

Nice! Your comments are not visible to me, maybe you need to "submit review" ?
"Files changed" -> "Review changes" -> "Submit review" ?

@yf13
Copy link
Contributor

yf13 commented Aug 2, 2024

@pussuw, thanks for teaching, I submitted the reviews. Please see if they are visibile now? Sorry for inconveninces as I never added in-place comments before.

@pussuw pussuw force-pushed the rv_fix_fork_syscall branch 2 times, most recently from bc74ffb to fde44de Compare August 2, 2024 12:07
@yf13
Copy link
Contributor

yf13 commented Aug 4, 2024

Seems that the warning with maix-bit/knsh_smp is gone for the riscv_cpustart.c in master branch, so we are not too far to pass the CI checks.

@xiaoxiang781216
Copy link
Contributor

@pussuw let's rebase to the latest.master.

pussuw added 3 commits August 5, 2024 12:28
The per CPU scratch register is needed by system calls -> enable it by
default.
Simplifies the implementation of dispatch_syscall, making it easier to
understand and maintain. Let the C-compiler do most of the work, instead
of doing everything as inline assembly.
When executing fork() via a system call, the parent's stack gets corrupted
by the child, as during exception return the child loads the parent's
stack pointer from the context save area.

This happens because the full parent stack (including what has been pushed
during the system call) is copied to the child. What should be copied, is
only the user stack of the parent (the kernel stack is not interesting).

Fix this by only copying the parent's user stack to the child; and make
the child return directly to userspace (not via dispatch_syscall).
@pussuw pussuw force-pushed the rv_fix_fork_syscall branch from fde44de to aeb7447 Compare August 5, 2024 09:29
@pkarashchenko
Copy link
Contributor

Seems like some defconfigs need to be normalized after adding new Kconfig dependency

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants