[PATCH v2] x86/syscalls: Mark expected switch fall-throughs

[PATCH v2] x86/syscalls: Mark expected switch fall-throughs

[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

Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs

[Thomas Gleixner]

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)

Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs

[Oleg Nesterov]

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)

Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs

[Thomas Gleixner]

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

Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs

[Steven Rostedt]

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

Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs

[Thomas Gleixner]

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.

Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs

[Dmitry V. Levin]

[-- 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 --]

Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs

[Thomas Gleixner]

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

[Dmitry V. Levin]

[-- 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 --]

Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs

[Thomas Gleixner]

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

[Steven Rostedt]

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

Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs

[Dmitry V. Levin]

[-- 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 --]

Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs

[Steven Rostedt]

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