* [PATCH v2] x86/syscalls: Mark expected switch fall-throughs @ 2019-02-28 19:27 Gustavo A. R. Silva 2019-03-23 16:14 ` Thomas Gleixner 0 siblings, 1 reply; 13+ messages in thread From: Gustavo A. R. Silva @ 2019-02-28 19:27 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86 Cc: linux-kernel, Dominik Brodowski, Andy Lutomirski, Kees Cook, Gustavo A. R. Silva In preparation to enable -Wimplicit-fallthrough by default, mark switch-case statements where fall-through is intentional, explicitly in order to fix a bunch of -Wimplicit-fallthrough warnings. In order to get the warnings mentioned above, the following line was added to the top Makefile: KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough=3,) Reviewed-by: Kees Cook <keescook@chromium.org> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> --- Changes in v2: - Expand commit text as requested by Thomas Gleixner. - Add Kees' Reviewed-by. arch/x86/include/asm/syscall.h | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/arch/x86/include/asm/syscall.h b/arch/x86/include/asm/syscall.h index d653139857af..04fc5c120558 100644 --- a/arch/x86/include/asm/syscall.h +++ b/arch/x86/include/asm/syscall.h @@ -125,23 +125,30 @@ static inline void syscall_get_arguments(struct task_struct *task, case 0: if (!n--) break; *args++ = regs->bx; + /* fall through */ case 1: if (!n--) break; *args++ = regs->cx; + /* fall through */ case 2: if (!n--) break; *args++ = regs->dx; + /* fall through */ case 3: if (!n--) break; *args++ = regs->si; + /* fall through */ case 4: if (!n--) break; *args++ = regs->di; + /* fall through */ case 5: if (!n--) break; *args++ = regs->bp; + /* fall through */ case 6: if (!n--) break; + /* fall through */ default: BUG(); break; @@ -152,23 +159,30 @@ static inline void syscall_get_arguments(struct task_struct *task, case 0: if (!n--) break; *args++ = regs->di; + /* fall through */ case 1: if (!n--) break; *args++ = regs->si; + /* fall through */ case 2: if (!n--) break; *args++ = regs->dx; + /* fall through */ case 3: if (!n--) break; *args++ = regs->r10; + /* fall through */ case 4: if (!n--) break; *args++ = regs->r8; + /* fall through */ case 5: if (!n--) break; *args++ = regs->r9; + /* fall through */ case 6: if (!n--) break; + /* fall through */ default: BUG(); break; @@ -186,23 +200,30 @@ static inline void syscall_set_arguments(struct task_struct *task, case 0: if (!n--) break; regs->bx = *args++; + /* fall through */ case 1: if (!n--) break; regs->cx = *args++; + /* fall through */ case 2: if (!n--) break; regs->dx = *args++; + /* fall through */ case 3: if (!n--) break; regs->si = *args++; + /* fall through */ case 4: if (!n--) break; regs->di = *args++; + /* fall through */ case 5: if (!n--) break; regs->bp = *args++; + /* fall through */ case 6: if (!n--) break; + /* fall through */ default: BUG(); break; @@ -213,23 +234,30 @@ static inline void syscall_set_arguments(struct task_struct *task, case 0: if (!n--) break; regs->di = *args++; + /* fall through */ case 1: if (!n--) break; regs->si = *args++; + /* fall through */ case 2: if (!n--) break; regs->dx = *args++; + /* fall through */ case 3: if (!n--) break; regs->r10 = *args++; + /* fall through */ case 4: if (!n--) break; regs->r8 = *args++; + /* fall through */ case 5: if (!n--) break; regs->r9 = *args++; + /* fall through */ case 6: if (!n--) break; + /* fall through */ default: BUG(); break; -- 2.21.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs 2019-02-28 19:27 [PATCH v2] x86/syscalls: Mark expected switch fall-throughs Gustavo A. R. Silva @ 2019-03-23 16:14 ` Thomas Gleixner 2019-03-26 15:12 ` Oleg Nesterov 0 siblings, 1 reply; 13+ messages in thread From: Thomas Gleixner @ 2019-03-23 16:14 UTC (permalink / raw) To: Gustavo A. R. Silva Cc: Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, LKML, Dominik Brodowski, Andy Lutomirski, Kees Cook, Eric W. Biederman, Oleg Nesterov On Thu, 28 Feb 2019, Gustavo A. R. Silva wrote: > arch/x86/include/asm/syscall.h | 28 ++++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) Second thoughts. So this adds 28 /* fall through */ comments. Now I appreciate the effort, but can we pretty please look at the code in question and figure out whether the implementation makes sense in the first place before adding falltrough comments blindly? The whole exercise can be simplified. Untested patch below. Looking at that stuff makes me wonder about two things: 1) The third argument of get/set(), i.e. the argument offset, is 0 on all call sites. Do we need it at all? 2) syscall_set_arguments() has been introduced in 2008 and we still have no caller. Instead of polishing it, can it be removed completely or are there plans to actually use it? Thanks, tglx 8<---------------- arch/x86/include/asm/syscall.h | 174 +++++++++++++++-------------------------- 1 file changed, 64 insertions(+), 110 deletions(-) --- a/arch/x86/include/asm/syscall.h +++ b/arch/x86/include/asm/syscall.h @@ -114,126 +114,80 @@ static inline int syscall_get_arch(void) #else /* CONFIG_X86_64 */ +static inline unsigned long syscall_get_argreg(struct pt_regs *regs, + unsigned int idx) +{ + switch (idx) { + case 0: return regs->di; + case 1: return regs->si; + case 2: return regs->dx; + case 3: return regs->r10; + case 4: return regs->r8; + case 5: return regs->r9; +#ifdef CONFIG_IA32_EMULATION + case 6: return regs->bx; + case 7: return regs->cx; + case 8: return regs->dx; + case 9: return regs->si; + case 10: return regs->di; + case 11: return regs->bp; +#endif + } + return 0; +} + static inline void syscall_get_arguments(struct task_struct *task, struct pt_regs *regs, - unsigned int i, unsigned int n, + unsigned int idx, unsigned int cnt, unsigned long *args) { -# ifdef CONFIG_IA32_EMULATION - if (task->thread_info.status & TS_COMPAT) - switch (i) { - case 0: - if (!n--) break; - *args++ = regs->bx; - case 1: - if (!n--) break; - *args++ = regs->cx; - case 2: - if (!n--) break; - *args++ = regs->dx; - case 3: - if (!n--) break; - *args++ = regs->si; - case 4: - if (!n--) break; - *args++ = regs->di; - case 5: - if (!n--) break; - *args++ = regs->bp; - case 6: - if (!n--) break; - default: - BUG(); - break; - } - else -# endif - switch (i) { - case 0: - if (!n--) break; - *args++ = regs->di; - case 1: - if (!n--) break; - *args++ = regs->si; - case 2: - if (!n--) break; - *args++ = regs->dx; - case 3: - if (!n--) break; - *args++ = regs->r10; - case 4: - if (!n--) break; - *args++ = regs->r8; - case 5: - if (!n--) break; - *args++ = regs->r9; - case 6: - if (!n--) break; - default: - BUG(); - break; - } + if (WARN_ON((idx + cnt) > 6)) + return; + + if (IS_ENABLED(CONFIG_IA32_EMULATION) && + task->thread_info.status & TS_COMPAT) + idx += 6; + + for (; cnt > 0; cnt--) + *args++ = syscall_get_argreg(regs, idx++); +} + +static inline void syscall_set_argreg(struct pt_regs *regs, + unsigned int idx, + unsigned long val) +{ + switch (idx) { + case 0: regs->di = val; break; + case 1: regs->si = val; break; + case 2: regs->dx = val; break; + case 3: regs->r10 = val; break; + case 4: regs->r8 = val; break; + case 5: regs->r9 = val; break; +#ifdef CONFIG_IA32_EMULATION + case 6: regs->bx = val; break; + case 7: regs->cx = val; break; + case 8: regs->dx = val; break; + case 9: regs->si = val; break; + case 10: regs->di = val; break; + case 11: regs->bp = val; break; +#endif + } } static inline void syscall_set_arguments(struct task_struct *task, struct pt_regs *regs, - unsigned int i, unsigned int n, + unsigned int idx, unsigned int cnt, const unsigned long *args) { -# ifdef CONFIG_IA32_EMULATION - if (task->thread_info.status & TS_COMPAT) - switch (i) { - case 0: - if (!n--) break; - regs->bx = *args++; - case 1: - if (!n--) break; - regs->cx = *args++; - case 2: - if (!n--) break; - regs->dx = *args++; - case 3: - if (!n--) break; - regs->si = *args++; - case 4: - if (!n--) break; - regs->di = *args++; - case 5: - if (!n--) break; - regs->bp = *args++; - case 6: - if (!n--) break; - default: - BUG(); - break; - } - else -# endif - switch (i) { - case 0: - if (!n--) break; - regs->di = *args++; - case 1: - if (!n--) break; - regs->si = *args++; - case 2: - if (!n--) break; - regs->dx = *args++; - case 3: - if (!n--) break; - regs->r10 = *args++; - case 4: - if (!n--) break; - regs->r8 = *args++; - case 5: - if (!n--) break; - regs->r9 = *args++; - case 6: - if (!n--) break; - default: - BUG(); - break; - } + if (WARN_ON((idx + cnt) > 6)) + return; + + if (IS_ENABLED(CONFIG_IA32_EMULATION) && + task->thread_info.status & TS_COMPAT) + idx += 6; + + for (; cnt > 0; cnt--) + syscall_set_argreg(regs, idx++, *args++); } static inline int syscall_get_arch(void) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs 2019-03-23 16:14 ` Thomas Gleixner @ 2019-03-26 15:12 ` Oleg Nesterov 2019-03-26 16:09 ` Thomas Gleixner 2019-03-27 1:26 ` Dmitry V. Levin 0 siblings, 2 replies; 13+ messages in thread From: Oleg Nesterov @ 2019-03-26 15:12 UTC (permalink / raw) To: Thomas Gleixner, Steven Rostedt Cc: Gustavo A. R. Silva, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, LKML, Dominik Brodowski, Andy Lutomirski, Kees Cook, Eric W. Biederman On 03/23, Thomas Gleixner wrote: > > On Thu, 28 Feb 2019, Gustavo A. R. Silva wrote: > > > arch/x86/include/asm/syscall.h | 28 ++++++++++++++++++++++++++++ > > 1 file changed, 28 insertions(+) > > Second thoughts. So this adds 28 /* fall through */ comments. Now I > appreciate the effort, but can we pretty please look at the code in > question and figure out whether the implementation makes sense in the first > place before adding falltrough comments blindly? > > The whole exercise can be simplified. Untested patch below. > > Looking at that stuff makes me wonder about two things: > > 1) The third argument of get/set(), i.e. the argument offset, is 0 on all > call sites. Do we need it at all? Probably "maxargs" can be removed too, Steven sent the patches a long ago, see https://lore.kernel.org/lkml/20161107212634.529267342@goodmis.org/ > 2) syscall_set_arguments() has been introduced in 2008 and we still have > no caller. Instead of polishing it, can it be removed completely or are > there plans to actually use it? I think it can die. > > Thanks, > > tglx > > 8<---------------- > > arch/x86/include/asm/syscall.h | 174 +++++++++++++++-------------------------- > 1 file changed, 64 insertions(+), 110 deletions(-) > > --- a/arch/x86/include/asm/syscall.h > +++ b/arch/x86/include/asm/syscall.h > @@ -114,126 +114,80 @@ static inline int syscall_get_arch(void) > > #else /* CONFIG_X86_64 */ > > +static inline unsigned long syscall_get_argreg(struct pt_regs *regs, > + unsigned int idx) > +{ > + switch (idx) { > + case 0: return regs->di; > + case 1: return regs->si; > + case 2: return regs->dx; > + case 3: return regs->r10; > + case 4: return regs->r8; > + case 5: return regs->r9; > +#ifdef CONFIG_IA32_EMULATION > + case 6: return regs->bx; > + case 7: return regs->cx; > + case 8: return regs->dx; > + case 9: return regs->si; > + case 10: return regs->di; > + case 11: return regs->bp; > +#endif > + } > + return 0; > +} > + > static inline void syscall_get_arguments(struct task_struct *task, > struct pt_regs *regs, > - unsigned int i, unsigned int n, > + unsigned int idx, unsigned int cnt, > unsigned long *args) > { > -# ifdef CONFIG_IA32_EMULATION > - if (task->thread_info.status & TS_COMPAT) > - switch (i) { > - case 0: > - if (!n--) break; > - *args++ = regs->bx; > - case 1: > - if (!n--) break; > - *args++ = regs->cx; > - case 2: > - if (!n--) break; > - *args++ = regs->dx; > - case 3: > - if (!n--) break; > - *args++ = regs->si; > - case 4: > - if (!n--) break; > - *args++ = regs->di; > - case 5: > - if (!n--) break; > - *args++ = regs->bp; > - case 6: > - if (!n--) break; > - default: > - BUG(); > - break; > - } > - else > -# endif > - switch (i) { > - case 0: > - if (!n--) break; > - *args++ = regs->di; > - case 1: > - if (!n--) break; > - *args++ = regs->si; > - case 2: > - if (!n--) break; > - *args++ = regs->dx; > - case 3: > - if (!n--) break; > - *args++ = regs->r10; > - case 4: > - if (!n--) break; > - *args++ = regs->r8; > - case 5: > - if (!n--) break; > - *args++ = regs->r9; > - case 6: > - if (!n--) break; > - default: > - BUG(); > - break; > - } > + if (WARN_ON((idx + cnt) > 6)) > + return; > + > + if (IS_ENABLED(CONFIG_IA32_EMULATION) && > + task->thread_info.status & TS_COMPAT) > + idx += 6; > + > + for (; cnt > 0; cnt--) > + *args++ = syscall_get_argreg(regs, idx++); > +} > + > +static inline void syscall_set_argreg(struct pt_regs *regs, > + unsigned int idx, > + unsigned long val) > +{ > + switch (idx) { > + case 0: regs->di = val; break; > + case 1: regs->si = val; break; > + case 2: regs->dx = val; break; > + case 3: regs->r10 = val; break; > + case 4: regs->r8 = val; break; > + case 5: regs->r9 = val; break; > +#ifdef CONFIG_IA32_EMULATION > + case 6: regs->bx = val; break; > + case 7: regs->cx = val; break; > + case 8: regs->dx = val; break; > + case 9: regs->si = val; break; > + case 10: regs->di = val; break; > + case 11: regs->bp = val; break; > +#endif > + } > } > > static inline void syscall_set_arguments(struct task_struct *task, > struct pt_regs *regs, > - unsigned int i, unsigned int n, > + unsigned int idx, unsigned int cnt, > const unsigned long *args) > { > -# ifdef CONFIG_IA32_EMULATION > - if (task->thread_info.status & TS_COMPAT) > - switch (i) { > - case 0: > - if (!n--) break; > - regs->bx = *args++; > - case 1: > - if (!n--) break; > - regs->cx = *args++; > - case 2: > - if (!n--) break; > - regs->dx = *args++; > - case 3: > - if (!n--) break; > - regs->si = *args++; > - case 4: > - if (!n--) break; > - regs->di = *args++; > - case 5: > - if (!n--) break; > - regs->bp = *args++; > - case 6: > - if (!n--) break; > - default: > - BUG(); > - break; > - } > - else > -# endif > - switch (i) { > - case 0: > - if (!n--) break; > - regs->di = *args++; > - case 1: > - if (!n--) break; > - regs->si = *args++; > - case 2: > - if (!n--) break; > - regs->dx = *args++; > - case 3: > - if (!n--) break; > - regs->r10 = *args++; > - case 4: > - if (!n--) break; > - regs->r8 = *args++; > - case 5: > - if (!n--) break; > - regs->r9 = *args++; > - case 6: > - if (!n--) break; > - default: > - BUG(); > - break; > - } > + if (WARN_ON((idx + cnt) > 6)) > + return; > + > + if (IS_ENABLED(CONFIG_IA32_EMULATION) && > + task->thread_info.status & TS_COMPAT) > + idx += 6; > + > + for (; cnt > 0; cnt--) > + syscall_set_argreg(regs, idx++, *args++); > } > > static inline int syscall_get_arch(void) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs 2019-03-26 15:12 ` Oleg Nesterov @ 2019-03-26 16:09 ` Thomas Gleixner 2019-03-26 16:16 ` Steven Rostedt 2019-03-27 1:26 ` Dmitry V. Levin 1 sibling, 1 reply; 13+ messages in thread From: Thomas Gleixner @ 2019-03-26 16:09 UTC (permalink / raw) To: Oleg Nesterov Cc: Steven Rostedt, Gustavo A. R. Silva, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, LKML, Dominik Brodowski, Andy Lutomirski, Kees Cook, Eric W. Biederman On Tue, 26 Mar 2019, Oleg Nesterov wrote: > On 03/23, Thomas Gleixner wrote: > > > > On Thu, 28 Feb 2019, Gustavo A. R. Silva wrote: > > > > > arch/x86/include/asm/syscall.h | 28 ++++++++++++++++++++++++++++ > > > 1 file changed, 28 insertions(+) > > > > Second thoughts. So this adds 28 /* fall through */ comments. Now I > > appreciate the effort, but can we pretty please look at the code in > > question and figure out whether the implementation makes sense in the first > > place before adding falltrough comments blindly? > > > > The whole exercise can be simplified. Untested patch below. > > > > Looking at that stuff makes me wonder about two things: > > > > 1) The third argument of get/set(), i.e. the argument offset, is 0 on all > > call sites. Do we need it at all? > > Probably "maxargs" can be removed too, Steven sent the patches a long ago, see > https://lore.kernel.org/lkml/20161107212634.529267342@goodmis.org/ Indeed. We should resurrect them. > > 2) syscall_set_arguments() has been introduced in 2008 and we still have > > no caller. Instead of polishing it, can it be removed completely or are > > there plans to actually use it? > > I think it can die. Good. Removed code is the least buggy code :) Gustavo, it would be really appreciated if you could take care of that, unless Steven wants to polish his old set up himself. If you have no cycles, please let us know. Thanks, tglx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs 2019-03-26 16:09 ` Thomas Gleixner @ 2019-03-26 16:16 ` Steven Rostedt 2019-03-26 16:17 ` Thomas Gleixner 0 siblings, 1 reply; 13+ messages in thread From: Steven Rostedt @ 2019-03-26 16:16 UTC (permalink / raw) To: Thomas Gleixner Cc: Oleg Nesterov, Gustavo A. R. Silva, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, LKML, Dominik Brodowski, Andy Lutomirski, Kees Cook, Eric W. Biederman On Tue, 26 Mar 2019 17:09:44 +0100 (CET) Thomas Gleixner <tglx@linutronix.de> wrote: > > > 1) The third argument of get/set(), i.e. the argument offset, is 0 on all > > > call sites. Do we need it at all? > > > > Probably "maxargs" can be removed too, Steven sent the patches a long ago, see > > https://lore.kernel.org/lkml/20161107212634.529267342@goodmis.org/ > > Indeed. We should resurrect them. > > > > 2) syscall_set_arguments() has been introduced in 2008 and we still have > > > no caller. Instead of polishing it, can it be removed completely or are > > > there plans to actually use it? > > > > I think it can die. > > Good. Removed code is the least buggy code :) > > Gustavo, it would be really appreciated if you could take care of that, > unless Steven wants to polish his old set up himself. If you have no > cycles, please let us know. I still have those patches in my quilt queue. I can polish them up and resend. -- Steve ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs 2019-03-26 16:16 ` Steven Rostedt @ 2019-03-26 16:17 ` Thomas Gleixner 0 siblings, 0 replies; 13+ messages in thread From: Thomas Gleixner @ 2019-03-26 16:17 UTC (permalink / raw) To: Steven Rostedt Cc: Oleg Nesterov, Gustavo A. R. Silva, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, LKML, Dominik Brodowski, Andy Lutomirski, Kees Cook, Eric W. Biederman On Tue, 26 Mar 2019, Steven Rostedt wrote: > On Tue, 26 Mar 2019 17:09:44 +0100 (CET) > Thomas Gleixner <tglx@linutronix.de> wrote: > > > > > 1) The third argument of get/set(), i.e. the argument offset, is 0 on all > > > > call sites. Do we need it at all? > > > > > > Probably "maxargs" can be removed too, Steven sent the patches a long ago, see > > > https://lore.kernel.org/lkml/20161107212634.529267342@goodmis.org/ > > > > Indeed. We should resurrect them. > > > > > > 2) syscall_set_arguments() has been introduced in 2008 and we still have > > > > no caller. Instead of polishing it, can it be removed completely or are > > > > there plans to actually use it? > > > > > > I think it can die. > > > > Good. Removed code is the least buggy code :) > > > > Gustavo, it would be really appreciated if you could take care of that, > > unless Steven wants to polish his old set up himself. If you have no > > cycles, please let us know. > > I still have those patches in my quilt queue. I can polish them up and > resend. Appreciated. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs 2019-03-26 15:12 ` Oleg Nesterov 2019-03-26 16:09 ` Thomas Gleixner @ 2019-03-27 1:26 ` Dmitry V. Levin 2019-03-27 14:29 ` Thomas Gleixner 1 sibling, 1 reply; 13+ messages in thread From: Dmitry V. Levin @ 2019-03-27 1:26 UTC (permalink / raw) To: Oleg Nesterov Cc: Thomas Gleixner, Steven Rostedt, Gustavo A. R. Silva, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, LKML, Dominik Brodowski, Andy Lutomirski, Kees Cook, Eric W. Biederman [-- Attachment #1: Type: text/plain, Size: 519 bytes --] On Tue, Mar 26, 2019 at 04:12:45PM +0100, Oleg Nesterov wrote: > On 03/23, Thomas Gleixner wrote: [...] > > 2) syscall_set_arguments() has been introduced in 2008 and we still have > > no caller. Instead of polishing it, can it be removed completely or are > > there plans to actually use it? > > I think it can die. When PTRACE_GET_SYSCALL_INFO is finally squeezed into the kernel, we could discuss adding PTRACE_SET_SYSCALL_INFO as well, and it will need syscall_set_arguments(). -- ldv [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs 2019-03-27 1:26 ` Dmitry V. Levin @ 2019-03-27 14:29 ` Thomas Gleixner 2019-03-27 22:20 ` Dmitry V. Levin 0 siblings, 1 reply; 13+ messages in thread From: Thomas Gleixner @ 2019-03-27 14:29 UTC (permalink / raw) To: Dmitry V. Levin Cc: Oleg Nesterov, Steven Rostedt, Gustavo A. R. Silva, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, LKML, Dominik Brodowski, Andy Lutomirski, Kees Cook, Eric W. Biederman On Wed, 27 Mar 2019, Dmitry V. Levin wrote: > On Tue, Mar 26, 2019 at 04:12:45PM +0100, Oleg Nesterov wrote: > > On 03/23, Thomas Gleixner wrote: > [...] > > > 2) syscall_set_arguments() has been introduced in 2008 and we still have > > > no caller. Instead of polishing it, can it be removed completely or are > > > there plans to actually use it? > > > > I think it can die. > > When PTRACE_GET_SYSCALL_INFO is finally squeezed into the kernel, > we could discuss adding PTRACE_SET_SYSCALL_INFO as well, and it > will need syscall_set_arguments(). So if that ever happens, then adding the code back isn't rocket science. But if not, then there is no point in carrying the dead horse around another 11 years. Thanks, tglx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs 2019-03-27 14:29 ` Thomas Gleixner @ 2019-03-27 22:20 ` Dmitry V. Levin 2019-03-27 22:52 ` Thomas Gleixner 0 siblings, 1 reply; 13+ messages in thread From: Dmitry V. Levin @ 2019-03-27 22:20 UTC (permalink / raw) To: Thomas Gleixner Cc: Oleg Nesterov, Steven Rostedt, Gustavo A. R. Silva, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, LKML, Dominik Brodowski, Andy Lutomirski, Kees Cook, Eric W. Biederman [-- Attachment #1: Type: text/plain, Size: 1181 bytes --] On Wed, Mar 27, 2019 at 03:29:16PM +0100, Thomas Gleixner wrote: > On Wed, 27 Mar 2019, Dmitry V. Levin wrote: > > On Tue, Mar 26, 2019 at 04:12:45PM +0100, Oleg Nesterov wrote: > > > On 03/23, Thomas Gleixner wrote: > > [...] > > > > 2) syscall_set_arguments() has been introduced in 2008 and we still have > > > > no caller. Instead of polishing it, can it be removed completely or are > > > > there plans to actually use it? > > > > > > I think it can die. > > > > When PTRACE_GET_SYSCALL_INFO is finally squeezed into the kernel, > > we could discuss adding PTRACE_SET_SYSCALL_INFO as well, and it > > will need syscall_set_arguments(). > > So if that ever happens, then adding the code back isn't rocket > science. But if not, then there is no point in carrying the dead horse > around another 11 years. Given that it took me roughly 4 months to get a relatively simple revert of commit 5e937a9ae913 accepted into linux-next, adding the code back might be time-consuming. Could we delay the removal of syscall_set_arguments() until PTRACE_GET_SYSCALL_INFO is merged into the kernel? I hope it won't take another 11 years. -- ldv [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs 2019-03-27 22:20 ` Dmitry V. Levin @ 2019-03-27 22:52 ` Thomas Gleixner 2019-03-27 23:03 ` Steven Rostedt 2019-03-27 23:12 ` Dmitry V. Levin 0 siblings, 2 replies; 13+ messages in thread From: Thomas Gleixner @ 2019-03-27 22:52 UTC (permalink / raw) To: Dmitry V. Levin Cc: Oleg Nesterov, Steven Rostedt, Gustavo A. R. Silva, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, LKML, Dominik Brodowski, Andy Lutomirski, Kees Cook, Eric W. Biederman On Thu, 28 Mar 2019, Dmitry V. Levin wrote: > On Wed, Mar 27, 2019 at 03:29:16PM +0100, Thomas Gleixner wrote: > > On Wed, 27 Mar 2019, Dmitry V. Levin wrote: > > > On Tue, Mar 26, 2019 at 04:12:45PM +0100, Oleg Nesterov wrote: > > > > On 03/23, Thomas Gleixner wrote: > > > [...] > > > > > 2) syscall_set_arguments() has been introduced in 2008 and we still have > > > > > no caller. Instead of polishing it, can it be removed completely or are > > > > > there plans to actually use it? > > > > > > > > I think it can die. > > > > > > When PTRACE_GET_SYSCALL_INFO is finally squeezed into the kernel, > > > we could discuss adding PTRACE_SET_SYSCALL_INFO as well, and it > > > will need syscall_set_arguments(). > > > > So if that ever happens, then adding the code back isn't rocket > > science. But if not, then there is no point in carrying the dead horse > > around another 11 years. > > Given that it took me roughly 4 months to get a relatively simple revert > of commit 5e937a9ae913 accepted into linux-next, adding the code back > might be time-consuming. > > Could we delay the removal of syscall_set_arguments() until > PTRACE_GET_SYSCALL_INFO is merged into the kernel? > I hope it won't take another 11 years. Hope dies last :) Seriously. If we keep it can we at least remove all the unused arguments which we have on both functions to simplify the whole mess? Thanks, tglx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs 2019-03-27 22:52 ` Thomas Gleixner @ 2019-03-27 23:03 ` Steven Rostedt 2019-03-27 23:12 ` Dmitry V. Levin 1 sibling, 0 replies; 13+ messages in thread From: Steven Rostedt @ 2019-03-27 23:03 UTC (permalink / raw) To: Thomas Gleixner Cc: Dmitry V. Levin, Oleg Nesterov, Gustavo A. R. Silva, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, LKML, Dominik Brodowski, Andy Lutomirski, Kees Cook, Eric W. Biederman On Wed, 27 Mar 2019 23:52:19 +0100 (CET) Thomas Gleixner <tglx@linutronix.de> wrote: > > Could we delay the removal of syscall_set_arguments() until > > PTRACE_GET_SYSCALL_INFO is merged into the kernel? > > I hope it won't take another 11 years. > > Hope dies last :) > > Seriously. If we keep it can we at least remove all the unused arguments > which we have on both functions to simplify the whole mess? I've finished forward porting my old patches, and was about to just remove that function. But instead, I'll make it identical to the set_get_arguments(). I have a bit more testing to do before I post the result. -- Steve ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs 2019-03-27 22:52 ` Thomas Gleixner 2019-03-27 23:03 ` Steven Rostedt @ 2019-03-27 23:12 ` Dmitry V. Levin 2019-03-27 23:15 ` Steven Rostedt 1 sibling, 1 reply; 13+ messages in thread From: Dmitry V. Levin @ 2019-03-27 23:12 UTC (permalink / raw) To: Thomas Gleixner Cc: Oleg Nesterov, Steven Rostedt, Gustavo A. R. Silva, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, LKML, Dominik Brodowski, Andy Lutomirski, Kees Cook, Eric W. Biederman [-- Attachment #1: Type: text/plain, Size: 1911 bytes --] On Wed, Mar 27, 2019 at 11:52:19PM +0100, Thomas Gleixner wrote: > On Thu, 28 Mar 2019, Dmitry V. Levin wrote: > > On Wed, Mar 27, 2019 at 03:29:16PM +0100, Thomas Gleixner wrote: > > > On Wed, 27 Mar 2019, Dmitry V. Levin wrote: > > > > On Tue, Mar 26, 2019 at 04:12:45PM +0100, Oleg Nesterov wrote: > > > > > On 03/23, Thomas Gleixner wrote: > > > > [...] > > > > > > 2) syscall_set_arguments() has been introduced in 2008 and we still have > > > > > > no caller. Instead of polishing it, can it be removed completely or are > > > > > > there plans to actually use it? > > > > > > > > > > I think it can die. > > > > > > > > When PTRACE_GET_SYSCALL_INFO is finally squeezed into the kernel, > > > > we could discuss adding PTRACE_SET_SYSCALL_INFO as well, and it > > > > will need syscall_set_arguments(). > > > > > > So if that ever happens, then adding the code back isn't rocket > > > science. But if not, then there is no point in carrying the dead horse > > > around another 11 years. > > > > Given that it took me roughly 4 months to get a relatively simple revert > > of commit 5e937a9ae913 accepted into linux-next, adding the code back > > might be time-consuming. > > > > Could we delay the removal of syscall_set_arguments() until > > PTRACE_GET_SYSCALL_INFO is merged into the kernel? > > I hope it won't take another 11 years. > > Hope dies last :) > > Seriously. If we keep it can we at least remove all the unused arguments > which we have on both functions to simplify the whole mess? In case of syscall_set_arguments() I think we can safely remove "i" and "n" arguments assuming i == 0 and n == 6. All I can say about syscall_get_arguments() is that - all current users invoke it with i == 0, - all current users that invoke it with n != 6 are in kernel/trace/trace_syscalls.c so it may actually be invoked with n < 6. -- ldv [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs 2019-03-27 23:12 ` Dmitry V. Levin @ 2019-03-27 23:15 ` Steven Rostedt 0 siblings, 0 replies; 13+ messages in thread From: Steven Rostedt @ 2019-03-27 23:15 UTC (permalink / raw) To: Dmitry V. Levin Cc: Thomas Gleixner, Oleg Nesterov, Gustavo A. R. Silva, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, LKML, Dominik Brodowski, Andy Lutomirski, Kees Cook, Eric W. Biederman On Thu, 28 Mar 2019 02:12:15 +0300 "Dmitry V. Levin" <ldv@altlinux.org> wrote: > > Seriously. If we keep it can we at least remove all the unused arguments > > which we have on both functions to simplify the whole mess? > > In case of syscall_set_arguments() I think we can safely remove > "i" and "n" arguments assuming i == 0 and n == 6. > > All I can say about syscall_get_arguments() is that > - all current users invoke it with i == 0, > - all current users that invoke it with n != 6 are in kernel/trace/trace_syscalls.c > so it may actually be invoked with n < 6. Yes, that's what my old (and current) patches address. I removed the i,n parameters and make it 0,6 hardcoded in the routines. Patches will be out soon. -- Steve ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-03-27 23:15 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-28 19:27 [PATCH v2] x86/syscalls: Mark expected switch fall-throughs Gustavo A. R. Silva 2019-03-23 16:14 ` Thomas Gleixner 2019-03-26 15:12 ` Oleg Nesterov 2019-03-26 16:09 ` Thomas Gleixner 2019-03-26 16:16 ` Steven Rostedt 2019-03-26 16:17 ` Thomas Gleixner 2019-03-27 1:26 ` Dmitry V. Levin 2019-03-27 14:29 ` Thomas Gleixner 2019-03-27 22:20 ` Dmitry V. Levin 2019-03-27 22:52 ` Thomas Gleixner 2019-03-27 23:03 ` Steven Rostedt 2019-03-27 23:12 ` Dmitry V. Levin 2019-03-27 23:15 ` Steven Rostedt
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).