mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Alexey Brodkin <Alexey.Brodkin@synopsys.com>
To: linux-snps-arc@lists.infradead.org
Cc: linux-kernel@vger.kernel.org,
	Vineet Gupta <Vineet.Gupta1@synopsys.com>,
	Alexey Brodkin <Alexey.Brodkin@synopsys.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Max Filippov <jcmvbkbc@gmail.com>,
	linux-arch@vger.kernel.org
Subject: [PATCH] ARC: Improve cmpxchng syscall implementation
Date: Mon, 19 Mar 2018 14:00:02 +0300	[thread overview]
Message-ID: <20180319110002.27419-1-abrodkin@synopsys.com> (raw)

arc_usr_cmpxchg syscall is supposed to be used on platforms
that lack support of Load-Locked/Store-Conditional instructions
in hardware. And in that case we mimic missing hardware features
with help of kernel's sycall that "atomically" checks current
value in memory and then if it matches caller expectation new
value is written to that same location.

What's important in the description above:
 - Check-and-exchange must be "atomical" which means
   preemption must be disabled during entire "transaction"
 - Data accessed is from user-space, i.e. we're dealing
   with virtual addresses

And in current implementation we have a couple of problems:

1. We do disable preemprion around __get_user() & __put_user()
   but that in its turn disables page fault handler.
   That means if a pointer to user's data has no mapping in
   the TLB we won't be able to access required data.
   Instead software "exception handling" code from __get_user_fn()
   will return -EFAULT.

2. What's worse if we're dealing with data from not yet allocated
   page (think of pre-copy-on-write state) we'll successfully
   read data but on write we'll silently return to user-space
   with correct result (which we really read just before). That leads
   to very strange problems in user-space app further down the line
   because new value was never written to the destination.

3. Regardless of what went wrong we'll return from syscall
   and user-space application will continue to execute.
   Even if user's pointer was completely bogus.
   In case of hardware LL/SC that app would have been killed
   by the kernel.

With that change we attempt to imrove on all 3 items above:

1. We still disable preemption around read-and-write of
   user's data but if we happen to fail with either of them
   we're enabling preemption and try to force page fault so
   that we have a correct mapping in the TLB. Then re-try
   again in "atomic" context.

2. If real page fault fails or even access_ok() returns false
   we send SIGSEGV to the user-space process so if something goes
   seriously wrong we'll know about it much earlier.

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Vineet Gupta <vgupta@synopsys.com>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Cc: linux-arch@vger.kernel.org
---
 arch/arc/kernel/process.c | 47 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/arch/arc/kernel/process.c b/arch/arc/kernel/process.c
index 5ac3b547453f..d7d3e16133d6 100644
--- a/arch/arc/kernel/process.c
+++ b/arch/arc/kernel/process.c
@@ -47,7 +47,9 @@ SYSCALL_DEFINE0(arc_gettls)
 SYSCALL_DEFINE3(arc_usr_cmpxchg, int *, uaddr, int, expected, int, new)
 {
 	struct pt_regs *regs = current_pt_regs();
-	int uval = -EFAULT;
+	struct page *page;
+	u32 val;
+	int ret;
 
 	/*
 	 * This is only for old cores lacking LLOCK/SCOND, which by defintion
@@ -60,23 +62,48 @@ SYSCALL_DEFINE3(arc_usr_cmpxchg, int *, uaddr, int, expected, int, new)
 	/* Z indicates to userspace if operation succeded */
 	regs->status32 &= ~STATUS_Z_MASK;
 
-	if (!access_ok(VERIFY_WRITE, uaddr, sizeof(int)))
-		return -EFAULT;
+	ret = access_ok(VERIFY_WRITE, uaddr, sizeof(*uaddr));
+	if (!ret)
+		goto fail;
 
+again:
 	preempt_disable();
 
-	if (__get_user(uval, uaddr))
-		goto done;
-
-	if (uval == expected) {
-		if (!__put_user(new, uaddr))
+	ret = __get_user(val, uaddr);
+	if (ret == -EFAULT) {
+		preempt_enable();
+		ret = get_user_pages_fast((unsigned long)uaddr, 1, 1, &page);
+		if (ret < 0)
+			goto fail;
+
+		put_page(page);
+		goto again;
+	} else if (ret)
+		goto fail;
+
+	if (val == expected) {
+		ret = __put_user(new, uaddr);
+		if (!ret)
 			regs->status32 |= STATUS_Z_MASK;
 	}
 
-done:
 	preempt_enable();
 
-	return uval;
+	if (ret == -EFAULT) {
+		ret = get_user_pages_fast((unsigned long)uaddr, 1, 1, &page);
+		if (ret < 0)
+			goto fail;
+
+		put_page(page);
+		goto again;
+	} else if (ret)
+		goto fail;
+
+	return val;
+
+fail:
+	force_sig(SIGSEGV, current);
+	return ret;
 }
 
 #ifdef CONFIG_ISA_ARCV2
-- 
2.11.0

             reply	other threads:[~2018-03-19 11:00 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-19 11:00 Alexey Brodkin [this message]
2018-03-19 18:29 ` Vineet Gupta
2018-03-21 11:54   ` Alexey Brodkin
2018-04-04  8:56     ` Alexey Brodkin
2018-04-18 18:16     ` Vineet Gupta
2018-06-19  7:58       ` Alexey Brodkin
2018-06-19  9:26 ` Peter Zijlstra

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=20180319110002.27419-1-abrodkin@synopsys.com \
    --to=alexey.brodkin@synopsys.com \
    --cc=Vineet.Gupta1@synopsys.com \
    --cc=jcmvbkbc@gmail.com \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-snps-arc@lists.infradead.org \
    --cc=peterz@infradead.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

all inboxes | Powered by JetHome®