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

[Hajime Tazaki <thehajime@gmail.com>]

On Thu, 05 Dec 2024 01:50:07 +0900,
Johannes Berg wrote:
> 
> 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

makes sense.  will fix it.

I added fs register record code to arch_switch_to() for nommu as we
don't use ptrace so, arch_switch_to() does the job in 64bit, but for
the kernel thread, we don't have to so, will remove it.

> >  /* 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?

During the past series, the pointer was sometimes NULL on random
conditions, but I couldn't reproduce it anymore..

I'll revert it until I could reproduce it.

> > @@ -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

will revert it.

> > +	/*
> > +	 * 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

will do it.

> > +++ 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 :)

ditto.

> > +++ 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?

sorry, I don't understand both of these comments; could you care to
elaborate ?

> > +
> >  #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

yes, will do it.

> > +/*
> > + * 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.

oops, my bad...
indeed the memory is never freed.

I'll update this part by not using heap allocation, but instead with
a variable.

-- Hajime