Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs
On Thu, 28 Mar 2019 02:12:15 +0300 "Dmitry V. Levin" 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
Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs
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 signature.asc Description: PGP signature
Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs
On Wed, 27 Mar 2019 23:52:19 +0100 (CET) Thomas Gleixner 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
Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs
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
Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs
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 signature.asc Description: PGP signature
Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs
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
Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs
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 signature.asc Description: PGP signature
Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs
On Tue, 26 Mar 2019, Steven Rostedt wrote: > On Tue, 26 Mar 2019 17:09:44 +0100 (CET) > Thomas Gleixner 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.529267...@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.
Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs
On Tue, 26 Mar 2019 17:09:44 +0100 (CET) Thomas Gleixner 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.529267...@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
Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs
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.529267...@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
Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs
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.529267...@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:
Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs
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
+
