From: Sasha Levin <sashal@kernel.org>
To: linux-kernel@vger.kernel.org, stable@vger.kernel.org
Cc: Al Viro <viro@zeniv.linux.org.uk>,
Guo Ren <guoren@linux.alibaba.com>,
Sasha Levin <sashal@kernel.org>
Subject: [PATCH AUTOSEL 5.4 30/32] csky: Fixup raw_copy_from_user()
Date: Fri, 22 May 2020 10:50:42 -0400 [thread overview]
Message-ID: <20200522145044.434677-30-sashal@kernel.org> (raw)
In-Reply-To: <20200522145044.434677-1-sashal@kernel.org>
From: Al Viro <viro@zeniv.linux.org.uk>
[ Upstream commit 51bb38cb78363fdad1f89e87357b7bc73e39ba88 ]
If raw_copy_from_user(to, from, N) returns K, callers expect
the first N - K bytes starting at to to have been replaced with
the contents of corresponding area starting at from and the last
K bytes of destination *left* *unmodified*.
What arch/sky/lib/usercopy.c is doing is broken - it can lead to e.g.
data corruption on write(2).
raw_copy_to_user() is inaccurate about return value, which is a bug,
but consequences are less drastic than for raw_copy_from_user().
And just what are those access_ok() doing in there? I mean, look into
linux/uaccess.h; that's where we do that check (as well as zero tail
on failure in the callers that need zeroing).
AFAICS, all of that shouldn't be hard to fix; something like a patch
below might make a useful starting point.
I would suggest moving these macros into usercopy.c (they are never
used anywhere else) and possibly expanding them there; if you leave
them alive, please at least rename __copy_user_zeroing(). Again,
it must not zero anything on failed read.
Said that, I'm not sure we won't be better off simply turning
usercopy.c into usercopy.S - all that is left there is a couple of
functions, each consisting only of inline asm.
Guo Ren reply:
Yes, raw_copy_from_user is wrong, it's no need zeroing code.
unsigned long _copy_from_user(void *to, const void __user *from,
unsigned long n)
{
unsigned long res = n;
might_fault();
if (likely(access_ok(from, n))) {
kasan_check_write(to, n);
res = raw_copy_from_user(to, from, n);
}
if (unlikely(res))
memset(to + (n - res), 0, res);
return res;
}
EXPORT_SYMBOL(_copy_from_user);
You are right and access_ok() should be removed.
but, how about:
do {
...
"2: stw %3, (%1, 0) \n" \
+ " subi %0, 4 \n" \
"9: stw %4, (%1, 4) \n" \
+ " subi %0, 4 \n" \
"10: stw %5, (%1, 8) \n" \
+ " subi %0, 4 \n" \
"11: stw %6, (%1, 12) \n" \
+ " subi %0, 4 \n" \
" addi %2, 16 \n" \
" addi %1, 16 \n" \
Don't expand __ex_table
AI Viro reply:
Hey, I've no idea about the instruction scheduling on csky -
if that doesn't slow the things down, all the better. It's just
that copy_to_user() and friends are on fairly hot codepaths,
and in quite a few situations they will dominate the speed of
e.g. read(2). So I tried to keep the fast path unchanged.
Up to the architecture maintainers, obviously. Which would be
you...
As for the fixups size increase (__ex_table size is unchanged)...
You have each of those macros expanded exactly once.
So the size is not a serious argument, IMO - useless complexity
would be, if it is, in fact, useless; the size... not really,
especially since those extra subi will at least offset it.
Again, up to you - asm optimizations of (essentially)
memcpy()-style loops are tricky and can depend upon the
fairly subtle details of architecture. So even on something
I know reasonably well I would resort to direct experiments
if I can't pass the buck to architecture maintainers.
It *is* worth optimizing - this is where read() from a file
that is already in page cache spends most of the time, etc.
Guo Ren reply:
Thx, after fixup some typo “sub %0, 4”, apply the patch.
TODO:
- user copy/from codes are still need optimizing.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
arch/csky/include/asm/uaccess.h | 49 +++++++++++++++++----------------
arch/csky/lib/usercopy.c | 8 ++----
2 files changed, 28 insertions(+), 29 deletions(-)
diff --git a/arch/csky/include/asm/uaccess.h b/arch/csky/include/asm/uaccess.h
index eaa1c3403a42..60f8a4112588 100644
--- a/arch/csky/include/asm/uaccess.h
+++ b/arch/csky/include/asm/uaccess.h
@@ -254,7 +254,7 @@ do { \
extern int __get_user_bad(void);
-#define __copy_user(to, from, n) \
+#define ___copy_to_user(to, from, n) \
do { \
int w0, w1, w2, w3; \
asm volatile( \
@@ -289,31 +289,34 @@ do { \
" subi %0, 4 \n" \
" br 3b \n" \
"5: cmpnei %0, 0 \n" /* 1B */ \
- " bf 8f \n" \
+ " bf 13f \n" \
" ldb %3, (%2, 0) \n" \
"6: stb %3, (%1, 0) \n" \
" addi %2, 1 \n" \
" addi %1, 1 \n" \
" subi %0, 1 \n" \
" br 5b \n" \
- "7: br 8f \n" \
+ "7: subi %0, 4 \n" \
+ "8: subi %0, 4 \n" \
+ "12: subi %0, 4 \n" \
+ " br 13f \n" \
".section __ex_table, \"a\" \n" \
".align 2 \n" \
- ".long 2b, 7b \n" \
- ".long 9b, 7b \n" \
- ".long 10b, 7b \n" \
+ ".long 2b, 13f \n" \
+ ".long 4b, 13f \n" \
+ ".long 6b, 13f \n" \
+ ".long 9b, 12b \n" \
+ ".long 10b, 8b \n" \
".long 11b, 7b \n" \
- ".long 4b, 7b \n" \
- ".long 6b, 7b \n" \
".previous \n" \
- "8: \n" \
+ "13: \n" \
: "=r"(n), "=r"(to), "=r"(from), "=r"(w0), \
"=r"(w1), "=r"(w2), "=r"(w3) \
: "0"(n), "1"(to), "2"(from) \
: "memory"); \
} while (0)
-#define __copy_user_zeroing(to, from, n) \
+#define ___copy_from_user(to, from, n) \
do { \
int tmp; \
int nsave; \
@@ -356,22 +359,22 @@ do { \
" addi %1, 1 \n" \
" subi %0, 1 \n" \
" br 5b \n" \
- "8: mov %3, %0 \n" \
- " movi %4, 0 \n" \
- "9: stb %4, (%1, 0) \n" \
- " addi %1, 1 \n" \
- " subi %3, 1 \n" \
- " cmpnei %3, 0 \n" \
- " bt 9b \n" \
- " br 7f \n" \
+ "8: stw %3, (%1, 0) \n" \
+ " subi %0, 4 \n" \
+ " bf 7f \n" \
+ "9: subi %0, 8 \n" \
+ " bf 7f \n" \
+ "13: stw %3, (%1, 8) \n" \
+ " subi %0, 12 \n" \
+ " bf 7f \n" \
".section __ex_table, \"a\" \n" \
".align 2 \n" \
- ".long 2b, 8b \n" \
+ ".long 2b, 7f \n" \
+ ".long 4b, 7f \n" \
+ ".long 6b, 7f \n" \
".long 10b, 8b \n" \
- ".long 11b, 8b \n" \
- ".long 12b, 8b \n" \
- ".long 4b, 8b \n" \
- ".long 6b, 8b \n" \
+ ".long 11b, 9b \n" \
+ ".long 12b,13b \n" \
".previous \n" \
"7: \n" \
: "=r"(n), "=r"(to), "=r"(from), "=r"(nsave), \
diff --git a/arch/csky/lib/usercopy.c b/arch/csky/lib/usercopy.c
index 647a23986fb5..3c9bd645e643 100644
--- a/arch/csky/lib/usercopy.c
+++ b/arch/csky/lib/usercopy.c
@@ -7,10 +7,7 @@
unsigned long raw_copy_from_user(void *to, const void *from,
unsigned long n)
{
- if (access_ok(from, n))
- __copy_user_zeroing(to, from, n);
- else
- memset(to, 0, n);
+ ___copy_from_user(to, from, n);
return n;
}
EXPORT_SYMBOL(raw_copy_from_user);
@@ -18,8 +15,7 @@ EXPORT_SYMBOL(raw_copy_from_user);
unsigned long raw_copy_to_user(void *to, const void *from,
unsigned long n)
{
- if (access_ok(to, n))
- __copy_user(to, from, n);
+ ___copy_to_user(to, from, n);
return n;
}
EXPORT_SYMBOL(raw_copy_to_user);
--
2.25.1
next prev parent reply other threads:[~2020-05-22 14:55 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-22 14:50 [PATCH AUTOSEL 5.4 01/32] ARM: dts: rockchip: fix phy nodename for rk3228-evb Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 02/32] ARM: dts: rockchip: fix phy nodename for rk3229-xms6 Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 03/32] arm64: dts: rockchip: fix status for &gmac2phy in rk3328-evb.dts Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 04/32] arm64: dts: rockchip: swap interrupts interrupt-names rk3399 gpu node Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 05/32] ARM: dts: rockchip: swap clock-names of gpu nodes Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 06/32] ARM: dts: rockchip: fix pinctrl sub nodename for spi in rk322x.dtsi Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 07/32] gpio: tegra: mask GPIO IRQs during IRQ shutdown Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 08/32] ALSA: usb-audio: add mapping for ASRock TRX40 Creator Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 09/32] net: microchip: encx24j600: add missed kthread_stop Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 10/32] gfs2: move privileged user check to gfs2_quota_lock_check Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 11/32] gfs2: don't call quota_unhold if quotas are not locked Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 12/32] gfs2: Grab glock reference sooner in gfs2_add_revoke Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 13/32] drm/amdgpu: drop unnecessary cancel_delayed_work_sync on PG ungate Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 14/32] drm/amd/powerplay: perform PG ungate prior to CG ungate Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 15/32] drm/amdgpu: Use GEM obj reference for KFD BOs Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 16/32] Revert "gfs2: Don't demote a glock until its revokes are written" Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 17/32] cachefiles: Fix race between read_waiter and read_copier involving op->to_do Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 18/32] usb: dwc3: pci: Enable extcon driver for Intel Merrifield Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 19/32] usb: phy: twl6030-usb: Fix a resource leak in an error handling path in 'twl6030_usb_probe()' Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 20/32] usb: gadget: legacy: fix redundant initialization warnings Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 21/32] net: freescale: select CONFIG_FIXED_PHY where needed Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 22/32] IB/i40iw: Remove bogus call to netdev_master_upper_dev_get() Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 23/32] riscv: stacktrace: Fix undefined reference to `walk_stackframe' Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 24/32] clk: ti: am33xx: fix RTC clock parent Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 25/32] csky: Fixup msa highest 3 bits mask Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 26/32] csky: Fixup perf callchain unwind Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 27/32] csky: Fixup remove duplicate irq_disable Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 28/32] hwmon: (nct7904) Fix incorrect range of temperature limit registers Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 29/32] cifs: Fix null pointer check in cifs_read Sasha Levin
2020-05-22 14:50 ` Sasha Levin [this message]
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 31/32] samples: bpf: Fix build error Sasha Levin
2020-05-22 14:50 ` [PATCH AUTOSEL 5.4 32/32] drivers: net: hamradio: Fix suspicious RCU usage warning in bpqether.c Sasha Levin
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=20200522145044.434677-30-sashal@kernel.org \
--to=sashal@kernel.org \
--cc=guoren@linux.alibaba.com \
--cc=linux-kernel@vger.kernel.org \
--cc=stable@vger.kernel.org \
--cc=viro@zeniv.linux.org.uk \
/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).