[PATCH v3 07/13] x86/um: nommu: process/thread handling

[Johannes Berg]

On Tue, 2024-12-03 at 13:23 +0900, Hajime Tazaki wrote:
> 
> +++ b/arch/um/kernel/process.c
> @@ -117,13 +117,17 @@ void new_thread_handler(void)
>  	 * callback returns only if the kernel thread execs a process
>  	 */
>  	fn(arg);
> +#ifndef CONFIG_MMU
> +	arch_switch_to(current);
> +#endif
>  	userspace(&current->thread.regs.regs);

that doesn't make sense, arch_switch_to() does nothing anyway on 64-bit

>  /* Called magically, see new_thread_handler above */
>  static void fork_handler(void)
>  {
> -	schedule_tail(current->thread.prev_sched);
> +	if (current->thread.prev_sched)
> +		schedule_tail(current->thread.prev_sched);

Why is that NULL on nommu?

> @@ -134,6 +138,33 @@ static void fork_handler(void)
>  
>  	current->thread.prev_sched = NULL;
>  
> +#ifndef CONFIG_MMU

again, don't sprinkle ifdefs around the C code files - make inlines in a
header file or something

> +	/*
> +	 * child of vfork(2) comes here.
> +	 * clone(2) also enters here but doesn't need to advance the %rsp.
> +	 *
> +	 * This fork can only come from libc's vfork, which
> +	 * does this:
> +	 *	popq %%rdx;
> +	 *	call *%rax; // zpoline => __kernel_vsyscall
> +	 *	pushq %%rdx;

or maybe not zpoline ... so maybe need to update this

> +++ b/arch/um/os-Linux/skas/process.c
> @@ -144,6 +144,7 @@ void wait_stub_done(int pid)
>  
>  extern unsigned long current_stub_stack(void);
>  
> +#ifdef CONFIG_MMU

I'll stop commenting on ifdef sprinkling :)

> +++ b/arch/x86/um/asm/processor.h
> @@ -38,6 +38,18 @@ static __always_inline void cpu_relax(void)
>  
>  #define task_pt_regs(t) (&(t)->thread.regs)
>  
> +#ifndef CONFIG_MMU
> +#define task_top_of_stack(task) \
> +({									\
> +	unsigned long __ptr = (unsigned long)task->stack;	\
> +	__ptr += THREAD_SIZE;			\
> +	__ptr;					\
> +})
> +
> +extern long current_top_of_stack;
> +extern long current_ptregs;
> +#endif

no need for "extern".

you only use all that once, does it need to be here?

> +
>  #include <asm/processor-generic.h>
>  
>  #endif
> diff --git a/arch/x86/um/do_syscall_64.c b/arch/x86/um/do_syscall_64.c
> index 5d0fa83e7fdc..ca468caff729 100644
> --- a/arch/x86/um/do_syscall_64.c
> +++ b/arch/x86/um/do_syscall_64.c
> @@ -1,14 +1,43 @@
>  // SPDX-License-Identifier: GPL-2.0
>  
> +//#define DEBUG 1

please remove

>  #include <linux/kernel.h>
>  #include <linux/ptrace.h>
>  #include <kern_util.h>
>  #include <sysdep/syscalls.h>
>  #include <os.h>
>  
> +/*
> + * save/restore the return address stored in the stack, as the child overwrites
> + * the contents after returning to userspace (i.e., by push %rdx).
> + *
> + * see the detail in fork_handler().
> + */
> +static void *vfork_save_stack(void)
> +{
> +	unsigned char *stack_copy;
> +
> +	stack_copy = kzalloc(8, GFP_KERNEL);

Using a heap allocation to track 8 bytes, when the pointer to the
allocation is already 8 bytes (you're on 64-bit) seems ... rather
wasteful?

I also don't see you ever free it? Restore probably should, but anyway,
it shouldn't exist.

johannes