| Message ID | 20210713234710.ba0da02a609f.I56390429bc78e79e859e374183370c6311535786@changeid |
|---|---|
| State | Accepted |
| Headers | show |
| Series | um: fix stub location calculation | expand |
On Tue, Jul 13, 2021 at 11:47 PM Johannes Berg <johannes@sipsolutions.net> wrote: > > From: Johannes Berg <johannes.berg@intel.com> > > In commit 9f0b4807a44f ("um: rework userspace stubs to not hard-code > stub location") I changed stub_segv_handler() to do a calculation with > a pointer to a stack variable to find the data page that we're using > for the stack and the rest of the data. This same commit was meant to > do it as well for stub_clone_handler(), but the change inadvertently > went into commit 84b2789d6115 ("um: separate child and parent errors > in clone stub") instead. > > This was reported to not be compiled correctly by gcc 5, causing the > code to crash here. I'm not sure why, perhaps it's UB because the var > isn't initialized? In any case, this trick always seemed bad, so just > create a new inline function that does the calculation in assembly. My best guess is that gcc 5 sees only local modifications, but no further reads. So it treats it as dead store. > Reported-by: subashab@codeaurora.org > Fixes: 9f0b4807a44f ("um: rework userspace stubs to not hard-code stub location") > Fixes: 84b2789d6115 ("um: separate child and parent errors in clone stub") > Signed-off-by: Johannes Berg <johannes.berg@intel.com> BTW: Marking data/f as volatile fixes the problem too. That way gcc no longer optimizes data/f away. diff --git a/arch/um/kernel/skas/clone.c b/arch/um/kernel/skas/clone.c index 592cdb1..6331941 100644 --- a/arch/um/kernel/skas/clone.c +++ b/arch/um/kernel/skas/clone.c @@ -25,7 +25,7 @@ void __attribute__ ((__section__ (".__syscall_stub"))) stub_clone_handler(void) { int stack; - struct stub_data *data = (void *) ((unsigned long)&stack & ~(UM_KERN_PAGE_SIZE - 1)); + volatile struct stub_data *data = (void *) ((unsigned long)&stack & ~(UM_KERN_PAGE_SIZE - 1)); long err; err = stub_syscall2(__NR_clone, CLONE_PARENT | CLONE_FILES | SIGCHLD, diff --git a/arch/x86/um/stub_segv.c b/arch/x86/um/stub_segv.c index 21836ea..87c3aef 100644 --- a/arch/x86/um/stub_segv.c +++ b/arch/x86/um/stub_segv.c @@ -13,7 +13,7 @@ stub_segv_handler(int sig, siginfo_t *info, void *p) { int stack; ucontext_t *uc = p; - struct faultinfo *f = (void *)(((unsigned long)&stack) & ~(UM_KERN_PAGE_SIZE - 1)); + volatile struct faultinfo *f = (void *)(((unsigned long)&stack) & ~(UM_KERN_PAGE_SIZE - 1)); GET_FAULTINFO_FROM_MC(*f, &uc->uc_mcontext); trap_myself();
On 2021-07-13 16:11, Richard Weinberger wrote: > On Tue, Jul 13, 2021 at 11:47 PM Johannes Berg > <johannes@sipsolutions.net> wrote: >> >> From: Johannes Berg <johannes.berg@intel.com> >> >> In commit 9f0b4807a44f ("um: rework userspace stubs to not hard-code >> stub location") I changed stub_segv_handler() to do a calculation with >> a pointer to a stack variable to find the data page that we're using >> for the stack and the rest of the data. This same commit was meant to >> do it as well for stub_clone_handler(), but the change inadvertently >> went into commit 84b2789d6115 ("um: separate child and parent errors >> in clone stub") instead. >> >> This was reported to not be compiled correctly by gcc 5, causing the >> code to crash here. I'm not sure why, perhaps it's UB because the var >> isn't initialized? In any case, this trick always seemed bad, so just >> create a new inline function that does the calculation in assembly. > > My best guess is that gcc 5 sees only local modifications, but no > further reads. > So it treats it as dead store. > >> Reported-by: subashab@codeaurora.org >> Fixes: 9f0b4807a44f ("um: rework userspace stubs to not hard-code stub >> location") >> Fixes: 84b2789d6115 ("um: separate child and parent errors in clone >> stub") >> Signed-off-by: Johannes Berg <johannes.berg@intel.com> > > BTW: Marking data/f as volatile fixes the problem too. > That way gcc no longer optimizes data/f away. > > diff --git a/arch/um/kernel/skas/clone.c b/arch/um/kernel/skas/clone.c > index 592cdb1..6331941 100644 > --- a/arch/um/kernel/skas/clone.c > +++ b/arch/um/kernel/skas/clone.c > @@ -25,7 +25,7 @@ void __attribute__ ((__section__ > (".__syscall_stub"))) > stub_clone_handler(void) > { > int stack; > - struct stub_data *data = (void *) ((unsigned long)&stack & > ~(UM_KERN_PAGE_SIZE - 1)); > + volatile struct stub_data *data = (void *) ((unsigned > long)&stack & ~(UM_KERN_PAGE_SIZE - 1)); > long err; > > err = stub_syscall2(__NR_clone, CLONE_PARENT | CLONE_FILES | > SIGCHLD, > diff --git a/arch/x86/um/stub_segv.c b/arch/x86/um/stub_segv.c > index 21836ea..87c3aef 100644 > --- a/arch/x86/um/stub_segv.c > +++ b/arch/x86/um/stub_segv.c > @@ -13,7 +13,7 @@ stub_segv_handler(int sig, siginfo_t *info, void *p) > { > int stack; > ucontext_t *uc = p; > - struct faultinfo *f = (void *)(((unsigned long)&stack) & > ~(UM_KERN_PAGE_SIZE - 1)); > + volatile struct faultinfo *f = (void *)(((unsigned > long)&stack) & ~(UM_KERN_PAGE_SIZE - 1)); > > GET_FAULTINFO_FROM_MC(*f, &uc->uc_mcontext); > trap_myself(); Both of these work for me. Thanks for the help. Reported-and-tested-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
On Tue, Jul 13, 2021 at 5:11 PM Richard Weinberger <richard.weinberger@gmail.com> wrote: > > On Tue, Jul 13, 2021 at 11:47 PM Johannes Berg > <johannes@sipsolutions.net> wrote: > > > > From: Johannes Berg <johannes.berg@intel.com> > > > > In commit 9f0b4807a44f ("um: rework userspace stubs to not hard-code > > stub location") I changed stub_segv_handler() to do a calculation with > > a pointer to a stack variable to find the data page that we're using > > for the stack and the rest of the data. This same commit was meant to > > do it as well for stub_clone_handler(), but the change inadvertently > > went into commit 84b2789d6115 ("um: separate child and parent errors > > in clone stub") instead. > > > > This was reported to not be compiled correctly by gcc 5, causing the > > code to crash here. I'm not sure why, perhaps it's UB because the var > > isn't initialized? In any case, this trick always seemed bad, so just > > create a new inline function that does the calculation in assembly. > > My best guess is that gcc 5 sees only local modifications, but no further reads. > So it treats it as dead store. > > > Reported-by: subashab@codeaurora.org > > Fixes: 9f0b4807a44f ("um: rework userspace stubs to not hard-code stub location") > > Fixes: 84b2789d6115 ("um: separate child and parent errors in clone stub") > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > > BTW: Marking data/f as volatile fixes the problem too. > That way gcc no longer optimizes data/f away. Yeah, in the bad compilation, (gdb) disas stub_segv_handler Dump of assembler code for function stub_segv_handler: 0x000000006033d0c5 <+0>: push %rbp 0x000000006033d0c6 <+1>: mov %rsp,%rbp 0x000000006033d0c9 <+4>: int3 0x000000006033d0ca <+5>: pop %rbp 0x000000006033d0cb <+6>: retq End of assembler dump. The store is optimized away -> the faultinfo is unmodified -> the segv handler treats the fault as unfixable -> init dead. > > diff --git a/arch/um/kernel/skas/clone.c b/arch/um/kernel/skas/clone.c > index 592cdb1..6331941 100644 > --- a/arch/um/kernel/skas/clone.c > +++ b/arch/um/kernel/skas/clone.c > @@ -25,7 +25,7 @@ void __attribute__ ((__section__ (".__syscall_stub"))) > stub_clone_handler(void) > { > int stack; > - struct stub_data *data = (void *) ((unsigned long)&stack & > ~(UM_KERN_PAGE_SIZE - 1)); > + volatile struct stub_data *data = (void *) ((unsigned > long)&stack & ~(UM_KERN_PAGE_SIZE - 1)); > long err; > > err = stub_syscall2(__NR_clone, CLONE_PARENT | CLONE_FILES | SIGCHLD, > diff --git a/arch/x86/um/stub_segv.c b/arch/x86/um/stub_segv.c > index 21836ea..87c3aef 100644 > --- a/arch/x86/um/stub_segv.c > +++ b/arch/x86/um/stub_segv.c > @@ -13,7 +13,7 @@ stub_segv_handler(int sig, siginfo_t *info, void *p) > { > int stack; > ucontext_t *uc = p; > - struct faultinfo *f = (void *)(((unsigned long)&stack) & > ~(UM_KERN_PAGE_SIZE - 1)); > + volatile struct faultinfo *f = (void *)(((unsigned > long)&stack) & ~(UM_KERN_PAGE_SIZE - 1)); > > GET_FAULTINFO_FROM_MC(*f, &uc->uc_mcontext); > trap_myself(); > > _______________________________________________ > linux-um mailing list > linux-um@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-um
On Tue, 2021-07-13 at 17:44 -0500, YiFei Zhu wrote: > > The store is optimized away -> the faultinfo is unmodified -> the segv > handler treats the fault as unfixable -> init dead. Yeah. I was confused about this in the clone case, but even there of course the store is important because that's the child's PID in the parent ... I somehow looked at the code last night and thought it was only a broken error case. I'll leave it to Richard to figure out which of the two things to apply, I guess. And it should probably come with a Cc: stable, which I forgot on my patch. johannes
diff --git a/arch/um/kernel/skas/clone.c b/arch/um/kernel/skas/clone.c index 592cdb138441..c0d2d592b31b 100644 --- a/arch/um/kernel/skas/clone.c +++ b/arch/um/kernel/skas/clone.c @@ -24,8 +24,7 @@ void __attribute__ ((__section__ (".__syscall_stub"))) stub_clone_handler(void) { - int stack; - struct stub_data *data = (void *) ((unsigned long)&stack & ~(UM_KERN_PAGE_SIZE - 1)); + struct stub_data *data = get_stub_page(); long err; err = stub_syscall2(__NR_clone, CLONE_PARENT | CLONE_FILES | SIGCHLD, diff --git a/arch/x86/um/shared/sysdep/stub_32.h b/arch/x86/um/shared/sysdep/stub_32.h index b95db9daf0e8..4c6c2be0c899 100644 --- a/arch/x86/um/shared/sysdep/stub_32.h +++ b/arch/x86/um/shared/sysdep/stub_32.h @@ -101,4 +101,16 @@ static inline void remap_stack_and_trap(void) "memory"); } +static __always_inline void *get_stub_page(void) +{ + unsigned long ret; + + asm volatile ( + "movl %%esp,%0 ;" + "andl %1,%0" + : "=a" (ret) + : "g" (~(UM_KERN_PAGE_SIZE - 1))); + + return (void *)ret; +} #endif diff --git a/arch/x86/um/shared/sysdep/stub_64.h b/arch/x86/um/shared/sysdep/stub_64.h index 6e2626b77a2e..e9c4b2b38803 100644 --- a/arch/x86/um/shared/sysdep/stub_64.h +++ b/arch/x86/um/shared/sysdep/stub_64.h @@ -108,4 +108,16 @@ static inline void remap_stack_and_trap(void) __syscall_clobber, "r10", "r8", "r9"); } +static __always_inline void *get_stub_page(void) +{ + unsigned long ret; + + asm volatile ( + "movq %%rsp,%0 ;" + "andq %1,%0" + : "=a" (ret) + : "g" (~(UM_KERN_PAGE_SIZE - 1))); + + return (void *)ret; +} #endif diff --git a/arch/x86/um/stub_segv.c b/arch/x86/um/stub_segv.c index 21836eaf1725..f7eefba034f9 100644 --- a/arch/x86/um/stub_segv.c +++ b/arch/x86/um/stub_segv.c @@ -11,9 +11,8 @@ void __attribute__ ((__section__ (".__syscall_stub"))) stub_segv_handler(int sig, siginfo_t *info, void *p) { - int stack; + struct faultinfo *f = get_stub_page(); ucontext_t *uc = p; - struct faultinfo *f = (void *)(((unsigned long)&stack) & ~(UM_KERN_PAGE_SIZE - 1)); GET_FAULTINFO_FROM_MC(*f, &uc->uc_mcontext); trap_myself();