* Re: [PATCH] LoongArch: Store syscall nr in thread_info [not found] ` <CAEyhmHRYghT5iFiLByUmC=AjdygiBWU8TH3joSyyWibu0Ki2xw@mail.gmail.com> @ 2023-11-22 7:58 ` Huacai Chen 2023-11-23 5:49 ` Hengqi Chen 0 siblings, 1 reply; 10+ messages in thread From: Huacai Chen @ 2023-11-22 7:58 UTC (permalink / raw) To: Hengqi Chen, Arnd Bergmann, linux-arch; +Cc: loongarch, kernel Hi, Hengqi, On Wed, Nov 22, 2023 at 3:34 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > Hi, Huacai, > > On Wed, Nov 22, 2023 at 2:32 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > Hi, Hengqi, > > > > On Wed, Nov 22, 2023 at 1:14 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > Currently, we store syscall number in pt_regs::regs[11] and it may be > > > changed during syscall execution. Take `execve` as an example: > > > > > > sys_execve > > > -> do_execve > > > -> do_execveat_common > > > -> bprm_execve > > > -> exec_binprm > > > -> search_binary_handler > > > -> load_elf_binary > > > -> ELF_PLAT_INIT > > > > > > ELF_PLAT_INIT reset regs[11] to 0, later in syscall_exit_to_user_mode > > > we get a wrong syscall nr. > > > > > > Known affected syscalls includes execve/execveat/rt_sigreturn. Tools > > > like execsnoop do not work properly because the sys_exit_* tracepoints > > > does not trigger at all. > > > > > > Let's store syscall nr in thread_info instead. > > Can we just modify ELF_PLAT_INIT and not clear regs[11]? > > > > I am uncertain about the side effects of changing ELF_PLAT_INIT. > From a completeness perspective, changing ELF_PLAT_INIT is suboptimal, > rt_sigreturn is affected in another code path, and there may be other > syscalls that I am unaware of. Save syscall number in thread_info has more side effects, because ptrace allows us to change the number during syscall, then we should keep consistency between syscall and regs[11]. And about ELF_PLAT_INIT, maybe Arnd can give us some more information. Hi, Arnd, I found some new architectures, such as ARM64 and RISC-V, just do nearly nothing in ELF_PLAT_INIT, while some old architectures, such as x86 and MIPS, clear most of the registers, do you know why? Huacai > > > Huacai > > > > > > > > Fixes: be769645a2aef ("LoongArch: Add system call support") > > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > > > --- > > > arch/loongarch/include/asm/syscall.h | 2 +- > > > arch/loongarch/kernel/syscall.c | 1 + > > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/loongarch/include/asm/syscall.h b/arch/loongarch/include/asm/syscall.h > > > index e286dc58476e..2317d674b92a 100644 > > > --- a/arch/loongarch/include/asm/syscall.h > > > +++ b/arch/loongarch/include/asm/syscall.h > > > @@ -23,7 +23,7 @@ extern void *sys_call_table[]; > > > static inline long syscall_get_nr(struct task_struct *task, > > > struct pt_regs *regs) > > > { > > > - return regs->regs[11]; > > > + return task_thread_info(task)->syscall; > > > } > > > > > > static inline void syscall_rollback(struct task_struct *task, > > > diff --git a/arch/loongarch/kernel/syscall.c b/arch/loongarch/kernel/syscall.c > > > index b4c5acd7aa3b..2783e33cf276 100644 > > > --- a/arch/loongarch/kernel/syscall.c > > > +++ b/arch/loongarch/kernel/syscall.c > > > @@ -52,6 +52,7 @@ void noinstr do_syscall(struct pt_regs *regs) > > > regs->orig_a0 = regs->regs[4]; > > > regs->regs[4] = -ENOSYS; > > > > > > + task_thread_info(current)->syscall = nr; > > > nr = syscall_enter_from_user_mode(regs, nr); > > > > > > if (nr < NR_syscalls) { > > > -- > > > 2.42.0 > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] LoongArch: Store syscall nr in thread_info 2023-11-22 7:58 ` [PATCH] LoongArch: Store syscall nr in thread_info Huacai Chen @ 2023-11-23 5:49 ` Hengqi Chen 2023-11-23 6:13 ` Huacai Chen 0 siblings, 1 reply; 10+ messages in thread From: Hengqi Chen @ 2023-11-23 5:49 UTC (permalink / raw) To: Huacai Chen; +Cc: Arnd Bergmann, linux-arch, loongarch, kernel On Wed, Nov 22, 2023 at 3:58 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > Hi, Hengqi, > > On Wed, Nov 22, 2023 at 3:34 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > Hi, Huacai, > > > > On Wed, Nov 22, 2023 at 2:32 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > Hi, Hengqi, > > > > > > On Wed, Nov 22, 2023 at 1:14 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > Currently, we store syscall number in pt_regs::regs[11] and it may be > > > > changed during syscall execution. Take `execve` as an example: > > > > > > > > sys_execve > > > > -> do_execve > > > > -> do_execveat_common > > > > -> bprm_execve > > > > -> exec_binprm > > > > -> search_binary_handler > > > > -> load_elf_binary > > > > -> ELF_PLAT_INIT > > > > > > > > ELF_PLAT_INIT reset regs[11] to 0, later in syscall_exit_to_user_mode > > > > we get a wrong syscall nr. > > > > > > > > Known affected syscalls includes execve/execveat/rt_sigreturn. Tools > > > > like execsnoop do not work properly because the sys_exit_* tracepoints > > > > does not trigger at all. > > > > > > > > Let's store syscall nr in thread_info instead. > > > Can we just modify ELF_PLAT_INIT and not clear regs[11]? > > > > > > > I am uncertain about the side effects of changing ELF_PLAT_INIT. > > From a completeness perspective, changing ELF_PLAT_INIT is suboptimal, > > rt_sigreturn is affected in another code path, and there may be other > > syscalls that I am unaware of. > Save syscall number in thread_info has more side effects, because > ptrace allows us to change the number during syscall, then we should > keep consistency between syscall and regs[11]. > How about the change below: diff --git a/arch/loongarch/include/asm/syscall.h b/arch/loongarch/include/asm/syscall.h index e286dc58476e..954ba53bcc9a 100644 --- a/arch/loongarch/include/asm/syscall.h +++ b/arch/loongarch/include/asm/syscall.h @@ -23,7 +23,9 @@ extern void *sys_call_table[]; static inline long syscall_get_nr(struct task_struct *task, struct pt_regs *regs) { - return regs->regs[11]; + long nr = task_thread_info(task)->syscall; + + return nr ? : regs->regs[11]; } static inline void syscall_rollback(struct task_struct *task, diff --git a/arch/loongarch/kernel/syscall.c b/arch/loongarch/kernel/syscall.c index b4c5acd7aa3b..553ab0d624cb 100644 --- a/arch/loongarch/kernel/syscall.c +++ b/arch/loongarch/kernel/syscall.c @@ -53,6 +53,7 @@ void noinstr do_syscall(struct pt_regs *regs) regs->regs[4] = -ENOSYS; nr = syscall_enter_from_user_mode(regs, nr); + current_thread_info()->syscall = nr; if (nr < NR_syscalls) { syscall_fn = sys_call_table[nr]; @@ -61,4 +62,5 @@ void noinstr do_syscall(struct pt_regs *regs) } syscall_exit_to_user_mode(regs); + current_thread_info()->syscall = 0; } * allow ptrace to change syscall nr * sys_exit_* will also see the right syscall nr * this works even if rt_sigreturn clobbers all pt_regs::regs > And about ELF_PLAT_INIT, maybe Arnd can give us some more information. > > Hi, Arnd, > > I found some new architectures, such as ARM64 and RISC-V, just do > nearly nothing in ELF_PLAT_INIT, while some old architectures, such as > x86 and MIPS, clear most of the registers, do you know why? > > Huacai > > > > > > Huacai > > > > > > > > > > > Fixes: be769645a2aef ("LoongArch: Add system call support") > > > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > > > > --- > > > > arch/loongarch/include/asm/syscall.h | 2 +- > > > > arch/loongarch/kernel/syscall.c | 1 + > > > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/arch/loongarch/include/asm/syscall.h b/arch/loongarch/include/asm/syscall.h > > > > index e286dc58476e..2317d674b92a 100644 > > > > --- a/arch/loongarch/include/asm/syscall.h > > > > +++ b/arch/loongarch/include/asm/syscall.h > > > > @@ -23,7 +23,7 @@ extern void *sys_call_table[]; > > > > static inline long syscall_get_nr(struct task_struct *task, > > > > struct pt_regs *regs) > > > > { > > > > - return regs->regs[11]; > > > > + return task_thread_info(task)->syscall; > > > > } > > > > > > > > static inline void syscall_rollback(struct task_struct *task, > > > > diff --git a/arch/loongarch/kernel/syscall.c b/arch/loongarch/kernel/syscall.c > > > > index b4c5acd7aa3b..2783e33cf276 100644 > > > > --- a/arch/loongarch/kernel/syscall.c > > > > +++ b/arch/loongarch/kernel/syscall.c > > > > @@ -52,6 +52,7 @@ void noinstr do_syscall(struct pt_regs *regs) > > > > regs->orig_a0 = regs->regs[4]; > > > > regs->regs[4] = -ENOSYS; > > > > > > > > + task_thread_info(current)->syscall = nr; > > > > nr = syscall_enter_from_user_mode(regs, nr); > > > > > > > > if (nr < NR_syscalls) { > > > > -- > > > > 2.42.0 > > > > ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] LoongArch: Store syscall nr in thread_info 2023-11-23 5:49 ` Hengqi Chen @ 2023-11-23 6:13 ` Huacai Chen 2023-11-23 8:08 ` Hengqi Chen 0 siblings, 1 reply; 10+ messages in thread From: Huacai Chen @ 2023-11-23 6:13 UTC (permalink / raw) To: Hengqi Chen; +Cc: Arnd Bergmann, linux-arch, loongarch, kernel Hi, Hengqi, On Thu, Nov 23, 2023 at 1:49 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > On Wed, Nov 22, 2023 at 3:58 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > Hi, Hengqi, > > > > On Wed, Nov 22, 2023 at 3:34 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > Hi, Huacai, > > > > > > On Wed, Nov 22, 2023 at 2:32 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > > > Hi, Hengqi, > > > > > > > > On Wed, Nov 22, 2023 at 1:14 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > > > Currently, we store syscall number in pt_regs::regs[11] and it may be > > > > > changed during syscall execution. Take `execve` as an example: > > > > > > > > > > sys_execve > > > > > -> do_execve > > > > > -> do_execveat_common > > > > > -> bprm_execve > > > > > -> exec_binprm > > > > > -> search_binary_handler > > > > > -> load_elf_binary > > > > > -> ELF_PLAT_INIT > > > > > > > > > > ELF_PLAT_INIT reset regs[11] to 0, later in syscall_exit_to_user_mode > > > > > we get a wrong syscall nr. > > > > > > > > > > Known affected syscalls includes execve/execveat/rt_sigreturn. Tools > > > > > like execsnoop do not work properly because the sys_exit_* tracepoints > > > > > does not trigger at all. > > > > > > > > > > Let's store syscall nr in thread_info instead. > > > > Can we just modify ELF_PLAT_INIT and not clear regs[11]? > > > > > > > > > > I am uncertain about the side effects of changing ELF_PLAT_INIT. > > > From a completeness perspective, changing ELF_PLAT_INIT is suboptimal, > > > rt_sigreturn is affected in another code path, and there may be other > > > syscalls that I am unaware of. > > Save syscall number in thread_info has more side effects, because > > ptrace allows us to change the number during syscall, then we should > > keep consistency between syscall and regs[11]. > > > > How about the change below: > > diff --git a/arch/loongarch/include/asm/syscall.h > b/arch/loongarch/include/asm/syscall.h > index e286dc58476e..954ba53bcc9a 100644 > --- a/arch/loongarch/include/asm/syscall.h > +++ b/arch/loongarch/include/asm/syscall.h > @@ -23,7 +23,9 @@ extern void *sys_call_table[]; > static inline long syscall_get_nr(struct task_struct *task, > struct pt_regs *regs) > { > - return regs->regs[11]; > + long nr = task_thread_info(task)->syscall; > + > + return nr ? : regs->regs[11]; > } > > static inline void syscall_rollback(struct task_struct *task, > diff --git a/arch/loongarch/kernel/syscall.c b/arch/loongarch/kernel/syscall.c > index b4c5acd7aa3b..553ab0d624cb 100644 > --- a/arch/loongarch/kernel/syscall.c > +++ b/arch/loongarch/kernel/syscall.c > @@ -53,6 +53,7 @@ void noinstr do_syscall(struct pt_regs *regs) > regs->regs[4] = -ENOSYS; > > nr = syscall_enter_from_user_mode(regs, nr); > + current_thread_info()->syscall = nr; > > if (nr < NR_syscalls) { > syscall_fn = sys_call_table[nr]; > @@ -61,4 +62,5 @@ void noinstr do_syscall(struct pt_regs *regs) > } > > syscall_exit_to_user_mode(regs); > + current_thread_info()->syscall = 0; > } > > * allow ptrace to change syscall nr > * sys_exit_* will also see the right syscall nr > * this works even if rt_sigreturn clobbers all pt_regs::regs No, I still prefer to modify ELF_PLAT_INIT, we can wait Arnd's comments. And, do you mean modifying ELF_PLAT_INIT cannot solve the rt_sigreturn's problem? Huacai > > > And about ELF_PLAT_INIT, maybe Arnd can give us some more information. > > > > Hi, Arnd, > > > > I found some new architectures, such as ARM64 and RISC-V, just do > > nearly nothing in ELF_PLAT_INIT, while some old architectures, such as > > x86 and MIPS, clear most of the registers, do you know why? > > > > Huacai > > > > > > > > > Huacai > > > > > > > > > > > > > > Fixes: be769645a2aef ("LoongArch: Add system call support") > > > > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > > > > > --- > > > > > arch/loongarch/include/asm/syscall.h | 2 +- > > > > > arch/loongarch/kernel/syscall.c | 1 + > > > > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/arch/loongarch/include/asm/syscall.h b/arch/loongarch/include/asm/syscall.h > > > > > index e286dc58476e..2317d674b92a 100644 > > > > > --- a/arch/loongarch/include/asm/syscall.h > > > > > +++ b/arch/loongarch/include/asm/syscall.h > > > > > @@ -23,7 +23,7 @@ extern void *sys_call_table[]; > > > > > static inline long syscall_get_nr(struct task_struct *task, > > > > > struct pt_regs *regs) > > > > > { > > > > > - return regs->regs[11]; > > > > > + return task_thread_info(task)->syscall; > > > > > } > > > > > > > > > > static inline void syscall_rollback(struct task_struct *task, > > > > > diff --git a/arch/loongarch/kernel/syscall.c b/arch/loongarch/kernel/syscall.c > > > > > index b4c5acd7aa3b..2783e33cf276 100644 > > > > > --- a/arch/loongarch/kernel/syscall.c > > > > > +++ b/arch/loongarch/kernel/syscall.c > > > > > @@ -52,6 +52,7 @@ void noinstr do_syscall(struct pt_regs *regs) > > > > > regs->orig_a0 = regs->regs[4]; > > > > > regs->regs[4] = -ENOSYS; > > > > > > > > > > + task_thread_info(current)->syscall = nr; > > > > > nr = syscall_enter_from_user_mode(regs, nr); > > > > > > > > > > if (nr < NR_syscalls) { > > > > > -- > > > > > 2.42.0 > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] LoongArch: Store syscall nr in thread_info 2023-11-23 6:13 ` Huacai Chen @ 2023-11-23 8:08 ` Hengqi Chen 2023-11-23 8:25 ` Huacai Chen 0 siblings, 1 reply; 10+ messages in thread From: Hengqi Chen @ 2023-11-23 8:08 UTC (permalink / raw) To: Huacai Chen; +Cc: Arnd Bergmann, linux-arch, loongarch, kernel On Thu, Nov 23, 2023 at 2:13 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > Hi, Hengqi, > > On Thu, Nov 23, 2023 at 1:49 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > On Wed, Nov 22, 2023 at 3:58 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > Hi, Hengqi, > > > > > > On Wed, Nov 22, 2023 at 3:34 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > Hi, Huacai, > > > > > > > > On Wed, Nov 22, 2023 at 2:32 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > > > > > Hi, Hengqi, > > > > > > > > > > On Wed, Nov 22, 2023 at 1:14 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > > > > > Currently, we store syscall number in pt_regs::regs[11] and it may be > > > > > > changed during syscall execution. Take `execve` as an example: > > > > > > > > > > > > sys_execve > > > > > > -> do_execve > > > > > > -> do_execveat_common > > > > > > -> bprm_execve > > > > > > -> exec_binprm > > > > > > -> search_binary_handler > > > > > > -> load_elf_binary > > > > > > -> ELF_PLAT_INIT > > > > > > > > > > > > ELF_PLAT_INIT reset regs[11] to 0, later in syscall_exit_to_user_mode > > > > > > we get a wrong syscall nr. > > > > > > > > > > > > Known affected syscalls includes execve/execveat/rt_sigreturn. Tools > > > > > > like execsnoop do not work properly because the sys_exit_* tracepoints > > > > > > does not trigger at all. > > > > > > > > > > > > Let's store syscall nr in thread_info instead. > > > > > Can we just modify ELF_PLAT_INIT and not clear regs[11]? > > > > > > > > > > > > > I am uncertain about the side effects of changing ELF_PLAT_INIT. > > > > From a completeness perspective, changing ELF_PLAT_INIT is suboptimal, > > > > rt_sigreturn is affected in another code path, and there may be other > > > > syscalls that I am unaware of. > > > Save syscall number in thread_info has more side effects, because > > > ptrace allows us to change the number during syscall, then we should > > > keep consistency between syscall and regs[11]. > > > > > > > How about the change below: > > > > diff --git a/arch/loongarch/include/asm/syscall.h > > b/arch/loongarch/include/asm/syscall.h > > index e286dc58476e..954ba53bcc9a 100644 > > --- a/arch/loongarch/include/asm/syscall.h > > +++ b/arch/loongarch/include/asm/syscall.h > > @@ -23,7 +23,9 @@ extern void *sys_call_table[]; > > static inline long syscall_get_nr(struct task_struct *task, > > struct pt_regs *regs) > > { > > - return regs->regs[11]; > > + long nr = task_thread_info(task)->syscall; > > + > > + return nr ? : regs->regs[11]; > > } > > > > static inline void syscall_rollback(struct task_struct *task, > > diff --git a/arch/loongarch/kernel/syscall.c b/arch/loongarch/kernel/syscall.c > > index b4c5acd7aa3b..553ab0d624cb 100644 > > --- a/arch/loongarch/kernel/syscall.c > > +++ b/arch/loongarch/kernel/syscall.c > > @@ -53,6 +53,7 @@ void noinstr do_syscall(struct pt_regs *regs) > > regs->regs[4] = -ENOSYS; > > > > nr = syscall_enter_from_user_mode(regs, nr); > > + current_thread_info()->syscall = nr; > > > > if (nr < NR_syscalls) { > > syscall_fn = sys_call_table[nr]; > > @@ -61,4 +62,5 @@ void noinstr do_syscall(struct pt_regs *regs) > > } > > > > syscall_exit_to_user_mode(regs); > > + current_thread_info()->syscall = 0; > > } > > > > * allow ptrace to change syscall nr > > * sys_exit_* will also see the right syscall nr > > * this works even if rt_sigreturn clobbers all pt_regs::regs > No, I still prefer to modify ELF_PLAT_INIT, we can wait Arnd's comments. > OK, I am not eager, anyway, we know the root cause. :) > And, do you mean modifying ELF_PLAT_INIT cannot solve the > rt_sigreturn's problem? > Right, see https://elixir.bootlin.com/linux/latest/source/arch/loongarch/kernel/signal.c#L807 > Huacai > > > > > > And about ELF_PLAT_INIT, maybe Arnd can give us some more information. > > > > > > Hi, Arnd, > > > > > > I found some new architectures, such as ARM64 and RISC-V, just do > > > nearly nothing in ELF_PLAT_INIT, while some old architectures, such as > > > x86 and MIPS, clear most of the registers, do you know why? > > > > > > Huacai > > > > > > > > > > > > Huacai > > > > > > > > > > > > > > > > > Fixes: be769645a2aef ("LoongArch: Add system call support") > > > > > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > > > > > > --- > > > > > > arch/loongarch/include/asm/syscall.h | 2 +- > > > > > > arch/loongarch/kernel/syscall.c | 1 + > > > > > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/arch/loongarch/include/asm/syscall.h b/arch/loongarch/include/asm/syscall.h > > > > > > index e286dc58476e..2317d674b92a 100644 > > > > > > --- a/arch/loongarch/include/asm/syscall.h > > > > > > +++ b/arch/loongarch/include/asm/syscall.h > > > > > > @@ -23,7 +23,7 @@ extern void *sys_call_table[]; > > > > > > static inline long syscall_get_nr(struct task_struct *task, > > > > > > struct pt_regs *regs) > > > > > > { > > > > > > - return regs->regs[11]; > > > > > > + return task_thread_info(task)->syscall; > > > > > > } > > > > > > > > > > > > static inline void syscall_rollback(struct task_struct *task, > > > > > > diff --git a/arch/loongarch/kernel/syscall.c b/arch/loongarch/kernel/syscall.c > > > > > > index b4c5acd7aa3b..2783e33cf276 100644 > > > > > > --- a/arch/loongarch/kernel/syscall.c > > > > > > +++ b/arch/loongarch/kernel/syscall.c > > > > > > @@ -52,6 +52,7 @@ void noinstr do_syscall(struct pt_regs *regs) > > > > > > regs->orig_a0 = regs->regs[4]; > > > > > > regs->regs[4] = -ENOSYS; > > > > > > > > > > > > + task_thread_info(current)->syscall = nr; > > > > > > nr = syscall_enter_from_user_mode(regs, nr); > > > > > > > > > > > > if (nr < NR_syscalls) { > > > > > > -- > > > > > > 2.42.0 > > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] LoongArch: Store syscall nr in thread_info 2023-11-23 8:08 ` Hengqi Chen @ 2023-11-23 8:25 ` Huacai Chen 2023-11-23 14:39 ` Hengqi Chen 0 siblings, 1 reply; 10+ messages in thread From: Huacai Chen @ 2023-11-23 8:25 UTC (permalink / raw) To: Hengqi Chen; +Cc: Arnd Bergmann, linux-arch, loongarch, kernel On Thu, Nov 23, 2023 at 4:09 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > On Thu, Nov 23, 2023 at 2:13 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > Hi, Hengqi, > > > > On Thu, Nov 23, 2023 at 1:49 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > On Wed, Nov 22, 2023 at 3:58 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > > > Hi, Hengqi, > > > > > > > > On Wed, Nov 22, 2023 at 3:34 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > > > Hi, Huacai, > > > > > > > > > > On Wed, Nov 22, 2023 at 2:32 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > > > > > > > Hi, Hengqi, > > > > > > > > > > > > On Wed, Nov 22, 2023 at 1:14 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > > > > > > > Currently, we store syscall number in pt_regs::regs[11] and it may be > > > > > > > changed during syscall execution. Take `execve` as an example: > > > > > > > > > > > > > > sys_execve > > > > > > > -> do_execve > > > > > > > -> do_execveat_common > > > > > > > -> bprm_execve > > > > > > > -> exec_binprm > > > > > > > -> search_binary_handler > > > > > > > -> load_elf_binary > > > > > > > -> ELF_PLAT_INIT > > > > > > > > > > > > > > ELF_PLAT_INIT reset regs[11] to 0, later in syscall_exit_to_user_mode > > > > > > > we get a wrong syscall nr. > > > > > > > > > > > > > > Known affected syscalls includes execve/execveat/rt_sigreturn. Tools > > > > > > > like execsnoop do not work properly because the sys_exit_* tracepoints > > > > > > > does not trigger at all. > > > > > > > > > > > > > > Let's store syscall nr in thread_info instead. > > > > > > Can we just modify ELF_PLAT_INIT and not clear regs[11]? > > > > > > > > > > > > > > > > I am uncertain about the side effects of changing ELF_PLAT_INIT. > > > > > From a completeness perspective, changing ELF_PLAT_INIT is suboptimal, > > > > > rt_sigreturn is affected in another code path, and there may be other > > > > > syscalls that I am unaware of. > > > > Save syscall number in thread_info has more side effects, because > > > > ptrace allows us to change the number during syscall, then we should > > > > keep consistency between syscall and regs[11]. > > > > > > > > > > How about the change below: > > > > > > diff --git a/arch/loongarch/include/asm/syscall.h > > > b/arch/loongarch/include/asm/syscall.h > > > index e286dc58476e..954ba53bcc9a 100644 > > > --- a/arch/loongarch/include/asm/syscall.h > > > +++ b/arch/loongarch/include/asm/syscall.h > > > @@ -23,7 +23,9 @@ extern void *sys_call_table[]; > > > static inline long syscall_get_nr(struct task_struct *task, > > > struct pt_regs *regs) > > > { > > > - return regs->regs[11]; > > > + long nr = task_thread_info(task)->syscall; > > > + > > > + return nr ? : regs->regs[11]; > > > } > > > > > > static inline void syscall_rollback(struct task_struct *task, > > > diff --git a/arch/loongarch/kernel/syscall.c b/arch/loongarch/kernel/syscall.c > > > index b4c5acd7aa3b..553ab0d624cb 100644 > > > --- a/arch/loongarch/kernel/syscall.c > > > +++ b/arch/loongarch/kernel/syscall.c > > > @@ -53,6 +53,7 @@ void noinstr do_syscall(struct pt_regs *regs) > > > regs->regs[4] = -ENOSYS; > > > > > > nr = syscall_enter_from_user_mode(regs, nr); > > > + current_thread_info()->syscall = nr; > > > > > > if (nr < NR_syscalls) { > > > syscall_fn = sys_call_table[nr]; > > > @@ -61,4 +62,5 @@ void noinstr do_syscall(struct pt_regs *regs) > > > } > > > > > > syscall_exit_to_user_mode(regs); > > > + current_thread_info()->syscall = 0; > > > } > > > > > > * allow ptrace to change syscall nr > > > * sys_exit_* will also see the right syscall nr > > > * this works even if rt_sigreturn clobbers all pt_regs::regs > > No, I still prefer to modify ELF_PLAT_INIT, we can wait Arnd's comments. > > > > OK, I am not eager, anyway, we know the root cause. :) > > > And, do you mean modifying ELF_PLAT_INIT cannot solve the > > rt_sigreturn's problem? > > > > Right, see https://elixir.bootlin.com/linux/latest/source/arch/loongarch/kernel/signal.c#L807 Is this the expected behavior for rt_sigreturn()? Otherwise I think RISC-V has the same problem. And if we really need the 'correct' syscall number, we can overwrite regs[11] in sys_rt_sigreturn(). And another question: do you have any updates about the BPF system hang problem? :) Huacai > > > Huacai > > > > > > > > > And about ELF_PLAT_INIT, maybe Arnd can give us some more information. > > > > > > > > Hi, Arnd, > > > > > > > > I found some new architectures, such as ARM64 and RISC-V, just do > > > > nearly nothing in ELF_PLAT_INIT, while some old architectures, such as > > > > x86 and MIPS, clear most of the registers, do you know why? > > > > > > > > Huacai > > > > > > > > > > > > > > > Huacai > > > > > > > > > > > > > > > > > > > > Fixes: be769645a2aef ("LoongArch: Add system call support") > > > > > > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > > > > > > > --- > > > > > > > arch/loongarch/include/asm/syscall.h | 2 +- > > > > > > > arch/loongarch/kernel/syscall.c | 1 + > > > > > > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/arch/loongarch/include/asm/syscall.h b/arch/loongarch/include/asm/syscall.h > > > > > > > index e286dc58476e..2317d674b92a 100644 > > > > > > > --- a/arch/loongarch/include/asm/syscall.h > > > > > > > +++ b/arch/loongarch/include/asm/syscall.h > > > > > > > @@ -23,7 +23,7 @@ extern void *sys_call_table[]; > > > > > > > static inline long syscall_get_nr(struct task_struct *task, > > > > > > > struct pt_regs *regs) > > > > > > > { > > > > > > > - return regs->regs[11]; > > > > > > > + return task_thread_info(task)->syscall; > > > > > > > } > > > > > > > > > > > > > > static inline void syscall_rollback(struct task_struct *task, > > > > > > > diff --git a/arch/loongarch/kernel/syscall.c b/arch/loongarch/kernel/syscall.c > > > > > > > index b4c5acd7aa3b..2783e33cf276 100644 > > > > > > > --- a/arch/loongarch/kernel/syscall.c > > > > > > > +++ b/arch/loongarch/kernel/syscall.c > > > > > > > @@ -52,6 +52,7 @@ void noinstr do_syscall(struct pt_regs *regs) > > > > > > > regs->orig_a0 = regs->regs[4]; > > > > > > > regs->regs[4] = -ENOSYS; > > > > > > > > > > > > > > + task_thread_info(current)->syscall = nr; > > > > > > > nr = syscall_enter_from_user_mode(regs, nr); > > > > > > > > > > > > > > if (nr < NR_syscalls) { > > > > > > > -- > > > > > > > 2.42.0 > > > > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] LoongArch: Store syscall nr in thread_info 2023-11-23 8:25 ` Huacai Chen @ 2023-11-23 14:39 ` Hengqi Chen 2023-12-03 3:17 ` Huacai Chen 0 siblings, 1 reply; 10+ messages in thread From: Hengqi Chen @ 2023-11-23 14:39 UTC (permalink / raw) To: Huacai Chen; +Cc: Arnd Bergmann, linux-arch, loongarch, kernel On Thu, Nov 23, 2023 at 4:25 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > On Thu, Nov 23, 2023 at 4:09 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > On Thu, Nov 23, 2023 at 2:13 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > Hi, Hengqi, > > > > > > On Thu, Nov 23, 2023 at 1:49 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > On Wed, Nov 22, 2023 at 3:58 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > > > > > Hi, Hengqi, > > > > > > > > > > On Wed, Nov 22, 2023 at 3:34 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > > > > > Hi, Huacai, > > > > > > > > > > > > On Wed, Nov 22, 2023 at 2:32 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > > > > > > > > > Hi, Hengqi, > > > > > > > > > > > > > > On Wed, Nov 22, 2023 at 1:14 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > > > > > > > > > Currently, we store syscall number in pt_regs::regs[11] and it may be > > > > > > > > changed during syscall execution. Take `execve` as an example: > > > > > > > > > > > > > > > > sys_execve > > > > > > > > -> do_execve > > > > > > > > -> do_execveat_common > > > > > > > > -> bprm_execve > > > > > > > > -> exec_binprm > > > > > > > > -> search_binary_handler > > > > > > > > -> load_elf_binary > > > > > > > > -> ELF_PLAT_INIT > > > > > > > > > > > > > > > > ELF_PLAT_INIT reset regs[11] to 0, later in syscall_exit_to_user_mode > > > > > > > > we get a wrong syscall nr. > > > > > > > > > > > > > > > > Known affected syscalls includes execve/execveat/rt_sigreturn. Tools > > > > > > > > like execsnoop do not work properly because the sys_exit_* tracepoints > > > > > > > > does not trigger at all. > > > > > > > > > > > > > > > > Let's store syscall nr in thread_info instead. > > > > > > > Can we just modify ELF_PLAT_INIT and not clear regs[11]? > > > > > > > > > > > > > > > > > > > I am uncertain about the side effects of changing ELF_PLAT_INIT. > > > > > > From a completeness perspective, changing ELF_PLAT_INIT is suboptimal, > > > > > > rt_sigreturn is affected in another code path, and there may be other > > > > > > syscalls that I am unaware of. > > > > > Save syscall number in thread_info has more side effects, because > > > > > ptrace allows us to change the number during syscall, then we should > > > > > keep consistency between syscall and regs[11]. > > > > > > > > > > > > > How about the change below: > > > > > > > > diff --git a/arch/loongarch/include/asm/syscall.h > > > > b/arch/loongarch/include/asm/syscall.h > > > > index e286dc58476e..954ba53bcc9a 100644 > > > > --- a/arch/loongarch/include/asm/syscall.h > > > > +++ b/arch/loongarch/include/asm/syscall.h > > > > @@ -23,7 +23,9 @@ extern void *sys_call_table[]; > > > > static inline long syscall_get_nr(struct task_struct *task, > > > > struct pt_regs *regs) > > > > { > > > > - return regs->regs[11]; > > > > + long nr = task_thread_info(task)->syscall; > > > > + > > > > + return nr ? : regs->regs[11]; > > > > } > > > > > > > > static inline void syscall_rollback(struct task_struct *task, > > > > diff --git a/arch/loongarch/kernel/syscall.c b/arch/loongarch/kernel/syscall.c > > > > index b4c5acd7aa3b..553ab0d624cb 100644 > > > > --- a/arch/loongarch/kernel/syscall.c > > > > +++ b/arch/loongarch/kernel/syscall.c > > > > @@ -53,6 +53,7 @@ void noinstr do_syscall(struct pt_regs *regs) > > > > regs->regs[4] = -ENOSYS; > > > > > > > > nr = syscall_enter_from_user_mode(regs, nr); > > > > + current_thread_info()->syscall = nr; > > > > > > > > if (nr < NR_syscalls) { > > > > syscall_fn = sys_call_table[nr]; > > > > @@ -61,4 +62,5 @@ void noinstr do_syscall(struct pt_regs *regs) > > > > } > > > > > > > > syscall_exit_to_user_mode(regs); > > > > + current_thread_info()->syscall = 0; > > > > } > > > > > > > > * allow ptrace to change syscall nr > > > > * sys_exit_* will also see the right syscall nr > > > > * this works even if rt_sigreturn clobbers all pt_regs::regs > > > No, I still prefer to modify ELF_PLAT_INIT, we can wait Arnd's comments. > > > > > > > OK, I am not eager, anyway, we know the root cause. :) > > > > > And, do you mean modifying ELF_PLAT_INIT cannot solve the > > > rt_sigreturn's problem? > > > > > > > Right, see https://elixir.bootlin.com/linux/latest/source/arch/loongarch/kernel/signal.c#L807 > Is this the expected behavior for rt_sigreturn()? Otherwise I think > RISC-V has the same problem. And if we really need the 'correct' > syscall number, we can overwrite regs[11] in sys_rt_sigreturn(). > I check with x86 and arm64, both have the same issue, seems no one care. > And another question: do you have any updates about the BPF system > hang problem? :) > Let's discuss this on a new thread. > Huacai > > > > > > Huacai > > > > > > > > > > > > And about ELF_PLAT_INIT, maybe Arnd can give us some more information. > > > > > > > > > > Hi, Arnd, > > > > > > > > > > I found some new architectures, such as ARM64 and RISC-V, just do > > > > > nearly nothing in ELF_PLAT_INIT, while some old architectures, such as > > > > > x86 and MIPS, clear most of the registers, do you know why? > > > > > > > > > > Huacai > > > > > > > > > > > > > > > > > > Huacai > > > > > > > > > > > > > > > > > > > > > > > Fixes: be769645a2aef ("LoongArch: Add system call support") > > > > > > > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > > > > > > > > --- > > > > > > > > arch/loongarch/include/asm/syscall.h | 2 +- > > > > > > > > arch/loongarch/kernel/syscall.c | 1 + > > > > > > > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > diff --git a/arch/loongarch/include/asm/syscall.h b/arch/loongarch/include/asm/syscall.h > > > > > > > > index e286dc58476e..2317d674b92a 100644 > > > > > > > > --- a/arch/loongarch/include/asm/syscall.h > > > > > > > > +++ b/arch/loongarch/include/asm/syscall.h > > > > > > > > @@ -23,7 +23,7 @@ extern void *sys_call_table[]; > > > > > > > > static inline long syscall_get_nr(struct task_struct *task, > > > > > > > > struct pt_regs *regs) > > > > > > > > { > > > > > > > > - return regs->regs[11]; > > > > > > > > + return task_thread_info(task)->syscall; > > > > > > > > } > > > > > > > > > > > > > > > > static inline void syscall_rollback(struct task_struct *task, > > > > > > > > diff --git a/arch/loongarch/kernel/syscall.c b/arch/loongarch/kernel/syscall.c > > > > > > > > index b4c5acd7aa3b..2783e33cf276 100644 > > > > > > > > --- a/arch/loongarch/kernel/syscall.c > > > > > > > > +++ b/arch/loongarch/kernel/syscall.c > > > > > > > > @@ -52,6 +52,7 @@ void noinstr do_syscall(struct pt_regs *regs) > > > > > > > > regs->orig_a0 = regs->regs[4]; > > > > > > > > regs->regs[4] = -ENOSYS; > > > > > > > > > > > > > > > > + task_thread_info(current)->syscall = nr; > > > > > > > > nr = syscall_enter_from_user_mode(regs, nr); > > > > > > > > > > > > > > > > if (nr < NR_syscalls) { > > > > > > > > -- > > > > > > > > 2.42.0 > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] LoongArch: Store syscall nr in thread_info 2023-11-23 14:39 ` Hengqi Chen @ 2023-12-03 3:17 ` Huacai Chen 2023-12-04 1:55 ` Hengqi Chen 0 siblings, 1 reply; 10+ messages in thread From: Huacai Chen @ 2023-12-03 3:17 UTC (permalink / raw) To: Hengqi Chen; +Cc: Arnd Bergmann, linux-arch, loongarch, kernel Hi, Hengqi, On Thu, Nov 23, 2023 at 10:39 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > On Thu, Nov 23, 2023 at 4:25 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > On Thu, Nov 23, 2023 at 4:09 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > On Thu, Nov 23, 2023 at 2:13 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > > > Hi, Hengqi, > > > > > > > > On Thu, Nov 23, 2023 at 1:49 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > > > On Wed, Nov 22, 2023 at 3:58 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > > > > > > > Hi, Hengqi, > > > > > > > > > > > > On Wed, Nov 22, 2023 at 3:34 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > > > > > > > Hi, Huacai, > > > > > > > > > > > > > > On Wed, Nov 22, 2023 at 2:32 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > > > > > > > > > > > Hi, Hengqi, > > > > > > > > > > > > > > > > On Wed, Nov 22, 2023 at 1:14 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > > > > > > > > > > > Currently, we store syscall number in pt_regs::regs[11] and it may be > > > > > > > > > changed during syscall execution. Take `execve` as an example: > > > > > > > > > > > > > > > > > > sys_execve > > > > > > > > > -> do_execve > > > > > > > > > -> do_execveat_common > > > > > > > > > -> bprm_execve > > > > > > > > > -> exec_binprm > > > > > > > > > -> search_binary_handler > > > > > > > > > -> load_elf_binary > > > > > > > > > -> ELF_PLAT_INIT > > > > > > > > > > > > > > > > > > ELF_PLAT_INIT reset regs[11] to 0, later in syscall_exit_to_user_mode > > > > > > > > > we get a wrong syscall nr. > > > > > > > > > > > > > > > > > > Known affected syscalls includes execve/execveat/rt_sigreturn. Tools > > > > > > > > > like execsnoop do not work properly because the sys_exit_* tracepoints > > > > > > > > > does not trigger at all. > > > > > > > > > > > > > > > > > > Let's store syscall nr in thread_info instead. > > > > > > > > Can we just modify ELF_PLAT_INIT and not clear regs[11]? > > > > > > > > > > > > > > > > > > > > > > I am uncertain about the side effects of changing ELF_PLAT_INIT. > > > > > > > From a completeness perspective, changing ELF_PLAT_INIT is suboptimal, > > > > > > > rt_sigreturn is affected in another code path, and there may be other > > > > > > > syscalls that I am unaware of. > > > > > > Save syscall number in thread_info has more side effects, because > > > > > > ptrace allows us to change the number during syscall, then we should > > > > > > keep consistency between syscall and regs[11]. > > > > > > > > > > > > > > > > How about the change below: > > > > > > > > > > diff --git a/arch/loongarch/include/asm/syscall.h > > > > > b/arch/loongarch/include/asm/syscall.h > > > > > index e286dc58476e..954ba53bcc9a 100644 > > > > > --- a/arch/loongarch/include/asm/syscall.h > > > > > +++ b/arch/loongarch/include/asm/syscall.h > > > > > @@ -23,7 +23,9 @@ extern void *sys_call_table[]; > > > > > static inline long syscall_get_nr(struct task_struct *task, > > > > > struct pt_regs *regs) > > > > > { > > > > > - return regs->regs[11]; > > > > > + long nr = task_thread_info(task)->syscall; > > > > > + > > > > > + return nr ? : regs->regs[11]; > > > > > } > > > > > > > > > > static inline void syscall_rollback(struct task_struct *task, > > > > > diff --git a/arch/loongarch/kernel/syscall.c b/arch/loongarch/kernel/syscall.c > > > > > index b4c5acd7aa3b..553ab0d624cb 100644 > > > > > --- a/arch/loongarch/kernel/syscall.c > > > > > +++ b/arch/loongarch/kernel/syscall.c > > > > > @@ -53,6 +53,7 @@ void noinstr do_syscall(struct pt_regs *regs) > > > > > regs->regs[4] = -ENOSYS; > > > > > > > > > > nr = syscall_enter_from_user_mode(regs, nr); > > > > > + current_thread_info()->syscall = nr; > > > > > > > > > > if (nr < NR_syscalls) { > > > > > syscall_fn = sys_call_table[nr]; > > > > > @@ -61,4 +62,5 @@ void noinstr do_syscall(struct pt_regs *regs) > > > > > } > > > > > > > > > > syscall_exit_to_user_mode(regs); > > > > > + current_thread_info()->syscall = 0; > > > > > } > > > > > > > > > > * allow ptrace to change syscall nr > > > > > * sys_exit_* will also see the right syscall nr > > > > > * this works even if rt_sigreturn clobbers all pt_regs::regs > > > > No, I still prefer to modify ELF_PLAT_INIT, we can wait Arnd's comments. > > > > > > > > > > OK, I am not eager, anyway, we know the root cause. :) > > > > > > > And, do you mean modifying ELF_PLAT_INIT cannot solve the > > > > rt_sigreturn's problem? > > > > > > > > > > Right, see https://elixir.bootlin.com/linux/latest/source/arch/loongarch/kernel/signal.c#L807 > > Is this the expected behavior for rt_sigreturn()? Otherwise I think > > RISC-V has the same problem. And if we really need the 'correct' > > syscall number, we can overwrite regs[11] in sys_rt_sigreturn(). > > > > I check with x86 and arm64, both have the same issue, seems no one care. Does the rt_sigreturn issue affect execsnoop? Huacai > > > And another question: do you have any updates about the BPF system > > hang problem? :) > > > > Let's discuss this on a new thread. > > > Huacai > > > > > > > > > Huacai > > > > > > > > > > > > > > > And about ELF_PLAT_INIT, maybe Arnd can give us some more information. > > > > > > > > > > > > Hi, Arnd, > > > > > > > > > > > > I found some new architectures, such as ARM64 and RISC-V, just do > > > > > > nearly nothing in ELF_PLAT_INIT, while some old architectures, such as > > > > > > x86 and MIPS, clear most of the registers, do you know why? > > > > > > > > > > > > Huacai > > > > > > > > > > > > > > > > > > > > > Huacai > > > > > > > > > > > > > > > > > > > > > > > > > > Fixes: be769645a2aef ("LoongArch: Add system call support") > > > > > > > > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > > > > > > > > > --- > > > > > > > > > arch/loongarch/include/asm/syscall.h | 2 +- > > > > > > > > > arch/loongarch/kernel/syscall.c | 1 + > > > > > > > > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > > > diff --git a/arch/loongarch/include/asm/syscall.h b/arch/loongarch/include/asm/syscall.h > > > > > > > > > index e286dc58476e..2317d674b92a 100644 > > > > > > > > > --- a/arch/loongarch/include/asm/syscall.h > > > > > > > > > +++ b/arch/loongarch/include/asm/syscall.h > > > > > > > > > @@ -23,7 +23,7 @@ extern void *sys_call_table[]; > > > > > > > > > static inline long syscall_get_nr(struct task_struct *task, > > > > > > > > > struct pt_regs *regs) > > > > > > > > > { > > > > > > > > > - return regs->regs[11]; > > > > > > > > > + return task_thread_info(task)->syscall; > > > > > > > > > } > > > > > > > > > > > > > > > > > > static inline void syscall_rollback(struct task_struct *task, > > > > > > > > > diff --git a/arch/loongarch/kernel/syscall.c b/arch/loongarch/kernel/syscall.c > > > > > > > > > index b4c5acd7aa3b..2783e33cf276 100644 > > > > > > > > > --- a/arch/loongarch/kernel/syscall.c > > > > > > > > > +++ b/arch/loongarch/kernel/syscall.c > > > > > > > > > @@ -52,6 +52,7 @@ void noinstr do_syscall(struct pt_regs *regs) > > > > > > > > > regs->orig_a0 = regs->regs[4]; > > > > > > > > > regs->regs[4] = -ENOSYS; > > > > > > > > > > > > > > > > > > + task_thread_info(current)->syscall = nr; > > > > > > > > > nr = syscall_enter_from_user_mode(regs, nr); > > > > > > > > > > > > > > > > > > if (nr < NR_syscalls) { > > > > > > > > > -- > > > > > > > > > 2.42.0 > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] LoongArch: Store syscall nr in thread_info 2023-12-03 3:17 ` Huacai Chen @ 2023-12-04 1:55 ` Hengqi Chen 2023-12-04 2:16 ` Huacai Chen 0 siblings, 1 reply; 10+ messages in thread From: Hengqi Chen @ 2023-12-04 1:55 UTC (permalink / raw) To: Huacai Chen; +Cc: Arnd Bergmann, linux-arch, loongarch, kernel On Sun, Dec 3, 2023 at 11:17 AM Huacai Chen <chenhuacai@kernel.org> wrote: > > Hi, Hengqi, > > On Thu, Nov 23, 2023 at 10:39 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > On Thu, Nov 23, 2023 at 4:25 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > On Thu, Nov 23, 2023 at 4:09 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > On Thu, Nov 23, 2023 at 2:13 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > > > > > Hi, Hengqi, > > > > > > > > > > On Thu, Nov 23, 2023 at 1:49 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > > > > > On Wed, Nov 22, 2023 at 3:58 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > > > > > > > > > Hi, Hengqi, > > > > > > > > > > > > > > On Wed, Nov 22, 2023 at 3:34 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > > > > > > > > > Hi, Huacai, > > > > > > > > > > > > > > > > On Wed, Nov 22, 2023 at 2:32 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > > > > > > > > > > > > > Hi, Hengqi, > > > > > > > > > > > > > > > > > > On Wed, Nov 22, 2023 at 1:14 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > Currently, we store syscall number in pt_regs::regs[11] and it may be > > > > > > > > > > changed during syscall execution. Take `execve` as an example: > > > > > > > > > > > > > > > > > > > > sys_execve > > > > > > > > > > -> do_execve > > > > > > > > > > -> do_execveat_common > > > > > > > > > > -> bprm_execve > > > > > > > > > > -> exec_binprm > > > > > > > > > > -> search_binary_handler > > > > > > > > > > -> load_elf_binary > > > > > > > > > > -> ELF_PLAT_INIT > > > > > > > > > > > > > > > > > > > > ELF_PLAT_INIT reset regs[11] to 0, later in syscall_exit_to_user_mode > > > > > > > > > > we get a wrong syscall nr. > > > > > > > > > > > > > > > > > > > > Known affected syscalls includes execve/execveat/rt_sigreturn. Tools > > > > > > > > > > like execsnoop do not work properly because the sys_exit_* tracepoints > > > > > > > > > > does not trigger at all. > > > > > > > > > > > > > > > > > > > > Let's store syscall nr in thread_info instead. > > > > > > > > > Can we just modify ELF_PLAT_INIT and not clear regs[11]? > > > > > > > > > > > > > > > > > > > > > > > > > I am uncertain about the side effects of changing ELF_PLAT_INIT. > > > > > > > > From a completeness perspective, changing ELF_PLAT_INIT is suboptimal, > > > > > > > > rt_sigreturn is affected in another code path, and there may be other > > > > > > > > syscalls that I am unaware of. > > > > > > > Save syscall number in thread_info has more side effects, because > > > > > > > ptrace allows us to change the number during syscall, then we should > > > > > > > keep consistency between syscall and regs[11]. > > > > > > > > > > > > > > > > > > > How about the change below: > > > > > > > > > > > > diff --git a/arch/loongarch/include/asm/syscall.h > > > > > > b/arch/loongarch/include/asm/syscall.h > > > > > > index e286dc58476e..954ba53bcc9a 100644 > > > > > > --- a/arch/loongarch/include/asm/syscall.h > > > > > > +++ b/arch/loongarch/include/asm/syscall.h > > > > > > @@ -23,7 +23,9 @@ extern void *sys_call_table[]; > > > > > > static inline long syscall_get_nr(struct task_struct *task, > > > > > > struct pt_regs *regs) > > > > > > { > > > > > > - return regs->regs[11]; > > > > > > + long nr = task_thread_info(task)->syscall; > > > > > > + > > > > > > + return nr ? : regs->regs[11]; > > > > > > } > > > > > > > > > > > > static inline void syscall_rollback(struct task_struct *task, > > > > > > diff --git a/arch/loongarch/kernel/syscall.c b/arch/loongarch/kernel/syscall.c > > > > > > index b4c5acd7aa3b..553ab0d624cb 100644 > > > > > > --- a/arch/loongarch/kernel/syscall.c > > > > > > +++ b/arch/loongarch/kernel/syscall.c > > > > > > @@ -53,6 +53,7 @@ void noinstr do_syscall(struct pt_regs *regs) > > > > > > regs->regs[4] = -ENOSYS; > > > > > > > > > > > > nr = syscall_enter_from_user_mode(regs, nr); > > > > > > + current_thread_info()->syscall = nr; > > > > > > > > > > > > if (nr < NR_syscalls) { > > > > > > syscall_fn = sys_call_table[nr]; > > > > > > @@ -61,4 +62,5 @@ void noinstr do_syscall(struct pt_regs *regs) > > > > > > } > > > > > > > > > > > > syscall_exit_to_user_mode(regs); > > > > > > + current_thread_info()->syscall = 0; > > > > > > } > > > > > > > > > > > > * allow ptrace to change syscall nr > > > > > > * sys_exit_* will also see the right syscall nr > > > > > > * this works even if rt_sigreturn clobbers all pt_regs::regs > > > > > No, I still prefer to modify ELF_PLAT_INIT, we can wait Arnd's comments. > > > > > > > > > > > > > OK, I am not eager, anyway, we know the root cause. :) > > > > > > > > > And, do you mean modifying ELF_PLAT_INIT cannot solve the > > > > > rt_sigreturn's problem? > > > > > > > > > > > > > Right, see https://elixir.bootlin.com/linux/latest/source/arch/loongarch/kernel/signal.c#L807 > > > Is this the expected behavior for rt_sigreturn()? Otherwise I think > > > RISC-V has the same problem. And if we really need the 'correct' > > > syscall number, we can overwrite regs[11] in sys_rt_sigreturn(). > > > > > > > I check with x86 and arm64, both have the same issue, seems no one care. > Does the rt_sigreturn issue affect execsnoop? > No, execsnoop only relies on sys_enter_execve/sys_exit_execve. > Huacai > > > > > > And another question: do you have any updates about the BPF system > > > hang problem? :) > > > > > > > Let's discuss this on a new thread. > > > > > Huacai > > > > > > > > > > > > Huacai > > > > > > > > > > > > > > > > > > And about ELF_PLAT_INIT, maybe Arnd can give us some more information. > > > > > > > > > > > > > > Hi, Arnd, > > > > > > > > > > > > > > I found some new architectures, such as ARM64 and RISC-V, just do > > > > > > > nearly nothing in ELF_PLAT_INIT, while some old architectures, such as > > > > > > > x86 and MIPS, clear most of the registers, do you know why? > > > > > > > > > > > > > > Huacai > > > > > > > > > > > > > > > > > > > > > > > > Huacai > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fixes: be769645a2aef ("LoongArch: Add system call support") > > > > > > > > > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > > > > > > > > > > --- > > > > > > > > > > arch/loongarch/include/asm/syscall.h | 2 +- > > > > > > > > > > arch/loongarch/kernel/syscall.c | 1 + > > > > > > > > > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > > > > > diff --git a/arch/loongarch/include/asm/syscall.h b/arch/loongarch/include/asm/syscall.h > > > > > > > > > > index e286dc58476e..2317d674b92a 100644 > > > > > > > > > > --- a/arch/loongarch/include/asm/syscall.h > > > > > > > > > > +++ b/arch/loongarch/include/asm/syscall.h > > > > > > > > > > @@ -23,7 +23,7 @@ extern void *sys_call_table[]; > > > > > > > > > > static inline long syscall_get_nr(struct task_struct *task, > > > > > > > > > > struct pt_regs *regs) > > > > > > > > > > { > > > > > > > > > > - return regs->regs[11]; > > > > > > > > > > + return task_thread_info(task)->syscall; > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > static inline void syscall_rollback(struct task_struct *task, > > > > > > > > > > diff --git a/arch/loongarch/kernel/syscall.c b/arch/loongarch/kernel/syscall.c > > > > > > > > > > index b4c5acd7aa3b..2783e33cf276 100644 > > > > > > > > > > --- a/arch/loongarch/kernel/syscall.c > > > > > > > > > > +++ b/arch/loongarch/kernel/syscall.c > > > > > > > > > > @@ -52,6 +52,7 @@ void noinstr do_syscall(struct pt_regs *regs) > > > > > > > > > > regs->orig_a0 = regs->regs[4]; > > > > > > > > > > regs->regs[4] = -ENOSYS; > > > > > > > > > > > > > > > > > > > > + task_thread_info(current)->syscall = nr; > > > > > > > > > > nr = syscall_enter_from_user_mode(regs, nr); > > > > > > > > > > > > > > > > > > > > if (nr < NR_syscalls) { > > > > > > > > > > -- > > > > > > > > > > 2.42.0 > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] LoongArch: Store syscall nr in thread_info 2023-12-04 1:55 ` Hengqi Chen @ 2023-12-04 2:16 ` Huacai Chen 2023-12-04 5:39 ` Hengqi Chen 0 siblings, 1 reply; 10+ messages in thread From: Huacai Chen @ 2023-12-04 2:16 UTC (permalink / raw) To: Hengqi Chen; +Cc: Arnd Bergmann, linux-arch, loongarch, kernel Hi, Hengqi, On Mon, Dec 4, 2023 at 9:56 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > On Sun, Dec 3, 2023 at 11:17 AM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > Hi, Hengqi, > > > > On Thu, Nov 23, 2023 at 10:39 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > On Thu, Nov 23, 2023 at 4:25 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > > > On Thu, Nov 23, 2023 at 4:09 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > > > On Thu, Nov 23, 2023 at 2:13 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > > > > > > > Hi, Hengqi, > > > > > > > > > > > > On Thu, Nov 23, 2023 at 1:49 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > > > > > > > On Wed, Nov 22, 2023 at 3:58 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > > > > > > > > > > > Hi, Hengqi, > > > > > > > > > > > > > > > > On Wed, Nov 22, 2023 at 3:34 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > > > > > > > > > > > Hi, Huacai, > > > > > > > > > > > > > > > > > > On Wed, Nov 22, 2023 at 2:32 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > > > > > > > > > > > > > > > Hi, Hengqi, > > > > > > > > > > > > > > > > > > > > On Wed, Nov 22, 2023 at 1:14 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > > > Currently, we store syscall number in pt_regs::regs[11] and it may be > > > > > > > > > > > changed during syscall execution. Take `execve` as an example: > > > > > > > > > > > > > > > > > > > > > > sys_execve > > > > > > > > > > > -> do_execve > > > > > > > > > > > -> do_execveat_common > > > > > > > > > > > -> bprm_execve > > > > > > > > > > > -> exec_binprm > > > > > > > > > > > -> search_binary_handler > > > > > > > > > > > -> load_elf_binary > > > > > > > > > > > -> ELF_PLAT_INIT > > > > > > > > > > > > > > > > > > > > > > ELF_PLAT_INIT reset regs[11] to 0, later in syscall_exit_to_user_mode > > > > > > > > > > > we get a wrong syscall nr. > > > > > > > > > > > > > > > > > > > > > > Known affected syscalls includes execve/execveat/rt_sigreturn. Tools > > > > > > > > > > > like execsnoop do not work properly because the sys_exit_* tracepoints > > > > > > > > > > > does not trigger at all. > > > > > > > > > > > > > > > > > > > > > > Let's store syscall nr in thread_info instead. > > > > > > > > > > Can we just modify ELF_PLAT_INIT and not clear regs[11]? > > > > > > > > > > > > > > > > > > > > > > > > > > > > I am uncertain about the side effects of changing ELF_PLAT_INIT. > > > > > > > > > From a completeness perspective, changing ELF_PLAT_INIT is suboptimal, > > > > > > > > > rt_sigreturn is affected in another code path, and there may be other > > > > > > > > > syscalls that I am unaware of. > > > > > > > > Save syscall number in thread_info has more side effects, because > > > > > > > > ptrace allows us to change the number during syscall, then we should > > > > > > > > keep consistency between syscall and regs[11]. > > > > > > > > > > > > > > > > > > > > > > How about the change below: > > > > > > > > > > > > > > diff --git a/arch/loongarch/include/asm/syscall.h > > > > > > > b/arch/loongarch/include/asm/syscall.h > > > > > > > index e286dc58476e..954ba53bcc9a 100644 > > > > > > > --- a/arch/loongarch/include/asm/syscall.h > > > > > > > +++ b/arch/loongarch/include/asm/syscall.h > > > > > > > @@ -23,7 +23,9 @@ extern void *sys_call_table[]; > > > > > > > static inline long syscall_get_nr(struct task_struct *task, > > > > > > > struct pt_regs *regs) > > > > > > > { > > > > > > > - return regs->regs[11]; > > > > > > > + long nr = task_thread_info(task)->syscall; > > > > > > > + > > > > > > > + return nr ? : regs->regs[11]; > > > > > > > } > > > > > > > > > > > > > > static inline void syscall_rollback(struct task_struct *task, > > > > > > > diff --git a/arch/loongarch/kernel/syscall.c b/arch/loongarch/kernel/syscall.c > > > > > > > index b4c5acd7aa3b..553ab0d624cb 100644 > > > > > > > --- a/arch/loongarch/kernel/syscall.c > > > > > > > +++ b/arch/loongarch/kernel/syscall.c > > > > > > > @@ -53,6 +53,7 @@ void noinstr do_syscall(struct pt_regs *regs) > > > > > > > regs->regs[4] = -ENOSYS; > > > > > > > > > > > > > > nr = syscall_enter_from_user_mode(regs, nr); > > > > > > > + current_thread_info()->syscall = nr; > > > > > > > > > > > > > > if (nr < NR_syscalls) { > > > > > > > syscall_fn = sys_call_table[nr]; > > > > > > > @@ -61,4 +62,5 @@ void noinstr do_syscall(struct pt_regs *regs) > > > > > > > } > > > > > > > > > > > > > > syscall_exit_to_user_mode(regs); > > > > > > > + current_thread_info()->syscall = 0; > > > > > > > } > > > > > > > > > > > > > > * allow ptrace to change syscall nr > > > > > > > * sys_exit_* will also see the right syscall nr > > > > > > > * this works even if rt_sigreturn clobbers all pt_regs::regs > > > > > > No, I still prefer to modify ELF_PLAT_INIT, we can wait Arnd's comments. > > > > > > > > > > > > > > > > OK, I am not eager, anyway, we know the root cause. :) > > > > > > > > > > > And, do you mean modifying ELF_PLAT_INIT cannot solve the > > > > > > rt_sigreturn's problem? > > > > > > > > > > > > > > > > Right, see https://elixir.bootlin.com/linux/latest/source/arch/loongarch/kernel/signal.c#L807 > > > > Is this the expected behavior for rt_sigreturn()? Otherwise I think > > > > RISC-V has the same problem. And if we really need the 'correct' > > > > syscall number, we can overwrite regs[11] in sys_rt_sigreturn(). > > > > > > > > > > I check with x86 and arm64, both have the same issue, seems no one care. > > Does the rt_sigreturn issue affect execsnoop? > > > > No, execsnoop only relies on sys_enter_execve/sys_exit_execve. Then I suggest the below one line patch, though Arnd still no response, I believe it has no side effect (because ARM64 and RISC-V clear nothing here): diff --git a/arch/loongarch/include/asm/elf.h b/arch/loongarch/include/asm/elf.h index b9a4ab54285c..45a2a2f7a27f 100644 --- a/arch/loongarch/include/asm/elf.h +++ b/arch/loongarch/include/asm/elf.h @@ -293,7 +293,7 @@ extern const char *__elf_platform; #define ELF_PLAT_INIT(_r, load_addr) do { \ _r->regs[1] = _r->regs[2] = _r->regs[3] = _r->regs[4] = 0; \ _r->regs[5] = _r->regs[6] = _r->regs[7] = _r->regs[8] = 0; \ - _r->regs[9] = _r->regs[10] = _r->regs[11] = _r->regs[12] = 0; \ + _r->regs[9] = _r->regs[10] = /* syscall */ = _r->regs[12] = 0; \ _r->regs[13] = _r->regs[14] = _r->regs[15] = _r->regs[16] = 0; \ _r->regs[17] = _r->regs[18] = _r->regs[19] = _r->regs[20] = 0; \ _r->regs[21] = _r->regs[22] = _r->regs[23] = _r->regs[24] = 0; \ > > > Huacai > > > > > > > > > And another question: do you have any updates about the BPF system > > > > hang problem? :) > > > > > > > > > > Let's discuss this on a new thread. > > > > > > > Huacai > > > > > > > > > > > > > > > Huacai > > > > > > > > > > > > > > > > > > > > > And about ELF_PLAT_INIT, maybe Arnd can give us some more information. > > > > > > > > > > > > > > > > Hi, Arnd, > > > > > > > > > > > > > > > > I found some new architectures, such as ARM64 and RISC-V, just do > > > > > > > > nearly nothing in ELF_PLAT_INIT, while some old architectures, such as > > > > > > > > x86 and MIPS, clear most of the registers, do you know why? > > > > > > > > > > > > > > > > Huacai > > > > > > > > > > > > > > > > > > > > > > > > > > > Huacai > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fixes: be769645a2aef ("LoongArch: Add system call support") > > > > > > > > > > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > > > > > > > > > > > --- > > > > > > > > > > > arch/loongarch/include/asm/syscall.h | 2 +- > > > > > > > > > > > arch/loongarch/kernel/syscall.c | 1 + > > > > > > > > > > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > > > > > > > diff --git a/arch/loongarch/include/asm/syscall.h b/arch/loongarch/include/asm/syscall.h > > > > > > > > > > > index e286dc58476e..2317d674b92a 100644 > > > > > > > > > > > --- a/arch/loongarch/include/asm/syscall.h > > > > > > > > > > > +++ b/arch/loongarch/include/asm/syscall.h > > > > > > > > > > > @@ -23,7 +23,7 @@ extern void *sys_call_table[]; > > > > > > > > > > > static inline long syscall_get_nr(struct task_struct *task, > > > > > > > > > > > struct pt_regs *regs) > > > > > > > > > > > { > > > > > > > > > > > - return regs->regs[11]; > > > > > > > > > > > + return task_thread_info(task)->syscall; > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > static inline void syscall_rollback(struct task_struct *task, > > > > > > > > > > > diff --git a/arch/loongarch/kernel/syscall.c b/arch/loongarch/kernel/syscall.c > > > > > > > > > > > index b4c5acd7aa3b..2783e33cf276 100644 > > > > > > > > > > > --- a/arch/loongarch/kernel/syscall.c > > > > > > > > > > > +++ b/arch/loongarch/kernel/syscall.c > > > > > > > > > > > @@ -52,6 +52,7 @@ void noinstr do_syscall(struct pt_regs *regs) > > > > > > > > > > > regs->orig_a0 = regs->regs[4]; > > > > > > > > > > > regs->regs[4] = -ENOSYS; > > > > > > > > > > > > > > > > > > > > > > + task_thread_info(current)->syscall = nr; > > > > > > > > > > > nr = syscall_enter_from_user_mode(regs, nr); > > > > > > > > > > > > > > > > > > > > > > if (nr < NR_syscalls) { > > > > > > > > > > > -- > > > > > > > > > > > 2.42.0 > > > > > > > > > > > > > > > > > > > > > ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] LoongArch: Store syscall nr in thread_info 2023-12-04 2:16 ` Huacai Chen @ 2023-12-04 5:39 ` Hengqi Chen 0 siblings, 0 replies; 10+ messages in thread From: Hengqi Chen @ 2023-12-04 5:39 UTC (permalink / raw) To: Huacai Chen; +Cc: Arnd Bergmann, linux-arch, loongarch, kernel Hi, Huacai, Thanks for the suggestion, just send the fix. Cheers, --- Hengqi On Mon, Dec 4, 2023 at 10:17 AM Huacai Chen <chenhuacai@kernel.org> wrote: > > Hi, Hengqi, > > On Mon, Dec 4, 2023 at 9:56 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > On Sun, Dec 3, 2023 at 11:17 AM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > Hi, Hengqi, > > > > > > On Thu, Nov 23, 2023 at 10:39 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > On Thu, Nov 23, 2023 at 4:25 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > > > > > On Thu, Nov 23, 2023 at 4:09 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > > > > > On Thu, Nov 23, 2023 at 2:13 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > > > > > > > > > Hi, Hengqi, > > > > > > > > > > > > > > On Thu, Nov 23, 2023 at 1:49 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > > > > > > > > > On Wed, Nov 22, 2023 at 3:58 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > > > > > > > > > > > > > Hi, Hengqi, > > > > > > > > > > > > > > > > > > On Wed, Nov 22, 2023 at 3:34 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > Hi, Huacai, > > > > > > > > > > > > > > > > > > > > On Wed, Nov 22, 2023 at 2:32 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > > > > > > > > > > > > > > > > > Hi, Hengqi, > > > > > > > > > > > > > > > > > > > > > > On Wed, Nov 22, 2023 at 1:14 PM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > Currently, we store syscall number in pt_regs::regs[11] and it may be > > > > > > > > > > > > changed during syscall execution. Take `execve` as an example: > > > > > > > > > > > > > > > > > > > > > > > > sys_execve > > > > > > > > > > > > -> do_execve > > > > > > > > > > > > -> do_execveat_common > > > > > > > > > > > > -> bprm_execve > > > > > > > > > > > > -> exec_binprm > > > > > > > > > > > > -> search_binary_handler > > > > > > > > > > > > -> load_elf_binary > > > > > > > > > > > > -> ELF_PLAT_INIT > > > > > > > > > > > > > > > > > > > > > > > > ELF_PLAT_INIT reset regs[11] to 0, later in syscall_exit_to_user_mode > > > > > > > > > > > > we get a wrong syscall nr. > > > > > > > > > > > > > > > > > > > > > > > > Known affected syscalls includes execve/execveat/rt_sigreturn. Tools > > > > > > > > > > > > like execsnoop do not work properly because the sys_exit_* tracepoints > > > > > > > > > > > > does not trigger at all. > > > > > > > > > > > > > > > > > > > > > > > > Let's store syscall nr in thread_info instead. > > > > > > > > > > > Can we just modify ELF_PLAT_INIT and not clear regs[11]? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I am uncertain about the side effects of changing ELF_PLAT_INIT. > > > > > > > > > > From a completeness perspective, changing ELF_PLAT_INIT is suboptimal, > > > > > > > > > > rt_sigreturn is affected in another code path, and there may be other > > > > > > > > > > syscalls that I am unaware of. > > > > > > > > > Save syscall number in thread_info has more side effects, because > > > > > > > > > ptrace allows us to change the number during syscall, then we should > > > > > > > > > keep consistency between syscall and regs[11]. > > > > > > > > > > > > > > > > > > > > > > > > > How about the change below: > > > > > > > > > > > > > > > > diff --git a/arch/loongarch/include/asm/syscall.h > > > > > > > > b/arch/loongarch/include/asm/syscall.h > > > > > > > > index e286dc58476e..954ba53bcc9a 100644 > > > > > > > > --- a/arch/loongarch/include/asm/syscall.h > > > > > > > > +++ b/arch/loongarch/include/asm/syscall.h > > > > > > > > @@ -23,7 +23,9 @@ extern void *sys_call_table[]; > > > > > > > > static inline long syscall_get_nr(struct task_struct *task, > > > > > > > > struct pt_regs *regs) > > > > > > > > { > > > > > > > > - return regs->regs[11]; > > > > > > > > + long nr = task_thread_info(task)->syscall; > > > > > > > > + > > > > > > > > + return nr ? : regs->regs[11]; > > > > > > > > } > > > > > > > > > > > > > > > > static inline void syscall_rollback(struct task_struct *task, > > > > > > > > diff --git a/arch/loongarch/kernel/syscall.c b/arch/loongarch/kernel/syscall.c > > > > > > > > index b4c5acd7aa3b..553ab0d624cb 100644 > > > > > > > > --- a/arch/loongarch/kernel/syscall.c > > > > > > > > +++ b/arch/loongarch/kernel/syscall.c > > > > > > > > @@ -53,6 +53,7 @@ void noinstr do_syscall(struct pt_regs *regs) > > > > > > > > regs->regs[4] = -ENOSYS; > > > > > > > > > > > > > > > > nr = syscall_enter_from_user_mode(regs, nr); > > > > > > > > + current_thread_info()->syscall = nr; > > > > > > > > > > > > > > > > if (nr < NR_syscalls) { > > > > > > > > syscall_fn = sys_call_table[nr]; > > > > > > > > @@ -61,4 +62,5 @@ void noinstr do_syscall(struct pt_regs *regs) > > > > > > > > } > > > > > > > > > > > > > > > > syscall_exit_to_user_mode(regs); > > > > > > > > + current_thread_info()->syscall = 0; > > > > > > > > } > > > > > > > > > > > > > > > > * allow ptrace to change syscall nr > > > > > > > > * sys_exit_* will also see the right syscall nr > > > > > > > > * this works even if rt_sigreturn clobbers all pt_regs::regs > > > > > > > No, I still prefer to modify ELF_PLAT_INIT, we can wait Arnd's comments. > > > > > > > > > > > > > > > > > > > OK, I am not eager, anyway, we know the root cause. :) > > > > > > > > > > > > > And, do you mean modifying ELF_PLAT_INIT cannot solve the > > > > > > > rt_sigreturn's problem? > > > > > > > > > > > > > > > > > > > Right, see https://elixir.bootlin.com/linux/latest/source/arch/loongarch/kernel/signal.c#L807 > > > > > Is this the expected behavior for rt_sigreturn()? Otherwise I think > > > > > RISC-V has the same problem. And if we really need the 'correct' > > > > > syscall number, we can overwrite regs[11] in sys_rt_sigreturn(). > > > > > > > > > > > > > I check with x86 and arm64, both have the same issue, seems no one care. > > > Does the rt_sigreturn issue affect execsnoop? > > > > > > > No, execsnoop only relies on sys_enter_execve/sys_exit_execve. > Then I suggest the below one line patch, though Arnd still no > response, I believe it has no side effect (because ARM64 and RISC-V > clear nothing here): > > diff --git a/arch/loongarch/include/asm/elf.h b/arch/loongarch/include/asm/elf.h > index b9a4ab54285c..45a2a2f7a27f 100644 > --- a/arch/loongarch/include/asm/elf.h > +++ b/arch/loongarch/include/asm/elf.h > @@ -293,7 +293,7 @@ extern const char *__elf_platform; > #define ELF_PLAT_INIT(_r, load_addr) do { \ > _r->regs[1] = _r->regs[2] = _r->regs[3] = _r->regs[4] = 0; \ > _r->regs[5] = _r->regs[6] = _r->regs[7] = _r->regs[8] = 0; \ > - _r->regs[9] = _r->regs[10] = _r->regs[11] = _r->regs[12] = 0; \ > + _r->regs[9] = _r->regs[10] = /* syscall */ = _r->regs[12] = 0; \ > _r->regs[13] = _r->regs[14] = _r->regs[15] = _r->regs[16] = 0; \ > _r->regs[17] = _r->regs[18] = _r->regs[19] = _r->regs[20] = 0; \ > _r->regs[21] = _r->regs[22] = _r->regs[23] = _r->regs[24] = 0; \ > > > > > > Huacai > > > > > > > > > > > > And another question: do you have any updates about the BPF system > > > > > hang problem? :) > > > > > > > > > > > > > Let's discuss this on a new thread. > > > > > > > > > Huacai > > > > > > > > > > > > > > > > > > Huacai > > > > > > > > > > > > > > > > > > > > > > > > And about ELF_PLAT_INIT, maybe Arnd can give us some more information. > > > > > > > > > > > > > > > > > > Hi, Arnd, > > > > > > > > > > > > > > > > > > I found some new architectures, such as ARM64 and RISC-V, just do > > > > > > > > > nearly nothing in ELF_PLAT_INIT, while some old architectures, such as > > > > > > > > > x86 and MIPS, clear most of the registers, do you know why? > > > > > > > > > > > > > > > > > > Huacai > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Huacai > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fixes: be769645a2aef ("LoongArch: Add system call support") > > > > > > > > > > > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > > > > > > > > > > > > --- > > > > > > > > > > > > arch/loongarch/include/asm/syscall.h | 2 +- > > > > > > > > > > > > arch/loongarch/kernel/syscall.c | 1 + > > > > > > > > > > > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/arch/loongarch/include/asm/syscall.h b/arch/loongarch/include/asm/syscall.h > > > > > > > > > > > > index e286dc58476e..2317d674b92a 100644 > > > > > > > > > > > > --- a/arch/loongarch/include/asm/syscall.h > > > > > > > > > > > > +++ b/arch/loongarch/include/asm/syscall.h > > > > > > > > > > > > @@ -23,7 +23,7 @@ extern void *sys_call_table[]; > > > > > > > > > > > > static inline long syscall_get_nr(struct task_struct *task, > > > > > > > > > > > > struct pt_regs *regs) > > > > > > > > > > > > { > > > > > > > > > > > > - return regs->regs[11]; > > > > > > > > > > > > + return task_thread_info(task)->syscall; > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > static inline void syscall_rollback(struct task_struct *task, > > > > > > > > > > > > diff --git a/arch/loongarch/kernel/syscall.c b/arch/loongarch/kernel/syscall.c > > > > > > > > > > > > index b4c5acd7aa3b..2783e33cf276 100644 > > > > > > > > > > > > --- a/arch/loongarch/kernel/syscall.c > > > > > > > > > > > > +++ b/arch/loongarch/kernel/syscall.c > > > > > > > > > > > > @@ -52,6 +52,7 @@ void noinstr do_syscall(struct pt_regs *regs) > > > > > > > > > > > > regs->orig_a0 = regs->regs[4]; > > > > > > > > > > > > regs->regs[4] = -ENOSYS; > > > > > > > > > > > > > > > > > > > > > > > > + task_thread_info(current)->syscall = nr; > > > > > > > > > > > > nr = syscall_enter_from_user_mode(regs, nr); > > > > > > > > > > > > > > > > > > > > > > > > if (nr < NR_syscalls) { > > > > > > > > > > > > -- > > > > > > > > > > > > 2.42.0 > > > > > > > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-12-04 5:39 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20231121070209.210934-1-hengqi.chen@gmail.com>
[not found] ` <CAAhV-H7SwSRDh8Ui2xVb1ncoaEQVd=dugphcBemkeaPNGQX2qw@mail.gmail.com>
[not found] ` <CAEyhmHRYghT5iFiLByUmC=AjdygiBWU8TH3joSyyWibu0Ki2xw@mail.gmail.com>
2023-11-22 7:58 ` [PATCH] LoongArch: Store syscall nr in thread_info Huacai Chen
2023-11-23 5:49 ` Hengqi Chen
2023-11-23 6:13 ` Huacai Chen
2023-11-23 8:08 ` Hengqi Chen
2023-11-23 8:25 ` Huacai Chen
2023-11-23 14:39 ` Hengqi Chen
2023-12-03 3:17 ` Huacai Chen
2023-12-04 1:55 ` Hengqi Chen
2023-12-04 2:16 ` Huacai Chen
2023-12-04 5:39 ` Hengqi Chen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).