linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
Cc: Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	"H. Peter Anvin" <hpa@zytor.com>,
	x86@kernel.org, LKML <linux-kernel@vger.kernel.org>,
	Dominik Brodowski <linux@dominikbrodowski.net>,
	Andy Lutomirski <luto@kernel.org>,
	Kees Cook <keescook@chromium.org>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	Oleg Nesterov <oleg@redhat.com>
Subject: Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs
Date: Sat, 23 Mar 2019 17:14:20 +0100 (CET)	[thread overview]
Message-ID: <alpine.DEB.2.21.1903231457590.1798@nanos.tec.linutronix.de> (raw)
In-Reply-To: <20190228192746.GA13021@embeddedor>

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)

  reply	other threads:[~2019-03-23 16:14 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.21.1903231457590.1798@nanos.tec.linutronix.de \
    --to=tglx@linutronix.de \
    --cc=bp@alien8.de \
    --cc=ebiederm@xmission.com \
    --cc=gustavo@embeddedor.com \
    --cc=hpa@zytor.com \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@dominikbrodowski.net \
    --cc=luto@kernel.org \
    --cc=mingo@redhat.com \
    --cc=oleg@redhat.com \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
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).