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(¤t->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
